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@xxxxxxxxxx> -- 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
