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

[Al Viro]

On Thu, Apr 19, 2012 at 07:31:01PM -0700, Linus Torvalds wrote:
> On Thu, Apr 19, 2012 at 5:43 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > However, there's an approach that might be feasible. ?Most of the time
> > the final fput() *is* done without any locks held and there's a very
> > large subclass of those call sites - those that come via fput_light().
> > What we could do, and what might be maintainable is:
> > ? ? ? ?* prohibit fput_light() with locks held. ?Right now we are very
> > close to that (or already there - I haven't finished checking).
> > ? ? ? ?* convert low-hanging fget/fput in syscalls to fget_light/fput_light.
> > Makes sense anyway.
> 
> Many of them would make sense, yes (looking at vfs_fstatat() etc.
> 
> But a lot of fput() calls come from close() -> filp_close -> fput().
> 
> And the "fput_light()" model *only* works together with fget_light()
> as it is now.
> 
> So I do think you need some other model. Of course, we can just do
> "fput_light(file, 1)" instead - that seems pretty ugly, though. But
> just making "fput()" do a defer on the last count sounds actively
> *wrong* for things like close(), which may actually have serious
> consistency guarantees (ie the process doing the close() may "know"
> that it is the last user, and depend on the fact that the close() did
> actually delete the inode etc.

Umm...  I really wonder if we *want* filp_close() under any kind of
locks.  You are right - it should not be deferred.  I haven't finished
checking the callers of that puppy, but if we really do it while holding
any kind of lock, we are asking for trouble.  So I'd rather switch
filp_close() to use of fput_nodefer() if that turns out to be possible.

FWIW, the set of primitives I'm thinking of right now is

__fput(file) - same as now
schedule_fput(file) - takes the only reference to file and schedules __fput()
fput_nodefer(file)
{
        if (atomic_long_dec_and_test(&file->f_count))
                __fput(file);
}
fput(file)
{
	if (unlikely(!fput_atomic(file))
		schedule_fput(file);
}
fput_light(file, need_fput)
{
	if (need_fput)
		fput_nodefer(file);
}
fput_light_defer(file, need_fput) // for callers in some weird ioctls, might
				  // not be needed at all
{
	if (need_fput)
		fput(file);
}

and filp_close() would, if that turns out to be possible, call fput_nodefer()
instead of fput().  If we *do* have places where we need deferral in
filp_close() (and I'm fairly sure that any such place is a deadlock right
now), well, we'll need a variant of filp_close() sans the call of fput...()
and those places would call that, followed by full (deferring) fput().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/