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