Re: [RFC] F_SETLEASE mess

[Jeff Layton <jlayton@redhat.com>]

On Fri, 2013-07-05 at 06:51 -0400, Jeff Layton wrote:
> On Fri, 2013-07-05 at 10:04 +0100, Al Viro wrote:
> > 	generic_add_lease() with F_WRLCK checks for other openers
> > in a very crude way - it wants no extra references to dentry (thus
> > excluding other struct file pointing to it) *and* no extra references
> > to in-core inode, excluding openers of other links.  It fails with
> > EAGAIN if those conditions are not met.
> > 
> > 	The way it deals with another open(2) racing with it (i.e.
> > managing to squeeze between the check and locks_insert_lock()) is
> > theoretically racy; do_dentry_open() would spin on ->i_lock, all
> > right, but... only if there already is something in inode->i_flock.
> > If this is the first lease/lock being set, break_lease() will do
> > nothing, rather than call __break_lease() and spin there.
> > 
> > 	It's _very_ hard to hit; we are holding ->i_lock and thus can't
> > be preempted, so open(2) would have to get *everything* (pathname
> > lookup, etc.) done in a very narrow window.  So I don't believe it's
> > exploitable, but it really smells bad.  The check is extremely crude
> > and if nothing else it's a DoS fodder - a luser that keeps hitting that
> > file with stat(2) can prevent F_SETLEASE from succeeding, even though
> > he wouldn't be able to open the damn thing at all...
> 
> (cc'ing Bruce since he's been poking around in this area recently and
> might have ideas...)
> 
> I see what you mean. I haven't looked closely at this yet, and I'm on
> holiday today, but I'll plan to give this more scrutiny when I get back
> next week. Giving it an initial look though...
> 
> We already hold an extra reference to the dentry via the path_get in
> do_dentry_open. So is this race possible if the two tasks are working on
> the same dentry? Or does it require a hardlinked inode?
> 
> If it's not possible to race on the same dentry, then one possible fix
> would be to go ahead and do an extra igrab(inode) in do_dentry_open
> before calling break_lease. I'm not particularly fond of that since it
> means taking the i_lock an extra time, but it looks like it would close
> the race.
> 


Hrm. I think we'd also need to couple that with an extra check for a
high refcount after doing locks_insert_lock in generic_add_lease, and
then call locks_delete_lock and return -EAGAIN if the counts have
changed.

-- 
Jeff Layton <jlayton@redhat.com>