Re: [PATCH] locks: try to catch potential deadlock between file-private and classic locks from same process

["J. Bruce Fields" <bfields@fieldses.org>]

On Tue, Mar 04, 2014 at 03:37:23PM -0500, Jeff Layton wrote:
> On Tue, 4 Mar 2014 12:19:44 -0800
> Andy Lutomirski <luto@amacapital.net> wrote:
> 
> > On Tue, Mar 4, 2014 at 12:14 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > > On Tue, 4 Mar 2014 14:35:51 -0500
> > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > >
> > >> On Tue, Mar 04, 2014 at 02:10:49PM -0500, Jeff Layton wrote:
> > >> > My expectation is that programs shouldn't mix classic and file-private
> > >> > locks, but Glenn Skinner pointed out to me that that may occur at times
> > >> > even if the programmer isn't aware.
> > >> >
> > >> > Suppose we have a program that uses file-private locks. That program
> > >> > then links in a library that uses classic POSIX locks. If those locks
> > >> > end up conflicting and one is using blocking locks, then the program
> > >> > could end up deadlocked.
> > >> >
> > >> > Try to catch this situation in posix_locks_deadlock by looking for the
> > >> > case where the blocking lock was set by the same process but has a
> > >> > different type, and have the kernel return EDEADLK if that occurs.
> > >> >
> > >> > This check is not perfect. You could (in principle) have a threaded
> > >> > process that is using classic locks in one thread and file-private locks
> > >> > in another. That's not necessarily a deadlockable situation but this
> > >> > check would cause an EDEADLK return in that case.
> > >> >
> > >> > By the same token, you could also have a file-private lock that was
> > >> > inherited across a fork(). If the inheriting process ends up blocking on
> > >> > that while trying to set a classic POSIX lock then this check would miss
> > >> > it and the program would deadlock.
> > >>
> > >> If the caller's not prepared for the library to use classic posix locks,
> > >> then it's not going to know how to recover from this EDEADLCK either, is
> > >> it?
> > >>
> > >
> > > Well, callers should be aware of that if we take this change. The
> > > semantics aren't yet set in stone...
> > >
> > >> I guess I don't understand how this helps anyone.
> > >>
> > >> Has it ever made sense for a library function and its caller to both use
> > >> classic posix locking on the same file without any coordination?
> > >>
> > >
> > > Not really, but that doesn't mean that it isn't done... ;)
> > >
> > >> Besides the first-close problem there's the problem that locks merge, so
> > >> for example you can't hold your own lock across a call to a function
> > >> that grabs and drops a lock on the same file.
> > >>
> > >
> > > It depends, but you're basically correct...
> > >
> > > It's likely that if the above situation occurred with a program using
> > > classic locks, then those locks were silently lost at times. It's also
> > > plausible that when it occurs that no one is aware of it due to the way
> > > POSIX locks work.
> > >
> > > If the program switched to using file-private locks and the library
> > > stays using classic locks (or vice versa), you then potentially trade
> > > that silent loss of locks for a deadlock (since classic and
> > > file-private locks always conflict).
> > >
> > > So, the idea would be to try to catch that situation explicitly and
> > > return a hard error instead of deadlocking. Unfortunately, it's a
> > > little tough to do that in all cases so all this does is try to catch a
> > > subset of them.
> > >
> > > Will it be helpful in the long run? I'm not sure. It seems unlikely to
> > > harm legit use cases though, and might catch some problematic
> > > situations. I can drop this if that's the consensus however.
> > 
> > I don't think I like it except in the case where there are no threads
> > (number of tasks sharing the fd table is 1) and where the struct file
> > only has one fd.  Otherwise I think it can have false positives.  Or
> > am I missing something?
> > 
> 
> The only case where I think this would hit a false positive is if you
> have a threaded program that's doing something weird like having one
> thread that's setting classic POSIX locks on a file, and one thread
> that isn't. Once you hit a conflict between the two, you'd get back
> EDEADLK on one of them, even though that situation might not actually
> be a deadlock.
> 
> That doesn't really seem like a real-world use-case though, so I'm
> generally OK with that potential false-positive.

Yes, you may be correct that those are almost certainly abuses of the
interface, but I think Andy's point is that EDEADLK doesn't mean "you're
doing something wrong", it has a stricter definition, and you're
catching cases that are "false positives" in the sense that they don't
necessarily identify actual deadlocks.

--b.