Re: [RFC] situation with fput() locking (was Re: [PULL REQUEST] : ima-appraisal patches)

[Al Viro <viro@ZenIV.linux.org.uk>]

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...