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: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-13 3:19 [PATCH v3 0/7] mm/vmalloc.c: allow vread() to read out vm_map_ram areas 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-13 15:51 ` kernel test robot
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.