* mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
@ 2023-10-17 14:26 Dan Carpenter
2023-10-18 8:54 ` Baoquan He
0 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2023-10-17 14:26 UTC (permalink / raw)
To: oe-kbuild, Lorenzo Stoakes
Cc: lkp, oe-kbuild-all, linux-kernel, Andrew Morton,
Linux Memory Management List, Baoquan He
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/202310171600.WCrsOwFj-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231017/202310171600.WCrsOwFj-lkp@intel.com/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 <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202310171600.WCrsOwFj-lkp@intel.com/
smatch warnings:
mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
2023-10-17 14:26 mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667) Dan Carpenter
@ 2023-10-18 8:54 ` Baoquan He
2023-10-18 10:32 ` Dan Carpenter
0 siblings, 1 reply; 12+ messages in thread
From: Baoquan He @ 2023-10-18 8:54 UTC (permalink / raw)
To: Dan Carpenter
Cc: oe-kbuild, Lorenzo Stoakes, lkp, oe-kbuild-all, linux-kernel,
Andrew Morton, Linux Memory Management List
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/202310171600.WCrsOwFj-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20231017/202310171600.WCrsOwFj-lkp@intel.com/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 <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202310171600.WCrsOwFj-lkp@intel.com/
>
> 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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
2023-10-18 8:54 ` Baoquan He
@ 2023-10-18 10:32 ` Dan Carpenter
2023-10-18 12:12 ` Baoquan He
0 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2023-10-18 10:32 UTC (permalink / raw)
To: Baoquan He
Cc: oe-kbuild, Lorenzo Stoakes, lkp, oe-kbuild-all, linux-kernel,
Andrew Morton, Linux Memory Management List
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/202310171600.WCrsOwFj-lkp@intel.com/config)
> > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > reproduce: (https://download.01.org/0day-ci/archive/20231017/202310171600.WCrsOwFj-lkp@intel.com/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 <lkp@intel.com>
> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > | Closes: https://lore.kernel.org/r/202310171600.WCrsOwFj-lkp@intel.com/
> >
> > 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
2023-10-18 10:32 ` Dan Carpenter
@ 2023-10-18 12:12 ` Baoquan He
2023-10-18 12:45 ` Philip Li
0 siblings, 1 reply; 12+ messages in thread
From: Baoquan He @ 2023-10-18 12:12 UTC (permalink / raw)
To: Dan Carpenter
Cc: oe-kbuild, Lorenzo Stoakes, lkp, oe-kbuild-all, linux-kernel,
Andrew Morton, Linux Memory Management List
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/202310171600.WCrsOwFj-lkp@intel.com/config)
> > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > > reproduce: (https://download.01.org/0day-ci/archive/20231017/202310171600.WCrsOwFj-lkp@intel.com/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 <lkp@intel.com>
> > > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > | Closes: https://lore.kernel.org/r/202310171600.WCrsOwFj-lkp@intel.com/
> > >
> > > 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/202310171600.WCrsOwFj-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20231017/202310171600.WCrsOwFj-lkp@intel.com/reproduce)
Thanks
Baoquan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
2023-10-18 12:12 ` Baoquan He
@ 2023-10-18 12:45 ` Philip Li
2023-10-18 15:15 ` Baoquan He
0 siblings, 1 reply; 12+ messages in thread
From: Philip Li @ 2023-10-18 12:45 UTC (permalink / raw)
To: Baoquan He
Cc: Dan Carpenter, oe-kbuild, Lorenzo Stoakes, lkp, oe-kbuild-all,
linux-kernel, Andrew Morton, Linux Memory Management List
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/202310171600.WCrsOwFj-lkp@intel.com/config)
> > > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > > > reproduce: (https://download.01.org/0day-ci/archive/20231017/202310171600.WCrsOwFj-lkp@intel.com/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 <lkp@intel.com>
> > > > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > > | Closes: https://lore.kernel.org/r/202310171600.WCrsOwFj-lkp@intel.com/
> > > >
> > > > 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/202310171600.WCrsOwFj-lkp@intel.com/config)
> reproduce: (https://download.01.org/0day-ci/archive/20231017/202310171600.WCrsOwFj-lkp@intel.com/reproduce)
Meanwhile, we will resolve the reproduce empty issue as early as possible.
>
> Thanks
> Baoquan
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
2023-10-18 12:45 ` Philip Li
@ 2023-10-18 15:15 ` Baoquan He
2023-10-18 15:52 ` Andrew Morton
0 siblings, 1 reply; 12+ messages in thread
From: Baoquan He @ 2023-10-18 15:15 UTC (permalink / raw)
To: Dan Carpenter, Philip Li
Cc: oe-kbuild, Lorenzo Stoakes, lkp, oe-kbuild-all, linux-kernel,
Andrew Morton, Linux Memory Management List
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/202310171600.WCrsOwFj-lkp@intel.com/config)
> > > > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > > > > reproduce: (https://download.01.org/0day-ci/archive/20231017/202310171600.WCrsOwFj-lkp@intel.com/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 <lkp@intel.com>
> > > > > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > > > | Closes: https://lore.kernel.org/r/202310171600.WCrsOwFj-lkp@intel.com/
> > > > >
> > > > > 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 <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.
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>
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
2023-10-18 15:15 ` Baoquan He
@ 2023-10-18 15:52 ` Andrew Morton
2023-10-19 2:28 ` Baoquan He
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2023-10-18 15:52 UTC (permalink / raw)
To: Baoquan He
Cc: Dan Carpenter, Philip Li, oe-kbuild, Lorenzo Stoakes, lkp,
oe-kbuild-all, linux-kernel, Linux Memory Management List
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?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
2023-10-18 15:52 ` Andrew Morton
@ 2023-10-19 2:28 ` Baoquan He
2023-10-19 5:40 ` Dan Carpenter
2023-10-19 16:50 ` Andrew Morton
0 siblings, 2 replies; 12+ messages in thread
From: Baoquan He @ 2023-10-19 2:28 UTC (permalink / raw)
To: Andrew Morton
Cc: Dan Carpenter, Philip Li, oe-kbuild, Lorenzo Stoakes, lkp,
oe-kbuild-all, linux-kernel, Linux Memory Management List
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
2023-10-19 2:28 ` Baoquan He
@ 2023-10-19 5:40 ` Dan Carpenter
2023-10-19 12:55 ` Baoquan He
2023-10-19 16:50 ` Andrew Morton
1 sibling, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2023-10-19 5:40 UTC (permalink / raw)
To: Baoquan He
Cc: Andrew Morton, Philip Li, oe-kbuild, Lorenzo Stoakes, lkp,
oe-kbuild-all, linux-kernel, Linux Memory Management List
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 <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;
>
>
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
2023-10-19 5:40 ` Dan Carpenter
@ 2023-10-19 12:55 ` Baoquan He
0 siblings, 0 replies; 12+ messages in thread
From: Baoquan He @ 2023-10-19 12:55 UTC (permalink / raw)
To: Dan Carpenter
Cc: Andrew Morton, Philip Li, oe-kbuild, Lorenzo Stoakes, lkp,
oe-kbuild-all, linux-kernel, Linux Memory Management List
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 <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;
> >
> >
>
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
2023-10-19 2:28 ` Baoquan He
2023-10-19 5:40 ` Dan Carpenter
@ 2023-10-19 16:50 ` Andrew Morton
2023-10-20 0:21 ` Baoquan He
1 sibling, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2023-10-19 16:50 UTC (permalink / raw)
To: Baoquan He
Cc: Dan Carpenter, Philip Li, oe-kbuild, Lorenzo Stoakes, lkp,
oe-kbuild-all, linux-kernel, Linux Memory Management List
On Thu, 19 Oct 2023 10:28:21 +0800 Baoquan He <bhe@redhat.com> 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667)
2023-10-19 16:50 ` Andrew Morton
@ 2023-10-20 0:21 ` Baoquan He
0 siblings, 0 replies; 12+ messages in thread
From: Baoquan He @ 2023-10-20 0:21 UTC (permalink / raw)
To: Andrew Morton
Cc: Dan Carpenter, Philip Li, oe-kbuild, Lorenzo Stoakes, lkp,
oe-kbuild-all, linux-kernel, Linux Memory Management List
On 10/19/23 at 09:50am, Andrew Morton wrote:
> On Thu, 19 Oct 2023 10:28:21 +0800 Baoquan He <bhe@redhat.com> 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-10-20 0:22 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-17 14:26 mm/vmalloc.c:3689 vread_iter() error: we previously assumed 'vm' could be null (see line 3667) Dan Carpenter
2023-10-18 8:54 ` Baoquan He
2023-10-18 10:32 ` Dan Carpenter
2023-10-18 12:12 ` Baoquan He
2023-10-18 12:45 ` Philip Li
2023-10-18 15:15 ` Baoquan He
2023-10-18 15:52 ` Andrew Morton
2023-10-19 2:28 ` Baoquan He
2023-10-19 5:40 ` Dan Carpenter
2023-10-19 12:55 ` Baoquan He
2023-10-19 16:50 ` Andrew Morton
2023-10-20 0:21 ` Baoquan He
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).