linux-kernel - Re: [PATCH] Convert backwards goto into a while loop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230103104338.4371e012.alex.williamson@redhat.com>
Date:   Tue, 3 Jan 2023 10:43:38 -0700
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Shachar Raindel <shacharr@...gle.com>
Cc:     Cornelia Huck <cohuck@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Convert backwards goto into a while loop

On Wed, 28 Dec 2022 21:32:12 +0000
Shachar Raindel <shacharr@...gle.com> wrote:

> The function vaddr_get_pfns used a goto retry structure to implement
> retrying.  This is discouraged by the coding style guide (which is
> only recommending goto for handling function exits). Convert the code
> to a while loop, making it explicit that the follow block only runs
> when the pin attempt failed.

What coding style guide are you referring to?  In
Documentation/process/coding-style.rst I only see goto mentioned in 7)
Centralized exiting of functions, which suggests it's a useful
mechanism for creating centralized cleanup code, while noting "[a]lbeit
deprecated by *some people*", emphasis added.  This doesn't suggest to
me such a strong statement as implied in this commit log.
 
> Signed-off-by: Shachar Raindel <shacharr@...gle.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c
> b/drivers/vfio/vfio_iommu_type1.c index 23c24fe98c00..7f38d7fc3f62
> 100644 --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -570,27 +570,28 @@ static int vaddr_get_pfns(struct mm_struct *mm,
> unsigned long vaddr, }
>  
>  		*pfn = page_to_pfn(pages[0]);
> -		goto done;
> -	}
> +	} else

Coding style would however discourage skipping the braces around this
half of the branch, as done here, as a) they're used around the former
half and b) the below is not a single simple statement.  Thanks,

Alex

> +		do {
> +
> +			/* This is not a normal page, lookup PFN for P2P DMA */
> +			vaddr = untagged_addr(vaddr);
>  
> -	vaddr = untagged_addr(vaddr);
> +			vma = vma_lookup(mm, vaddr);
>  
> -retry:
> -	vma = vma_lookup(mm, vaddr);
> +			if (!vma || !(vma->vm_flags & VM_PFNMAP))
> +				break;
>  
> -	if (vma && vma->vm_flags & VM_PFNMAP) {
> -		ret = follow_fault_pfn(vma, mm, vaddr, pfn, prot & IOMMU_WRITE);
> -		if (ret == -EAGAIN)
> -			goto retry;
> +			ret = follow_fault_pfn(vma, mm, vaddr, pfn,
> +					       prot & IOMMU_WRITE);
> +			if (ret)
> +				continue; /* Retry for EAGAIN, otherwise bail */
>  
> -		if (!ret) {
>  			if (is_invalid_reserved_pfn(*pfn))
>  				ret = 1;
>  			else
>  				ret = -EFAULT;
> -		}
> -	}
> -done:
> +		} while (ret == -EAGAIN);
> +
>  	mmap_read_unlock(mm);
>  	return ret;
>  }