Re: Huge race in lockd for async lock requests?

J. Bruce Fields
2009-05-29T00:26:35Z

On Thu, May 28, 2009 at 03:34:19PM -0600, Rob Gardner wrote:
> J. Bruce Fields wrote:
>>>>>> At 04:43 PM 5/19/2009, Rob Gardner wrote:
>>>>>>
>>>>>>> I've got a question about lockd in conjunction with a
>>>>>>> filesystem that provides its own (async) locking.
>>>>>>>
>>>>>>> After nlmsvc_lock() calls vfs_lock_file(), it seems to be
>>>>>>> that we might get the async callback (nlmsvc_grant_deferred)
>>>>>>> at any time. What's to stop it from arriving before we even
>>>>>>> put the block on the nlm_block list? If this happens, then
>>>>>>> nlmsvc_grant_deferred() will print "grant for unknown block"
>>>>>>> and then we'll wait forever for a grant that will never come.
>>>>>>>
>>> dealing with a filesystem that provides its own locking functions via
>>> file->f_op->lock(). Such a filesystem might easily defer a
>>> non-blocking lock request and invoke the callback later. At least I
>>> don't know of any rule that says that it can't do this, and clearly
>>> the code expects this possibility:
>>>
>>> case FILE_LOCK_DEFERRED:
>>> if (wait)
>>> break;
>>> /* Filesystem lock operation is in progress
>>> Add it to the queue waiting for callback */
>>> ret = nlmsvc_defer_lock_rqst(rqstp, block);
>>>
>>> It looks to me like a bug in the server. The server must be able
>>> to deal with async filesystem callbacks happening at any time,
>>> however inconvenient.
>>>
>>
>> Absolutely, if that's possible then it's a server bug.
>>
>> --b.
>>
>
> It's definitely possible for the async filesystem callback to occur at
> any time.

Looking at the code.... This is all under the BKL, and as far as I can
tell there aren't any blocking operations anywhere there, so I don't
think this should happen if the filesystem is careful. Have you seen it
happen?

Of course this may be fragile--we'll have to think about what to do when
we eventually remove the BKL.

--b.

> I think at the very least, nlmsvc_lock() ought to put the
> block on the nlm_block list *before* calling vfs_lock_file(), and then
> remove it immediately if the lock is granted synchronously. I would like
> to develop and submit a patch for this, but I am currently working with
> a much older kernel and it will take some time before I get to work with
> newer bits.