[PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping()

[PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping()

[Sean Christopherson]

Drop the local "int err" that's buried in the middle guest_memfd's user
fault handler to avoid the potential for variable shadowing, e.g. if an
"err" variable were also declared at function scope.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/guest_memfd.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index 94bafd6c558c..abbec01d7a3a 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -330,12 +330,10 @@ static vm_fault_t kvm_gmem_fault_user_mapping(struct vm_fault *vmf)
 
 	folio = kvm_gmem_get_folio(inode, vmf->pgoff);
 	if (IS_ERR(folio)) {
-		int err = PTR_ERR(folio);
-
-		if (err == -EAGAIN)
+		if (PTR_ERR(folio) == -EAGAIN)
 			return VM_FAULT_RETRY;
 
-		return vmf_error(err);
+		return vmf_error(PTR_ERR(folio));
 	}
 
 	if (WARN_ON_ONCE(folio_test_large(folio))) {

base-commit: 6b36119b94d0b2bb8cea9d512017efafd461d6ac
-- 
2.51.0.710.ga91ca5db03-goog

Re: [PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping()

[Ackerley Tng]

Sean Christopherson <seanjc@google.com> writes:

> Drop the local "int err" that's buried in the middle guest_memfd's user
> fault handler to avoid the potential for variable shadowing, e.g. if an
> "err" variable were also declared at function scope.
>

Is the takeaway here that the variable name "err", if used, should be
defined at function scope?

IOW, would this code have been okay if any other variable name were
used, like if err_folio were used instead of err?

> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Ackerley Tng <ackerleytng@google.com>

> ---
>  virt/kvm/guest_memfd.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 94bafd6c558c..abbec01d7a3a 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -330,12 +330,10 @@ static vm_fault_t kvm_gmem_fault_user_mapping(struct vm_fault *vmf)
>  
>  	folio = kvm_gmem_get_folio(inode, vmf->pgoff);
>  	if (IS_ERR(folio)) {
> -		int err = PTR_ERR(folio);
> -
> -		if (err == -EAGAIN)
> +		if (PTR_ERR(folio) == -EAGAIN)
>  			return VM_FAULT_RETRY;
>  
> -		return vmf_error(err);
> +		return vmf_error(PTR_ERR(folio));
>  	}
>  
>  	if (WARN_ON_ONCE(folio_test_large(folio))) {
>
> base-commit: 6b36119b94d0b2bb8cea9d512017efafd461d6ac

Re: [PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping()

[Sean Christopherson]

On Thu, Oct 09, 2025, Ackerley Tng wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > Drop the local "int err" that's buried in the middle guest_memfd's user
> > fault handler to avoid the potential for variable shadowing, e.g. if an
> > "err" variable were also declared at function scope.
> >
> 
> Is the takeaway here that the variable name "err", if used, should be
> defined at function scope?

Generally speaking, for generic variables like "err", "r", and "ret", yes, because
the danger of shadowing is high, while the odds of _wanting_ to contain a return
code are low.

But as a broad rule, there's simply no "right" answer other than "it depends".
As Paolo put it, damned if you do, damned if you don't.  See this thread for more:

https://lore.kernel.org/all/YZL1ZiKQVRQd8rZi@google.com

> IOW, would this code have been okay if any other variable name were
> used, like if err_folio were used instead of err?

Probably not?  Because that has it's own problems.  E.g. then you can end up
introducing bugs like this:

	int err;

	err = blah();
	if (err)
		goto fault_err;

        folio = kvm_gmem_get_folio(inode, vmf->pgoff);
        if (IS_ERR(folio)) {
		int folio_err = PTR_ERR(folio);

		if (folio_err == -EAGAIN)
                        return VM_FAULT_RETRY;

		goto fault_err;
        }

	...

fault_err:
	return vmf_error(err);

Re: [PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping()

[Ackerley Tng]

Sean Christopherson <seanjc@google.com> writes:

> On Thu, Oct 09, 2025, Ackerley Tng wrote:
>> Sean Christopherson <seanjc@google.com> writes:
>> 
>> > Drop the local "int err" that's buried in the middle guest_memfd's user
>> > fault handler to avoid the potential for variable shadowing, e.g. if an
>> > "err" variable were also declared at function scope.
>> >
>> 
>> Is the takeaway here that the variable name "err", if used, should be
>> defined at function scope?
>
> Generally speaking, for generic variables like "err", "r", and "ret", yes, because
> the danger of shadowing is high, while the odds of _wanting_ to contain a return
> code are low.
>
> But as a broad rule, there's simply no "right" answer other than "it depends".
> As Paolo put it, damned if you do, damned if you don't.  See this thread for more:
>
> https://lore.kernel.org/all/YZL1ZiKQVRQd8rZi@google.com
>
>> IOW, would this code have been okay if any other variable name were
>> used, like if err_folio were used instead of err?
>
> Probably not?  Because that has it's own problems.  E.g. then you can end up
> introducing bugs like this:
>
> 	int err;
>
> 	err = blah();
> 	if (err)
> 		goto fault_err;
>
>         folio = kvm_gmem_get_folio(inode, vmf->pgoff);
>         if (IS_ERR(folio)) {
> 		int folio_err = PTR_ERR(folio);
>
> 		if (folio_err == -EAGAIN)
>                         return VM_FAULT_RETRY;
>
> 		goto fault_err;
>         }
>
> 	...
>
> fault_err:
> 	return vmf_error(err);

This is true too. Thanks, I see why it depends.

Re: [PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping()

[Sean Christopherson]

On Tue, 07 Oct 2025 15:27:33 -0700, Sean Christopherson wrote:
> Drop the local "int err" that's buried in the middle guest_memfd's user
> fault handler to avoid the potential for variable shadowing, e.g. if an
> "err" variable were also declared at function scope.
> 
> No functional change intended.
> 
> 
> [...]

Applied to kvm-x86 gmem, thanks!

[1/1] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping()
      https://github.com/kvm-x86/linux/commit/c1168f24b444

--
https://github.com/kvm-x86/linux/tree/next

Re: [PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping()

[Sean Christopherson]

On Wed, Oct 15, 2025, Sean Christopherson wrote:
> On Tue, 07 Oct 2025 15:27:33 -0700, Sean Christopherson wrote:
> > Drop the local "int err" that's buried in the middle guest_memfd's user
> > fault handler to avoid the potential for variable shadowing, e.g. if an
> > "err" variable were also declared at function scope.
> > 
> > No functional change intended.
> > 
> > 
> > [...]
> 
> Applied to kvm-x86 gmem, thanks!
> 
> [1/1] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping()
>       https://github.com/kvm-x86/linux/commit/c1168f24b444

FYI, I rebased this onto 6.18-rc2 to avoid a silly merge.  New hash:

[1/1] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping()
      https://github.com/kvm-x86/linux/commit/5f3e10797ab8