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 <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Marek Szyprowski <[email protected]>
Cc: Robin Murphy <[email protected]>
Cc: Jaroslav Kysela <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: Pierre-Louis Bossart <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Alex Williamson <[email protected]>
Cc: Cornelia Huck <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
Since memremap() returns NULL pointer for size = 0, there is no need
to duplicate this check in the callers.
Cc: Christoph Hellwig <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Alex Williamson <[email protected]>
Cc: Cornelia Huck <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
On Fri, 15 Nov 2019 20:00:44 +0200
Andy Shevchenko <[email protected]> wrote:
> Since memremap() returns NULL pointer for size = 0, there is no need
> to duplicate this check in the callers.
>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: Alex Williamson <[email protected]>
> Cc: Cornelia Huck <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> 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
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?
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