On Mon, 16 Feb 2015 11:24:03 -0800 Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > On Mon, Feb 16, 2015 at 10:46 AM, Linus Torvalds > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > This code is so broken that my initial reaction is "We need to just > > revert the crap". > > How the hell is flock_lock_file() supposed to work at all, btw? > > Say we have an existing flock, and now do a new one that conflicts. I > see what looks like three separate bugs. > > - We go through the first loop, find a lock of another type, and > delete it in preparation for replacing it > > - we *drop* the lock context spinlock. > > - BUG #1? So now there is no lock at all, and somebody can come in > and see that unlocked state. Is that really valid? > > - another thread comes in while the first thread dropped the lock > context lock, and wants to add its own lock. It doesn't see the > deleted or pending locks, so it just adds it > > - the first thread gets the context spinlock again, and adds the lock > that replaced the original > > - BUG #2? So now there are *two* locks on the thing, and the next > time you do an unlock (or when you close the file), it will only > remove/replace the first one. > > Both of those bugs are due to the whole "drop the lock in the middle", > which is pretty much always a mistake. BUG#2 could easily explain the > warning Kirill reports, afaik. > > BUG#3 seems to be independent, and is about somebody replacing an > existing lock, but the new lock conflicts. Again, the first loop will > remove the old lock, and then the second loop will see the conflict, > and return an error (and we may then end up waiting for it for the > FILE_LOCK_DEFERRED case). Now the original lock is gone. Is that > really right? That sounds bogus. *Failing* to insert a flock causing > the old flock to go away? > > Now, flock semantics are pretty much insane, so maybe all these bugs > except for #2 aren't actually bugs, and are "features" of flock. But > bug #2 can't be a semantic feature. > > Is there something I'm missing here? > > This was all just looking at a *single* function. Quite frankly, I > hate how the code also just does > > if (filp->f_op->flock) > filp->f_op->flock(filp, F_SETLKW, &fl); > else > flock_lock_file(filp, &fl); > > and blithely assumes that some random filesystem will get the flock > semantics right, when even the code code screwed it up this badly. > > And maybe I'm wrong, and there's some reason why none of the things > above can actually happen, but it looks really bad to me. > > Linus Now that I look, it may be best to just revert this whole set for now. Linus, are you amenable to doing that? While I think this could be a nice cleanup, it obviously needs more testing and scrutiny, and it won't hurt to wait another release or two to make sure I get this right. There may be some merge conflicts with Bruce's tree, but I'd rather deal with those than break the file locking code. Once we do that, then we could introduce some smaller patches to fix up the bug you spotted, but at least we'll be proceeding from a spot that is known to work. I'll start preparing a pull request that does that... Thanks, -- Jeff Layton <jlayton@xxxxxxxxxxxxxxx> -- 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
