Re: [PATCH v1 5/5] vfio/pci: Drop duplicate check for size parameter of memremap()

[Alex Williamson]

On Fri, 15 Nov 2019 20:00:44 +0200
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:

> Since memremap() returns NULL pointer for size = 0, there is no need
> to duplicate this check in the callers.
> 
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Cc: Cornelia Huck <cohuck@xxxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>  drivers/vfio/pci/vfio_pci_igd.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_igd.c b/drivers/vfio/pci/vfio_pci_igd.c
> index 53d97f459252..3088a33af271 100644
> --- a/drivers/vfio/pci/vfio_pci_igd.c
> +++ b/drivers/vfio/pci/vfio_pci_igd.c
> @@ -75,13 +75,7 @@ static int vfio_pci_igd_opregion_init(struct vfio_pci_device *vdev)
>  		return -EINVAL;
>  	}
>  
> -	size = le32_to_cpu(*(__le32 *)(base + 16));
> -	if (!size) {
> -		memunmap(base);
> -		return -EINVAL;
> -	}
> -
> -	size *= 1024; /* In KB */
> +	size = le32_to_cpu(*(__le32 *)(base + 16)) * 1024; /* In KB */
>  
>  	if (size != OPREGION_SIZE) {
>  		memunmap(base);

This seems convoluted, patch 1/5 states "[t]here is no use of memremap()
to be called with size = 0", which we weren't doing thanks to the check
removed above.  So now we are potentially calling it with zero,
apparently only to take advantage of this new behavior, and we lose the
error granularity that previously such a condition failed with an
-EINVAL and now we fail with an -ENONMEM and cannot distinguish whether
the OpRegion table size was empty or we just weren't able to memremap()
it.  I don't see how this is an improvement.  Thanks,

Alex