LinuxLists.cc - mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)

2023-10-17 14:27:01

Subject: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)

tree: If you fix the issue the same patch/commit), | Reported-by: kernel | Reported-by: Dan Carpenter | Closes: kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git">https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
e1ce7ae1f47921cb9c
2beb7ee5c2a48ad8e7 mm: vmalloc: convert vread() to vread_iter()
href="https://download.01.org/0day-ci/archive/20231017/202310171600.WCrsOwFj-lkp@intel.com/config)">https://download.01.org/0day-ci/archive/20231017/[email protected]/config)
12.2.0-14) 12.2.0
download.01.org/0day-ci/archive/20231017/202310171600.WCrsOwFj-lkp@intel.com/reproduce)">https://download.01.org/0day-ci/archive/20231017/[email protected]/reproduce)
in a separate patch/commit (i.e. not just a new version of
kindly add following tags
test robot <[email protected]>
<[email protected]>
re.kernel.org/r/202310171600.WCrsOwFj-lkp@intel.com/">https://lore.kernel.org/r/[email protected]/
error: we previously assumed 'vm' could be null (see line 3667)
2023-03-22 3619 long vread_iter(struct iov_iter *iter, const char *addr, size_t count)
2005-04-16 3620 {
2013-04-29 3621 struct vmap_area *va;
2013-04-29 3622 struct vm_struct *vm;
2023-03-22 3623 char *vaddr;
2023-03-22 3624 size_t n, size, flags, remains;
2005-04-16 3625
2022-03-24 3626 addr = kasan_reset_tag(addr);
2022-03-24 3627
2005-04-16 3628 /* Don't allow overflow */
2005-04-16 3629 if ((unsigned long) addr + count < count)
2005-04-16 3630 count = -(unsigned long) addr;
2005-04-16 3631
2023-03-22 3632 remains = count;
2023-03-22 3633
2013-04-29 3634 spin_lock(&vmap_area_lock);
2021-09-02 3635 va = find_vmap_area_exceed_addr((unsigned long)addr);
Dimitropoulos 2021-04-29 3636 if (!va)
2023-03-22 3637 goto finished_zero;
2021-09-02 3638
2021-09-02 3639 /* no intersects with alive vmap_area */
2023-03-22 3640 if ((unsigned long)addr + remains <= va->va_start)
2023-03-22 3641 goto finished_zero;
2021-09-02 3642
Dimitropoulos 2021-04-29 3643 list_for_each_entry_from(va, &vmap_area_list, list) {
2023-03-22 3644 size_t copied;
2023-03-22 3645
2023-03-22 3646 if (remains == 0)
2023-03-22 3647 goto finished;
2013-04-29 3648
2023-02-06 3649 vm = va->vm;
2023-02-06 3650 flags = va->flags & VMAP_FLAGS_MASK;
2023-02-06 3651 /*
2023-02-06 3652 * VMAP_BLOCK indicates a sub-type of vm_map_ram area, need
2023-02-06 3653 * be set together with VMAP_RAM.
2023-02-06 3654 */
2023-02-06 3655 WARN_ON(flags == VMAP_BLOCK);
2023-02-06 3656
2023-02-06 3657 if (!vm && !flags)
2013-04-29 3658 continue;
2013-04-29 3659
2023-02-06 3660 if (vm && (vm->flags & VM_UNINITIALIZED))
2023-02-06 3661 continue;
2023-03-22 3662
2023-02-06 3663 /* Pair with smp_wmb() in clear_vm_uninitialized_flag() */
2023-02-06 3664 smp_rmb();
2023-02-06 3665
2023-02-06 3666 vaddr = (char *) va->va_start;
2023-02-06 @3667 size = vm ? get_vm_area_size(vm) : va_size(va);
2023-02-06 3668
2023-02-06 3669 if (addr >= vaddr + size)
2005-04-16 3670 continue;
2023-03-22 3671
2023-03-22 3672 if (addr < vaddr) {
2023-03-22 3673 size_t to_zero = min_t(size_t, vaddr - addr, remains);
2023-03-22 3674 size_t zeroed = zero_iter(iter, to_zero);
2023-03-22 3675
2023-03-22 3676 addr += zeroed;
2023-03-22 3677 remains -= zeroed;
2023-03-22 3678
2023-03-22 3679 if (remains == 0 || zeroed != to_zero)
2005-04-16 3680 goto finished;
2005-04-16 3681 }
2023-03-22 3682
2023-02-06 3683 n = vaddr + size - addr;
2023-03-22 3684 if (n > remains)
2023-03-22 3685 n = remains;
2023-02-06 3686
2023-02-06 3687 if (flags & VMAP_RAM)
2023-03-22 3688 copied = vmap_ram_vread_iter(iter, addr, n, flags);
2023-02-06 @3689 else if (!(vm->flags & VM_IOREMAP))
^^^^^^^^^
2023-03-22 3690 copied = aligned_vread_iter(iter, addr, n);
2009-09-21 3691 else /* IOREMAP area is treated as memory hole */
2023-03-22 3692 copied = zero_iter(iter, n);
2023-03-22 3693
2023-03-22 3694 addr += copied;
2023-03-22 3695 remains -= copied;
2023-03-22 3696
2023-03-22 3697 if (copied != n)
2023-03-22 3698 goto finished;
2005-04-16 3699 }
2009-09-21 3700
2023-03-22 3701 finished_zero:
2023-03-22 3702 spin_unlock(&vmap_area_lock);
2009-09-21 3703 /* zero-fill memory holes */
2023-03-22 3704 return count - remains + zero_iter(iter, remains);
2023-03-22 3705 finished:
2023-03-22 3706 /* Nothing remains, or We couldn't copy/zero everything. */
2023-03-22 3707 spin_unlock(&vmap_area_lock);
2009-09-21 3708
2023-03-22 3709 return count - remains;
2005-04-16 3710 }
ntel/lkp-tests/wiki">https://github.com/intel/lkp-tests/wiki

2023-10-18 08:56:04

by Baoquan He

[permalink] [raw]

Subject: Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)

Hi,

On 10/17/23 at 05:26pm, Dan Carpenter wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: 213f891525c222e8ed145ce1ce7ae1f47921cb9c
> commit: 4c91c07c93bbbdd7f2d9de2beb7ee5c2a48ad8e7 mm: vmalloc: convert vread() to vread_iter()
> config: x86_64-allnoconfig (https://download.01.org/0day-ci/archive/20231017/[email protected]/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20231017/[email protected]/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <[email protected]>
> | Reported-by: Dan Carpenter <[email protected]>
> | Closes: https://lore.kernel.org/r/[email protected]/
>
> smatch warnings:
> mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)

I see the code deficit, while the reproduce link seems to be unavilable.
Could you double check the link and provide a good one so that I can
verify the code fix?

Thanks
Baoquan

>
> vim +/vm +3689 mm/vmalloc.c
>
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3619 long vread_iter(struct iov_iter *iter, const char *addr, size_t count)
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 3620 {
> e81ce85f960c2e Joonsoo Kim 2013-04-29 3621 struct vmap_area *va;
> e81ce85f960c2e Joonsoo Kim 2013-04-29 3622 struct vm_struct *vm;
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3623 char *vaddr;
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3624 size_t n, size, flags, remains;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 3625
> 4aff1dc4fb3a5a Andrey Konovalov 2022-03-24 3626 addr = kasan_reset_tag(addr);
> 4aff1dc4fb3a5a Andrey Konovalov 2022-03-24 3627
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 3628 /* Don't allow overflow */
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 3629 if ((unsigned long) addr + count < count)
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 3630 count = -(unsigned long) addr;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 3631
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3632 remains = count;
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3633
> e81ce85f960c2e Joonsoo Kim 2013-04-29 3634 spin_lock(&vmap_area_lock);
> f181234a5a21fd Chen Wandun 2021-09-02 3635 va = find_vmap_area_exceed_addr((unsigned long)addr);
> f608788cd2d6ca Serapheim Dimitropoulos 2021-04-29 3636 if (!va)
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3637 goto finished_zero;
> f181234a5a21fd Chen Wandun 2021-09-02 3638
> f181234a5a21fd Chen Wandun 2021-09-02 3639 /* no intersects with alive vmap_area */
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3640 if ((unsigned long)addr + remains <= va->va_start)
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3641 goto finished_zero;
> f181234a5a21fd Chen Wandun 2021-09-02 3642
> f608788cd2d6ca Serapheim Dimitropoulos 2021-04-29 3643 list_for_each_entry_from(va, &vmap_area_list, list) {
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3644 size_t copied;
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3645
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3646 if (remains == 0)
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3647 goto finished;
> e81ce85f960c2e Joonsoo Kim 2013-04-29 3648
> 06c8994626d1b7 Baoquan He 2023-02-06 3649 vm = va->vm;
> 06c8994626d1b7 Baoquan He 2023-02-06 3650 flags = va->flags & VMAP_FLAGS_MASK;
> 06c8994626d1b7 Baoquan He 2023-02-06 3651 /*
> 06c8994626d1b7 Baoquan He 2023-02-06 3652 * VMAP_BLOCK indicates a sub-type of vm_map_ram area, need
> 06c8994626d1b7 Baoquan He 2023-02-06 3653 * be set together with VMAP_RAM.
> 06c8994626d1b7 Baoquan He 2023-02-06 3654 */
> 06c8994626d1b7 Baoquan He 2023-02-06 3655 WARN_ON(flags == VMAP_BLOCK);
> 06c8994626d1b7 Baoquan He 2023-02-06 3656
> 06c8994626d1b7 Baoquan He 2023-02-06 3657 if (!vm && !flags)
>
> NULL check
>
> e81ce85f960c2e Joonsoo Kim 2013-04-29 3658 continue;
> e81ce85f960c2e Joonsoo Kim 2013-04-29 3659
> 30a7a9b17c4b03 Baoquan He 2023-02-06 3660 if (vm && (vm->flags & VM_UNINITIALIZED))
> 30a7a9b17c4b03 Baoquan He 2023-02-06 3661 continue;
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3662
> 30a7a9b17c4b03 Baoquan He 2023-02-06 3663 /* Pair with smp_wmb() in clear_vm_uninitialized_flag() */
> 30a7a9b17c4b03 Baoquan He 2023-02-06 3664 smp_rmb();
> 30a7a9b17c4b03 Baoquan He 2023-02-06 3665
> 06c8994626d1b7 Baoquan He 2023-02-06 3666 vaddr = (char *) va->va_start;
> 06c8994626d1b7 Baoquan He 2023-02-06 @3667 size = vm ? get_vm_area_size(vm) : va_size(va);
> 06c8994626d1b7 Baoquan He 2023-02-06 3668
> 06c8994626d1b7 Baoquan He 2023-02-06 3669 if (addr >= vaddr + size)
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 3670 continue;
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3671
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3672 if (addr < vaddr) {
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3673 size_t to_zero = min_t(size_t, vaddr - addr, remains);
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3674 size_t zeroed = zero_iter(iter, to_zero);
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3675
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3676 addr += zeroed;
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3677 remains -= zeroed;
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3678
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3679 if (remains == 0 || zeroed != to_zero)
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 3680 goto finished;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 3681 }
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3682
> 06c8994626d1b7 Baoquan He 2023-02-06 3683 n = vaddr + size - addr;
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3684 if (n > remains)
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3685 n = remains;
> 06c8994626d1b7 Baoquan He 2023-02-06 3686
> 06c8994626d1b7 Baoquan He 2023-02-06 3687 if (flags & VMAP_RAM)
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3688 copied = vmap_ram_vread_iter(iter, addr, n, flags);
> 06c8994626d1b7 Baoquan He 2023-02-06 @3689 else if (!(vm->flags & VM_IOREMAP))
> ^^^^^^^^^
> Unchecked dereference
>
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3690 copied = aligned_vread_iter(iter, addr, n);
> d0107eb07320b5 KAMEZAWA Hiroyuki 2009-09-21 3691 else /* IOREMAP area is treated as memory hole */
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3692 copied = zero_iter(iter, n);
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3693
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3694 addr += copied;
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3695 remains -= copied;
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3696
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3697 if (copied != n)
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3698 goto finished;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 3699 }
> d0107eb07320b5 KAMEZAWA Hiroyuki 2009-09-21 3700
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3701 finished_zero:
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3702 spin_unlock(&vmap_area_lock);
> d0107eb07320b5 KAMEZAWA Hiroyuki 2009-09-21 3703 /* zero-fill memory holes */
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3704 return count - remains + zero_iter(iter, remains);
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3705 finished:
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3706 /* Nothing remains, or We couldn't copy/zero everything. */
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3707 spin_unlock(&vmap_area_lock);
> d0107eb07320b5 KAMEZAWA Hiroyuki 2009-09-21 3708
> 4c91c07c93bbbd Lorenzo Stoakes 2023-03-22 3709 return count - remains;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 3710 }
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>

