lkml.org 
[lkml]   [2012]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC] situation with fput() locking (was Re: [PULL REQUEST] : ima-appraisal patches)
    On Fri, Apr 20, 2012 at 02:12:55PM -0700, Linus Torvalds wrote:

    > Are they all like that? No. But most of the ones outside of mm/ do fit
    > that simple pattern and should probably be fixed up just to have them
    > not contain VM locking details in them *anyway*.

    Kinda-sorta. I agree that such helpers make sense, but you are too
    optimistic about the number of such places. And clusterfuck around
    mremap() is fairly deep, so propagating all way back to up_write()
    wont' be fun.

    sys_shmdt() is misusing do_munmap(), AFAICS. And there we call it many
    times in a loop, unmapping only a subset, so it's not like we could
    blindly pick a string of vmas and handle them all separately afterwards.
    It could be handled if we passed an initially empty list, collecting
    vmas in it as we go, but... ouch

    BTW, looks like I've just found a bug in oprofile - take a look at
    sys_munmap() and you'll see that it's almost exactly your proposed
    helper. The only difference is that it starts with calling
    profile_munmap(addr);
    (mind you, *before* grabbing ->mmap_sem), which is to say
    blocking_notifier_call_chain(&munmap_notifier, 0, (void *)addr);
    i.e. a really fancy way to arrange a call of munmap_notify() from
    drivers/oprofile/buffer_sync.c. Which does the following:
    {
    unsigned long addr = (unsigned long)data;
    struct mm_struct *mm = current->mm;
    struct vm_area_struct *mpnt;

    down_read(&mm->mmap_sem);

    mpnt = find_vma(mm, addr);
    if (mpnt && mpnt->vm_file && (mpnt->vm_flags & VM_EXEC)) {
    up_read(&mm->mmap_sem);
    /* To avoid latency problems, we only process the current CPU,
    * hoping that most samples for the task are on this CPU
    */
    sync_buffer(raw_smp_processor_id());
    return 0;
    }

    up_read(&mm->mmap_sem);
    return 0;
    }

    Leaving aside the convoluted way it's done, it's obviously both racy and
    incomplete. The worst part is, sync_buffer() *can't* be called with
    any ->mmap_sem held; it goes through a bunch of task_struct, grabbing
    ->mmap_sem shared. Fun...


    \
     
     \ /
      Last update: 2012-04-21 00:15    [W:0.035 / U:148.056 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site