lkml.org 
[lkml]   [2012]   [Mar]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file v3
On Fri, Mar 09, 2012 at 03:26:20PM +0100, Oleg Nesterov wrote:
> >
> > To be sure it's not increased somewhere else before
> > down_write taken.
>
> Who can do this? Only another CLONE_VM thread. And _only_ if we
> already have the bug in mm_exe accounting logic. And only if that
> thread does something to trigger the bug in the small window
> between.

ok, agreed.

> > > into removed_exe_file_vma().
> >
> > This one looks like a good idea for me -- it's cheap and
> > not a hot path.
>
> But not in this patch, please.
>

Sure.

> > > But imho your WARN looks like "OK, I checked it lockless but I
> > > am not sure this is correct".
> >
> > Oleg, I bet if someone will be changing num_exe_file_vmas overall
> > idea -- this prctl code will be fixed at last moment (if ever) only
> > because it's very specific, so I wanted to not miss such moment
> > and add some check that the rest of the kernel is in a good state.
> > This test is cheap but may prevent potential problem if one day
> > mm::exe_file concept will be reworked.
>
> The test is cheap indeed. If you mean performance-wise.
>
> But it looks confusing, imho. I do not care about a couple of CPU
> cycles. The code should be optimized for the reading in the first
> place, not for executing ;) Imho, of course.
>
> And once again. Following your logic you need another WARN_ON()
> right after we drop mmap_sem. Why? To be sure it's not increased
> somewhere else _after_ down_write taken. And another one after
> fput.
>
> Sure, bugs are possible. And yes, in theory this WARN_ON() can
> catch some problem. But there is tradeoff. Given that you need
> another thread to trigger the (potential) bug and the window is
> tiny, how high do you estimate the probability it can help?
>
> > Sure I can simply drop this WARN_ON ;)
>
> Oh, keep it if you like it ;)
>
> Yes I hate it, but you are the author and this is almost cosmetic.

OK, Oleg, can't argue, you've convinced me ;) I'll drop this WARN_ON.
Would it be enough for your Reviewed-by tag? /me hides

Cyrill


\
 
 \ /
  Last update: 2012-03-09 15:45    [from the cache]
©2003-2011 Jasper Spaans