Re: [patch 141/192] fs/proc/kcore.c: add mmap interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 30, 2021 at 6:54 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> When we do the kernel monitor, use the DRGN
> (https://github.com/osandov/drgn) access to kernel data structures, found
> that the system calls a lot.  DRGN is implemented by reading /proc/kcore.
> After looking at the kcore code, it is found that kcore does not implement
> mmap, resulting in frequent context switching triggered by read.
> Therefore, we want to add mmap interface to optimize performance.

Ok, this is funky, but I'm going to drop this patch because I think
it's buggy as is.

 Since

> +static int mmap_kcore(struct file *file, struct vm_area_struct *vma)
> +{
> +       size_t size = vma->vm_end - vma->vm_start;

Ok.

But then:

> +       start = kc_offset_to_vaddr(((u64)vma->vm_pgoff << PAGE_SHIFT) -
> +               ((data_offset >> PAGE_SHIFT) << PAGE_SHIFT));

Not only is that

        ((data_offset >> PAGE_SHIFT) << PAGE_SHIFT)

a very strange calculation (did you mean "data_offset & PAGE_MASK"?),
but I don't see anything that protects against underflow in that
calculation. pg_off can easily be arbitrarily small (eg zero), so that
subtraction can underflow afaik.

So that needs a test, and return -EINVAL or whatever.

But even if that is fixed, this test is entirely broken:

> +       list_for_each_entry(m, &kclist_head, list) {
> +               if (start >= m->addr && size <= m->size)
> +                       break;
> +       }

No, that's wrong.

You allow 'size' to be as big as 'm->size', but you do that even if
'start' isn't 'm->start'.

The proper check would be something like

       u64 end = start + size;

        if (start >= m->addr && end <= m->addr+m->size) ..

or similar (and that should check that "start+size" hasn't overflowed).

So I see what appears to be multiple problems, and while I hand-waved
some fixes for them, those are very much "maybe something like this",
and I'm going to drop this patch. Not for 5.14.

           Linus



[Index of Archives]     [Kernel Archive]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]

  Powered by Linux