> > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
