Re: [PATCH] lockd: Show pid of lockd for remote locks

["Benjamin Coddington" <bcodding@redhat.com>]

On 20 May 2019, at 9:12, Benjamin Coddington wrote:

> On 18 May 2019, at 22:15, Xuewei Zhang wrote:
>
>> On Sat, May 18, 2019 at 5:09 AM Benjamin Coddington 
>> <bcodding@redhat.com> wrote:
>>>
>>> On 17 May 2019, at 17:45, Xuewei Zhang wrote:
>>>> Seems this patch introduced a bug in how lock protocol handles
>>>> GRANTED_MSG in nfs.
>>>
>>> Yes, you're right: it's broken, and broken badly because we find 
>>> conflicting
>>> locks based on lockd's fl_pid and lockd's fl_owner, which is 
>>> current->files.
>>> That means that clients are not differentiated, and that means that 
>>> v3 locks
>>> are broken.
>>
>> Thanks a lot for the quick response and confirming the problem!
>>
>>>
>>> I'd really like to see the fl_pid value make sense on the server 
>>> when we
>>> show it to userspace, so I think that we should stuff the svid in 
>>> fl_owner.
>>>
>>> Clearly I need to be more careful making changes here, so I am going 
>>> to take
>>> my time fixing this, and I won't get to it until Monday. A revert 
>>> would get
>>> us back to safe behavior.
>>
>> From my limited understanding, b8eee0e90f97 ("lockd: Show pid of 
>> lockd
>> for remote locks")
>> exists only for fixing lockd in 9d5b86ac13c5 ("fs/locks: Remove
>> fl_nspid and use fs-specific...").
>>
>> But I don't see anything wrong in 9d5b86ac13c5 ("fs/locks: Remove
>> fl_nspid and use fs-specific..."). Could you let me know what's the
>> problem? Thanks a lot!
>>
>> If 9d5b86ac13c5 ("fs/locks: Remove fl_nspid and use fs-specific...")
>> is correct, we
>> probably don't need to add another fixing patch. Perhaps reverting 
>> b8eee0e90f97
>> ("lockd: Show pid of lockd for remote locks") would be the best way 
>> then.
>
> I think we have an existing problem:  the NLM server is setting 
> fl_owner to
> current->files and (before the bad patch) fl_pid to svid.
>
> That means that we can't tell the difference between locks from 
> different
> clients that may have the same svid.  The bad patch just made the 
> problem
> far more likely to occur, that's what you're now noticing.

Ok, I just noticed that we set fl_owner to the nlm_host in
nlm4svc_retrieve_args, so things are not as dire as I thought.  What 
would
be nice is a sane set of tests for NLM..

Since we already were placing the nlm_host in fl_owner, I think 
reverting
9d5b86ac13c5 at this point is the proper thing to do.

Ben