Messages in this thread Patch in this message | | | Date | Thu, 19 Oct 2023 10:28:21 +0800 | From | Baoquan He <> | Subject | Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667) |
| |
On 10/18/23 at 08:52am, Andrew Morton wrote: > On Wed, 18 Oct 2023 23:15:31 +0800 Baoquan He <bhe@redhat.com> wrote: > > > From: Baoquan He <bhe@redhat.com> > > Date: Wed, 18 Oct 2023 22:50:14 +0800 > > Subject: [PATCH] mm/vmalloc: fix the unchecked dereference warning in vread_iter() > > Content-type: text/plain > > > > LKP reported smatch warning as below: > > > > =================== > > smatch warnings: > > mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667) > > ...... > > 06c8994626d1b7 @3667 size = vm ? get_vm_area_size(vm) : va_size(va); > > ...... > > 06c8994626d1b7 @3689 else if (!(vm->flags & VM_IOREMAP)) > > ^^^^^^^^^ > > Unchecked dereference > > ===================== > > > > So add checking on whether 'vm' is not null when dereferencing it in > > vread_iter(). This mutes smatch complaint. > > > > ... > > > > --- a/mm/vmalloc.c > > +++ b/mm/vmalloc.c > > @@ -3813,7 +3813,7 @@ long vread_iter(struct iov_iter *iter, const char *addr, size_t count) > > > > if (flags & VMAP_RAM) > > copied = vmap_ram_vread_iter(iter, addr, n, flags); > > - else if (!(vm->flags & VM_IOREMAP)) > > + else if (!(vm && (vm->flags & VM_IOREMAP))) > > copied = aligned_vread_iter(iter, addr, n); > > else /* IOREMAP area is treated as memory hole */ > > copied = zero_iter(iter, n); > > So is this not a real runtime bug? We're only doing this to suppress a > smatch warning? > > If so, can we please include a description of *why* this wasn't a bug? > What conditions ensure that vm!=NULL at this point?
I think this is not a real runtime bug. The only chance it can hapen is when (flags == VMAP_BLOCK) is true. That has been warned and could never happen. I updated patch log and paste v2 here.
/* * VMAP_BLOCK indicates a sub-type of vm_map_ram area, need * be set together with VMAP_RAM. */ WARN_ON(flags == VMAP_BLOCK); if (!vm && !flags) continue;
From 89cc02302766ab7a67cc668390c24079b4a9406b Mon Sep 17 00:00:00 2001 From: Baoquan He <bhe@redhat.com> Date: Wed, 18 Oct 2023 22:50:14 +0800 Subject: [PATCH v2] mm/vmalloc: fix the unchecked dereference warning in vread_iter() Content-type: text/plain
LKP reported smatch warning as below:
=================== smatch warnings: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667) ...... 06c8994626d1b7 @3667 size = vm ? get_vm_area_size(vm) : va_size(va); ...... 06c8994626d1b7 @3689 else if (!(vm->flags & VM_IOREMAP)) ^^^^^^^^^ Unchecked dereference =====================
This is not a real-time bug because the possible null 'vm' in the pointed place could only happen when flags == VMAP_BLOCK. However, the case 'flags == VMAP_BLOCK' should never happen and has been detected with WARN_ON. Please check vm_map_ram() implementation and the earlier checking in vread_iter() at below:
~~~~~~~~~~~~~~~~~~~~~~~~~~ /* * VMAP_BLOCK indicates a sub-type of vm_map_ram area, need * be set together with VMAP_RAM. */ WARN_ON(flags == VMAP_BLOCK);
if (!vm && !flags) continue; ~~~~~~~~~~~~~~~~~~~~~~~~~~
So add checking on whether 'vm' could be null when dereferencing it in vread_iter(). This mutes smatch complaint.
Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Closes: https://lore.kernel.org/r/202310171600.WCrsOwFj-lkp@intel.com/ Signed-off-by: Baoquan He <bhe@redhat.com> --- v1->v2: - Update patch log to state that it's not a realtime bug as Andrew suggested.
mm/vmalloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/vmalloc.c b/mm/vmalloc.c index aad48ed8d86b..2cc992392db7 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -3813,7 +3813,7 @@ long vread_iter(struct iov_iter *iter, const char *addr, size_t count) if (flags & VMAP_RAM) copied = vmap_ram_vread_iter(iter, addr, n, flags); - else if (!(vm->flags & VM_IOREMAP)) + else if (!(vm && (vm->flags & VM_IOREMAP))) copied = aligned_vread_iter(iter, addr, n); else /* IOREMAP area is treated as memory hole */ copied = zero_iter(iter, n); -- 2.41.0
| |