2023-10-18 10:56:45

by Dan Carpenter

[permalink] [raw]

Subject: Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)

On Wed, Oct 18, 2023 at 04:54:33PM +0800, Baoquan He wrote:
> Hi,
>
> On 10/17/23 at 05:26pm, Dan Carpenter wrote:
> > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head: 213f891525c222e8ed145ce1ce7ae1f47921cb9c
> > commit: 4c91c07c93bbbdd7f2d9de2beb7ee5c2a48ad8e7 mm: vmalloc: convert vread() to vread_iter()
> > config: x86_64-allnoconfig (https://download.01.org/0day-ci/archive/20231017/[email protected]/config)
> > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > reproduce: (https://download.01.org/0day-ci/archive/20231017/[email protected]/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <[email protected]>
> > | Reported-by: Dan Carpenter <[email protected]>
> > | Closes: https://lore.kernel.org/r/[email protected]/
> >
> > smatch warnings:
> > mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
>
> I see the code deficit, while the reproduce link seems to be unavilable.
> Could you double check the link and provide a good one so that I can
> verify the code fix?

Here's a link. :)

https://repo.or.cz/smatch.git/blob/HEAD:/Documentation/smatch.txt

Just build it and run:

~/smatch/smatch_scripts/kchecker drivers/whatever/file.c

regards,
dan carpenter

2023-10-18 12:13:43

by Baoquan He

[permalink] [raw]

Subject: Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)

Hi Dan,

On 10/18/23 at 01:32pm, Dan Carpenter wrote:
> On Wed, Oct 18, 2023 at 04:54:33PM +0800, Baoquan He wrote:
> > Hi,
> >
> > On 10/17/23 at 05:26pm, Dan Carpenter wrote:
> > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > head: 213f891525c222e8ed145ce1ce7ae1f47921cb9c
> > > commit: 4c91c07c93bbbdd7f2d9de2beb7ee5c2a48ad8e7 mm: vmalloc: convert vread() to vread_iter()
> > > config: x86_64-allnoconfig (https://download.01.org/0day-ci/archive/20231017/[email protected]/config)
> > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > > reproduce: (https://download.01.org/0day-ci/archive/20231017/[email protected]/reproduce)
> > >
> > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > the same patch/commit), kindly add following tags
> > > | Reported-by: kernel test robot <[email protected]>
> > > | Reported-by: Dan Carpenter <[email protected]>
> > > | Closes: https://lore.kernel.org/r/[email protected]/
> > >
> > > smatch warnings:
> > > mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
> >
> > I see the code deficit, while the reproduce link seems to be unavilable.
> > Could you double check the link and provide a good one so that I can
> > verify the code fix?
>
> Here's a link. :)
>
> https://repo.or.cz/smatch.git/blob/HEAD:/Documentation/smatch.txt
>
> Just build it and run:
>
> ~/smatch/smatch_scripts/kchecker drivers/whatever/file.c

I don't know smatch and lkp well, and have no idea on how to use above
smatch.txt to build the target file.c. I meant in this lkp report, the
config file is available, however, the reproduce file is empty. Could
you help add an available reproduce file link? or give a little more
detail guiding me how to make use of above smatch file to build .c file?
Thanks a lot in advance.

config: x86_64-allnoconfig (https://download.01.org/0day-ci/archive/20231017/[email protected]/config)
reproduce: (https://download.01.org/0day-ci/archive/20231017/[email protected]/reproduce)

Thanks
Baoquan

2023-10-18 12:46:07

by Philip Li

[permalink] [raw]

Subject: Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)

On Wed, Oct 18, 2023 at 08:12:30PM +0800, Baoquan He wrote:
> Hi Dan,
>
> On 10/18/23 at 01:32pm, Dan Carpenter wrote:
> > On Wed, Oct 18, 2023 at 04:54:33PM +0800, Baoquan He wrote:
> > > Hi,
> > >
> > > On 10/17/23 at 05:26pm, Dan Carpenter wrote:
> > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > > head: 213f891525c222e8ed145ce1ce7ae1f47921cb9c
> > > > commit: 4c91c07c93bbbdd7f2d9de2beb7ee5c2a48ad8e7 mm: vmalloc: convert vread() to vread_iter()
> > > > config: x86_64-allnoconfig (https://download.01.org/0day-ci/archive/20231017/[email protected]/config)
> > > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > > > reproduce: (https://download.01.org/0day-ci/archive/20231017/[email protected]/reproduce)
> > > >
> > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > > the same patch/commit), kindly add following tags
> > > > | Reported-by: kernel test robot <[email protected]>
> > > > | Reported-by: Dan Carpenter <[email protected]>
> > > > | Closes: https://lore.kernel.org/r/[email protected]/
> > > >
> > > > smatch warnings:
> > > > mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
> > >
> > > I see the code deficit, while the reproduce link seems to be unavilable.
> > > Could you double check the link and provide a good one so that I can
> > > verify the code fix?
> >
> > Here's a link. :)
> >
> > https://repo.or.cz/smatch.git/blob/HEAD:/Documentation/smatch.txt
> >
> > Just build it and run:
> >
> > ~/smatch/smatch_scripts/kchecker drivers/whatever/file.c
>
> I don't know smatch and lkp well, and have no idea on how to use above

Hi Baoquan, sorry there's issue in the generation of reproduce step, but even
it is generated, it doesn't contain the detail to setup smatch. You can follow
the smatch.txt to do the setup.

> smatch.txt to build the target file.c. I meant in this lkp report, the
> config file is available, however, the reproduce file is empty. Could
> you help add an available reproduce file link? or give a little more
> detail guiding me how to make use of above smatch file to build .c file?

On the other side, Dan has added analysis to the report as below.
It's possible to resolve the issue without running the smatch check.
You can give this a try.

06c8994626d1b7 Baoquan He 2023-02-06 3657 if (!vm && !flags)

NULL check

...

06c8994626d1b7 Baoquan He 2023-02-06 @3689 else if (!(vm->flags & VM_IOREMAP))
^^^^^^^^^
Unchecked dereference

> Thanks a lot in advance.
>
> config: x86_64-allnoconfig (https://download.01.org/0day-ci/archive/20231017/[email protected]/config)
> reproduce: (https://download.01.org/0day-ci/archive/20231017/[email protected]/reproduce)

Meanwhile, we will resolve the reproduce empty issue as early as possible.

>
> Thanks
> Baoquan
>
>

2023-10-18 15:16:53

by Baoquan He

[permalink] [raw]

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:45pm, Philip Li wrote:
> On Wed, Oct 18, 2023 at 08:12:30PM +0800, Baoquan He wrote:
> > Hi Dan,
> >
> > On 10/18/23 at 01:32pm, Dan Carpenter wrote:
> > > On Wed, Oct 18, 2023 at 04:54:33PM +0800, Baoquan He wrote:
> > > > Hi,
> > > >
> > > > On 10/17/23 at 05:26pm, Dan Carpenter wrote:
> > > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > > > head: 213f891525c222e8ed145ce1ce7ae1f47921cb9c
> > > > > commit: 4c91c07c93bbbdd7f2d9de2beb7ee5c2a48ad8e7 mm: vmalloc: convert vread() to vread_iter()
> > > > > config: x86_64-allnoconfig (https://download.01.org/0day-ci/archive/20231017/[email protected]/config)
> > > > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > > > > reproduce: (https://download.01.org/0day-ci/archive/20231017/[email protected]/reproduce)
> > > > >
> > > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > > > the same patch/commit), kindly add following tags
> > > > > | Reported-by: kernel test robot <[email protected]>
> > > > > | Reported-by: Dan Carpenter <[email protected]>
> > > > > | Closes: https://lore.kernel.org/r/[email protected]/
> > > > >
> > > > > smatch warnings:
> > > > > mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
> > > >
> > > > I see the code deficit, while the reproduce link seems to be unavilable.
> > > > Could you double check the link and provide a good one so that I can
> > > > verify the code fix?
> > >
> > > Here's a link. :)
> > >
> > > https://repo.or.cz/smatch.git/blob/HEAD:/Documentation/smatch.txt
> > >
> > > Just build it and run:
> > >
> > > ~/smatch/smatch_scripts/kchecker drivers/whatever/file.c
> >
> > I don't know smatch and lkp well, and have no idea on how to use above
>
> Hi Baoquan, sorry there's issue in the generation of reproduce step, but even
> it is generated, it doesn't contain the detail to setup smatch. You can follow
> the smatch.txt to do the setup.
>
> > smatch.txt to build the target file.c. I meant in this lkp report, the
> > config file is available, however, the reproduce file is empty. Could
> > you help add an available reproduce file link? or give a little more
> > detail guiding me how to make use of above smatch file to build .c file?
>
> On the other side, Dan has added analysis to the report as below.
> It's possible to resolve the issue without running the smatch check.
> You can give this a try.
>
> 06c8994626d1b7 Baoquan He 2023-02-06 3657 if (!vm && !flags)
>
> NULL check
>
> ...
>
> 06c8994626d1b7 Baoquan He 2023-02-06 @3689 else if (!(vm->flags & VM_IOREMAP))
> ^^^^^^^^^
> Unchecked dereference

Thanks for the detailed explanation, Philipp.

Yes, the code deficit is identified. I planned to reproduce and verify
with LKP's reproducer so that it's 100% clear. Anyway, below patch
should fix it.

From a39fdd50030a7d88a7f69bcba29e6f75020a1915 Mon Sep 17 00:00:00 2001
From: Baoquan He <[email protected]>
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.

Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
Closes: https://lore.kernel.org/r/[email protected]/
Signed-off-by: Baoquan He <[email protected]>
---
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

2023-10-18 15:53:04

by Andrew Morton

[permalink] [raw]

Subject: Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)

On Wed, 18 Oct 2023 23:15:31 +0800 Baoquan He <[email protected]> wrote:

> From: Baoquan He <[email protected]>
> 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?

2023-10-19 02:29:43

by Baoquan He

[permalink] [raw]

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 <[email protected]> wrote:
>
> > From: Baoquan He <[email protected]>
> > 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 <[email protected]>
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 <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
Closes: https://lore.kernel.org/r/[email protected]/
Signed-off-by: Baoquan He <[email protected]>
---
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

2023-10-19 05:42:05

by Dan Carpenter

[permalink] [raw]

Subject: Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)

On Thu, Oct 19, 2023 at 10:28:21AM +0800, Baoquan He wrote:
> On 10/18/23 at 08:52am, Andrew Morton wrote:
> > On Wed, 18 Oct 2023 23:15:31 +0800 Baoquan He <[email protected]> wrote:
> >
> > > From: Baoquan He <[email protected]>
> > > 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;
>
>

Thanks. If you want you could just ignore the warning. It's a one time
warning so we won't send the mail again and if people have questions
about it, they can just look it up on lore.

The truth is when I was reviewing this code the first time I got mixed
up between flags and vm->flags so that's part of why I reported it.

Smatch ignores the WARN_ON(). Historically WARN_ON() has been useless
for indicating whether something can happen or not. These days,
WARN_ON() is treated as a syzkaller bug so we prefer pr_warn() if
something can actually happen. We still see a lot of WARN_ON()s
happening in real life so I'm not eager to make Smatch treat them like a
BUG_ON().

Also, sadly, even if we changed the WARN_ON() to a BUG_ON() it still
wouldn't silence the warning because Smatch is not quite clever enough
to parse that.

regards,
dan carpenter

2023-10-19 13:00:05

by Baoquan He

[permalink] [raw]

Subject: Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)

On 10/19/23 at 08:40am, Dan Carpenter wrote:
> On Thu, Oct 19, 2023 at 10:28:21AM +0800, Baoquan He wrote:
> > On 10/18/23 at 08:52am, Andrew Morton wrote:
> > > On Wed, 18 Oct 2023 23:15:31 +0800 Baoquan He <[email protected]> wrote:
> > >
> > > > From: Baoquan He <[email protected]>
> > > > 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;
> >
> >
>
> Thanks. If you want you could just ignore the warning. It's a one time
> warning so we won't send the mail again and if people have questions
> about it, they can just look it up on lore.
>
> The truth is when I was reviewing this code the first time I got mixed
> up between flags and vm->flags so that's part of why I reported it.
>
> Smatch ignores the WARN_ON(). Historically WARN_ON() has been useless
> for indicating whether something can happen or not. These days,
> WARN_ON() is treated as a syzkaller bug so we prefer pr_warn() if
> something can actually happen. We still see a lot of WARN_ON()s
> happening in real life so I'm not eager to make Smatch treat them like a
> BUG_ON().
>
> Also, sadly, even if we changed the WARN_ON() to a BUG_ON() it still
> wouldn't silence the warning because Smatch is not quite clever enough
> to parse that.

Thanks for your sharing, Dan.

My understanding was that our current code alwasys have va->flags with
VMAP_RAM when it's set. So the case va->flags == VMAP_BLOCK won't
happen. I now understand your worry that it possibly happens. People
could change that in the futuer with buggy code or intented action while
not noticing that. I may not get your suggestion clearly, wonder if you
are suggesting this could be a realtime bug and need be stated in patch
log clearly.

Thanks
Baoquan

2023-10-19 16:51:02

by Andrew Morton

[permalink] [raw]

Subject: Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)

On Thu, 19 Oct 2023 10:28:21 +0800 Baoquan He <[email protected]> wrote:

> 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.

Thanks, I updated the changelog in-place.

2023-10-20 00:22:33

by Baoquan He

[permalink] [raw]

Subject: Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)

On 10/19/23 at 09:50am, Andrew Morton wrote:
> On Thu, 19 Oct 2023 10:28:21 +0800 Baoquan He <[email protected]> wrote:
>
> > 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.
>
> Thanks, I updated the changelog in-place.

Thanks, Andrew.