Re: Huge race in lockd for async lock requests?

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

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.