Re: [RFC] F_SETLEASE mess

[Bruce Fields <bfields@fieldses.org>]

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

nfsd isn't using write leases yet (I want to get read delegations sorted
out first), and I don't understand Samba's requirements for write
leases.

In the future, when nfsd does write delegations: they're an optional
optimization, and if we're concerned about such a DOS one solution might
be just to change everything to a trylock when acquiring delegations:
it's always acceptable to just fail the delegation.

On Fri, Jul 05, 2013 at 08:08:44AM -0400, Jeff Layton wrote:
> On Fri, 2013-07-05 at 06:51 -0400, Jeff Layton wrote:
> > 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?

Well, as Al says, the race is roughly:

	take i_lock
	generic_add_lease checks d_count, i_count

		Conflicting opener does "*everything* (pathname lookup,
		etc.)" (including that path_get, and breake_lease()
		(which sees no lock, so doesn't try to get i_lock.))

	...
	locks_insert_lock() adds new lock.

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

I think the only reason for this race is the attempt to optimize out an
i_lock acquisition in break_lease.  If we're willing to call igrab
(which also takes the i_lock), then we may as well just take the i_lock.

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

... but checking the counts again afterwards might work.  (Dumb
question: in the absence of a lock on the opener's side, are the memory
accesses ordered such that a lease-setter is guaranteed to see the new
counts from an opener that didn't see the new i_flock value?)

How important is the optimization that skips the i_lock in break_lease?

--b.