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