[PATCH v1 1/5] memremap: Check for size parameter

[PATCH v1 1/5] memremap: Check for size parameter

[Andy Shevchenko]

There is no use of memremap() to be called with size = 0.
Simple return NULL pointer and allow callers to drop this check.

Fixes: 92281dee825f ("arch: introduce memremap()")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 kernel/iomem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/iomem.c b/kernel/iomem.c
index 62c92e43aa0d..bf7a5fc32760 100644
--- a/kernel/iomem.c
+++ b/kernel/iomem.c
@@ -74,6 +74,9 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags)
 				       IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE);
 	void *addr = NULL;
 
+	if (!size)
+		return NULL;
+
 	if (!flags)
 		return NULL;
 
-- 
2.24.0

[PATCH v1 2/5] dma-mapping: Drop duplicate check for size parameter of memremap()

[Andy Shevchenko]

Since memremap() returns NULL pointer for size = 0, there is no need
to duplicate this check in the callers.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 kernel/dma/coherent.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c
index 551b0eb7028a..7e669c083324 100644
--- a/kernel/dma/coherent.c
+++ b/kernel/dma/coherent.c
@@ -42,16 +42,11 @@ static int dma_init_coherent_memory(phys_addr_t phys_addr,
 		struct dma_coherent_mem **mem)
 {
 	struct dma_coherent_mem *dma_mem = NULL;
-	void *mem_base = NULL;
+	void *mem_base;
 	int pages = size >> PAGE_SHIFT;
 	int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
 	int ret;
 
-	if (!size) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	mem_base = memremap(phys_addr, size, MEMREMAP_WC);
 	if (!mem_base) {
 		ret = -EINVAL;
-- 
2.24.0

Re: [PATCH v1 2/5] dma-mapping: Drop duplicate check for size parameter of memremap()

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v1 3/5] ALSA: hda: Drop duplicate check for size parameter of memremap()

[Andy Shevchenko]

Since memremap() returns NULL pointer for size = 0, there is no need
to duplicate this check in the callers.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 sound/hda/intel-nhlt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c
index 097ff6c10099..5fc10f1b244c 100644
--- a/sound/hda/intel-nhlt.c
+++ b/sound/hda/intel-nhlt.c
@@ -16,7 +16,7 @@ struct nhlt_acpi_table *intel_nhlt_init(struct device *dev)
 	acpi_handle handle;
 	union acpi_object *obj;
 	struct nhlt_resource_desc *nhlt_ptr;
-	struct nhlt_acpi_table *nhlt_table = NULL;
+	struct nhlt_acpi_table *nhlt_table;
 
 	handle = ACPI_HANDLE(dev);
 	if (!handle) {
@@ -36,8 +36,7 @@ struct nhlt_acpi_table *intel_nhlt_init(struct device *dev)
 	}
 
 	nhlt_ptr = (struct nhlt_resource_desc  *)obj->buffer.pointer;
-	if (nhlt_ptr->length)
-		nhlt_table = (struct nhlt_acpi_table *)
+	nhlt_table = (struct nhlt_acpi_table *)
 			memremap(nhlt_ptr->min_addr, nhlt_ptr->length,
 				 MEMREMAP_WB);
 	ACPI_FREE(obj);
-- 
2.24.0

[PATCH v1 4/5] efi/esrt: Drop duplicate check for size parameter of memremap()

[Andy Shevchenko]

Since memremap() returns NULL pointer for size = 0, there is no need
to duplicate this check in the callers.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/firmware/efi/esrt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index 2762e0662bf4..dce56a4c4011 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -381,7 +381,7 @@ static int __init esrt_sysfs_init(void)
 	int error;
 
 	pr_debug("esrt-sysfs: loading.\n");
-	if (!esrt_data || !esrt_data_size)
+	if (!esrt_data)
 		return -ENOSYS;
 
 	esrt = memremap(esrt_data, esrt_data_size, MEMREMAP_WB);
-- 
2.24.0

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

[Andy Shevchenko]

Since memremap() returns NULL pointer for size = 0, there is no need
to duplicate this check in the callers.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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);
-- 
2.24.0

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@linux.intel.com> wrote:

> Since memremap() returns NULL pointer for size = 0, there is no need
> to duplicate this check in the callers.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  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

Re: [PATCH v1 1/5] memremap: Check for size parameter

[Christoph Hellwig]

On Fri, Nov 15, 2019 at 08:00:40PM +0200, Andy Shevchenko wrote:
> There is no use of memremap() to be called with size = 0.
> Simple return NULL pointer and allow callers to drop this check.

Given that this really is an error condition, maybe a WARN_ON_ONCE
would fit here?

Re: [PATCH v1 1/5] memremap: Check for size parameter

[Andy Shevchenko]

On Sat, Nov 16, 2019 at 05:29:37PM +0100, Christoph Hellwig wrote:
> On Fri, Nov 15, 2019 at 08:00:40PM +0200, Andy Shevchenko wrote:
> > There is no use of memremap() to be called with size = 0.
> > Simple return NULL pointer and allow callers to drop this check.
> 
> Given that this really is an error condition, maybe a WARN_ON_ONCE
> would fit here?

It appears some users are using defensive programming and rely simple on error
code. I dunno if they are really expect to have size == 0 in some (non-fatal)
cases.

-- 
With Best Regards,
Andy Shevchenko