From: Baoquan He <bhe@redhat.com>
To: Dan Carpenter <error27@gmail.com>
Cc: oe-kbuild@lists.linux.dev, linux-mm@kvack.org, lkp@intel.com,
oe-kbuild-all@lists.linux.dev, linux-kernel@vger.kernel.org,
urezki@gmail.com, lstoakes@gmail.com,
stephen.s.brennan@oracle.com, willy@infradead.org,
akpm@linux-foundation.org, hch@infradead.org
Subject: Re: [PATCH v3 3/7] mm/vmalloc.c: allow vread() to read out vm_map_ram areas
Date: Sun, 15 Jan 2023 22:08:55 +0800 [thread overview]
Message-ID: <Y8QI977QBDbuuGW5@fedora> (raw)
In-Reply-To: <202301132345.KVjvHMFq-lkp@intel.com>
Hi Dan,
On 01/14/23 at 10:57am, Dan Carpenter wrote:
> Hi Baoquan,
>
> url: https://github.com/intel-lab-lkp/linux/commits/Baoquan-He/mm-vmalloc-c-add-used_map-into-vmap_block-to-track-space-of-vmap_block/20230113-112149
> base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> patch link: https://lore.kernel.org/r/20230113031921.64716-4-bhe%40redhat.com
> patch subject: [PATCH v3 3/7] mm/vmalloc.c: allow vread() to read out vm_map_ram areas
> config: i386-randconfig-m021
> compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>
>
> smatch warnings:
> mm/vmalloc.c:3682 vread() error: we previously assumed 'vm' could be null (see line 3664)
Thanks for checking this. I went through the code flow again, personally
think that the issue and risk pointed out could not exist. Please see
the comment at below.
>
> vim +/vm +3682 mm/vmalloc.c
>
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3630 long vread(char *buf, char *addr, unsigned long count)
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3631 {
> e81ce85f960c2e2 Joonsoo Kim 2013-04-29 3632 struct vmap_area *va;
> e81ce85f960c2e2 Joonsoo Kim 2013-04-29 3633 struct vm_struct *vm;
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3634 char *vaddr, *buf_start = buf;
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3635 unsigned long buflen = count;
> 129dbdf298d7383 Baoquan He 2023-01-13 3636 unsigned long n, size, flags;
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3637
> 4aff1dc4fb3a5a3 Andrey Konovalov 2022-03-24 3638 addr = kasan_reset_tag(addr);
> 4aff1dc4fb3a5a3 Andrey Konovalov 2022-03-24 3639
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3640 /* Don't allow overflow */
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3641 if ((unsigned long) addr + count < count)
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3642 count = -(unsigned long) addr;
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3643
> e81ce85f960c2e2 Joonsoo Kim 2013-04-29 3644 spin_lock(&vmap_area_lock);
> f181234a5a21fd0 Chen Wandun 2021-09-02 3645 va = find_vmap_area_exceed_addr((unsigned long)addr);
> f608788cd2d6cae Serapheim Dimitropoulos 2021-04-29 3646 if (!va)
> f608788cd2d6cae Serapheim Dimitropoulos 2021-04-29 3647 goto finished;
> f181234a5a21fd0 Chen Wandun 2021-09-02 3648
> f181234a5a21fd0 Chen Wandun 2021-09-02 3649 /* no intersects with alive vmap_area */
> f181234a5a21fd0 Chen Wandun 2021-09-02 3650 if ((unsigned long)addr + count <= va->va_start)
> f181234a5a21fd0 Chen Wandun 2021-09-02 3651 goto finished;
> f181234a5a21fd0 Chen Wandun 2021-09-02 3652
> f608788cd2d6cae Serapheim Dimitropoulos 2021-04-29 3653 list_for_each_entry_from(va, &vmap_area_list, list) {
> e81ce85f960c2e2 Joonsoo Kim 2013-04-29 3654 if (!count)
> e81ce85f960c2e2 Joonsoo Kim 2013-04-29 3655 break;
> e81ce85f960c2e2 Joonsoo Kim 2013-04-29 3656
> 129dbdf298d7383 Baoquan He 2023-01-13 3657 vm = va->vm;
> 129dbdf298d7383 Baoquan He 2023-01-13 3658 flags = va->flags & VMAP_FLAGS_MASK;
> 129dbdf298d7383 Baoquan He 2023-01-13 3659
> 129dbdf298d7383 Baoquan He 2023-01-13 3660 if (!vm && !flags)
> ^^^
> vm can be NULL if a flag in VMAP_FLAGS_MASK is set.
>
> e81ce85f960c2e2 Joonsoo Kim 2013-04-29 3661 continue;
Right, after the 'continue;' line, only two cases could happen when it
comes here. (vm != null) or (vm->flags & VMAP_RAM) is true.
> e81ce85f960c2e2 Joonsoo Kim 2013-04-29 3662
> 129dbdf298d7383 Baoquan He 2023-01-13 3663 vaddr = (char *) va->va_start;
> 129dbdf298d7383 Baoquan He 2023-01-13 @3664 size = vm ? get_vm_area_size(vm) : va_size(va);
> ^^
>
> 129dbdf298d7383 Baoquan He 2023-01-13 3665
> 129dbdf298d7383 Baoquan He 2023-01-13 3666 if (addr >= vaddr + size)
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3667 continue;
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3668 while (addr < vaddr) {
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3669 if (count == 0)
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3670 goto finished;
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3671 *buf = '\0';
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3672 buf++;
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3673 addr++;
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3674 count--;
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3675 }
> 129dbdf298d7383 Baoquan He 2023-01-13 3676 n = vaddr + size - addr;
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3677 if (n > count)
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3678 n = count;
> 129dbdf298d7383 Baoquan He 2023-01-13 3679
> 129dbdf298d7383 Baoquan He 2023-01-13 3680 if (flags & VMAP_RAM)
>
> assume VMAP_RAM is not set
OK, then vm is not null.
>
> 129dbdf298d7383 Baoquan He 2023-01-13 3681 vmap_ram_vread(buf, addr, n, flags);
> 129dbdf298d7383 Baoquan He 2023-01-13 @3682 else if (!(vm->flags & VM_IOREMAP))
> ^^^^^^^^^
> Unchecked dereference. Should this be "flags" instead of "vm->flags"?
Thus, here, in 'else if', vm is not null. And in this 'else if', we are
intending to check vm->flags. I don't see issue or risk here. Please
help check if I miss anything.
Thanks
Baoquan
>
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3683 aligned_vread(buf, addr, n);
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3684 else /* IOREMAP area is treated as memory hole */
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3685 memset(buf, 0, n);
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3686 buf += n;
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3687 addr += n;
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3688 count -= n;
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3689 }
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3690 finished:
> e81ce85f960c2e2 Joonsoo Kim 2013-04-29 3691 spin_unlock(&vmap_area_lock);
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3692
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3693 if (buf == buf_start)
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3694 return 0;
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3695 /* zero-fill memory holes */
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3696 if (buf != buf_start + buflen)
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3697 memset(buf, 0, buflen - (buf - buf_start));
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3698
> d0107eb07320b5d KAMEZAWA Hiroyuki 2009-09-21 3699 return buflen;
> ^1da177e4c3f415 Linus Torvalds 2005-04-16 3700 }
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests
>
next prev parent reply other threads:[~2023-01-15 14:09 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-13 3:19 [PATCH v3 0/7] " Baoquan He
2023-01-13 3:19 ` [PATCH v3 1/7] mm/vmalloc.c: add used_map into vmap_block to track space of vmap_block Baoquan He
2023-01-16 11:39 ` Uladzislau Rezki
2023-01-16 12:22 ` Lorenzo Stoakes
2023-01-13 3:19 ` [PATCH v3 2/7] mm/vmalloc.c: add flags to mark vm_map_ram area Baoquan He
2023-01-16 11:42 ` Uladzislau Rezki
2023-01-16 12:23 ` Lorenzo Stoakes
2023-01-18 2:13 ` Baoquan He
2023-01-13 3:19 ` [PATCH v3 3/7] mm/vmalloc.c: allow vread() to read out vm_map_ram areas Baoquan He
2023-01-14 7:57 ` Dan Carpenter
2023-01-15 14:08 ` Baoquan He [this message]
2023-01-16 13:08 ` Dan Carpenter
2023-01-18 2:17 ` Baoquan He
2023-01-16 11:50 ` Uladzislau Rezki
2023-01-19 9:52 ` Baoquan He
2023-01-19 12:48 ` Baoquan He
2023-01-20 11:54 ` Uladzislau Rezki
2023-01-16 19:01 ` Matthew Wilcox
2023-01-16 19:48 ` Lorenzo Stoakes
2023-01-13 3:19 ` [PATCH v3 4/7] mm/vmalloc: explicitly identify vm_map_ram area when shown in /proc/vmcoreinfo Baoquan He
2023-01-16 11:43 ` Uladzislau Rezki
2023-01-16 12:24 ` Lorenzo Stoakes
2023-01-13 3:19 ` [PATCH v3 5/7] mm/vmalloc: skip the uninitilized vmalloc areas Baoquan He
2023-01-16 11:44 ` Uladzislau Rezki
2023-01-16 12:24 ` Lorenzo Stoakes
2023-01-13 3:19 ` [PATCH v3 6/7] powerpc: mm: add VM_IOREMAP flag to the vmalloc area Baoquan He
2023-01-16 11:46 ` Uladzislau Rezki
2023-01-16 12:25 ` Lorenzo Stoakes
2023-01-13 3:19 ` [PATCH v3 7/7] sh: mm: set " Baoquan He
2023-01-16 11:46 ` Uladzislau Rezki
2023-01-16 12:25 ` Lorenzo Stoakes
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y8QI977QBDbuuGW5@fedora \
--to=bhe@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=error27@gmail.com \
--cc=hch@infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=lkp@intel.com \
--cc=lstoakes@gmail.com \
--cc=oe-kbuild-all@lists.linux.dev \
--cc=oe-kbuild@lists.linux.dev \
--cc=stephen.s.brennan@oracle.com \
--cc=urezki@gmail.com \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox