> On Jan 22, 2024, at 6:14 PM, Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote:
>
>
>
>> On Jan 22, 2024, at 4:57 PM, NeilBrown <neilb@xxxxxxx> wrote:
>>
>> On Tue, 23 Jan 2024, Chuck Lever wrote:
>>> On Mon, Jan 22, 2024 at 02:58:16PM +1100, NeilBrown wrote:
>>>>
>>>> The test on so_count in nfsd4_release_lockowner() is nonsense and
>>>> harmful. Revert to using check_for_locks(), changing that to not sleep.
>>>>
>>>> First: harmful.
>>>> As is documented in the kdoc comment for nfsd4_release_lockowner(), the
>>>> test on so_count can transiently return a false positive resulting in a
>>>> return of NFS4ERR_LOCKS_HELD when in fact no locks are held. This is
>>>> clearly a protocol violation and with the Linux NFS client it can cause
>>>> incorrect behaviour.
>>>>
>>>> If NFS4_RELEASE_LOCKOWNER is sent while some other thread is still
>>>> processing a LOCK request which failed because, at the time that request
>>>> was received, the given owner held a conflicting lock, then the nfsd
>>>> thread processing that LOCK request can hold a reference (conflock) to
>>>> the lock owner that causes nfsd4_release_lockowner() to return an
>>>> incorrect error.
>>>>
>>>> The Linux NFS client ignores that NFS4ERR_LOCKS_HELD error because it
>>>> never sends NFS4_RELEASE_LOCKOWNER without first releasing any locks, so
>>>> it knows that the error is impossible. It assumes the lock owner was in
>>>> fact released so it feels free to use the same lock owner identifier in
>>>> some later locking request.
>>>>
>>>> When it does reuse a lock owner identifier for which a previous RELEASE
>>>> failed, it will naturally use a lock_seqid of zero. However the server,
>>>> which didn't release the lock owner, will expect a larger lock_seqid and
>>>> so will respond with NFS4ERR_BAD_SEQID.
>>>>
>>>> So clearly it is harmful to allow a false positive, which testing
>>>> so_count allows.
>>>>
>>>> The test is nonsense because ... well... it doesn't mean anything.
>>>>
>>>> so_count is the sum of three different counts.
>>>> 1/ the set of states listed on so_stateids
>>>> 2/ the set of active vfs locks owned by any of those states
>>>> 3/ various transient counts such as for conflicting locks.
>>>>
>>>> When it is tested against '2' it is clear that one of these is the
>>>> transient reference obtained by find_lockowner_str_locked(). It is not
>>>> clear what the other one is expected to be.
>>>>
>>>> In practice, the count is often 2 because there is precisely one state
>>>> on so_stateids. If there were more, this would fail.
>>>>
>>>> It my testing I see two circumstances when RELEASE_LOCKOWNER is called.
>>>> In one case, CLOSE is called before RELEASE_LOCKOWNER. That results in
>>>> all the lock states being removed, and so the lockowner being discarded
>>>> (it is removed when there are no more references which usually happens
>>>> when the lock state is discarded). When nfsd4_release_lockowner() finds
>>>> that the lock owner doesn't exist, it returns success.
>>>>
>>>> The other case shows an so_count of '2' and precisely one state listed
>>>> in so_stateid. It appears that the Linux client uses a separate lock
>>>> owner for each file resulting in one lock state per lock owner, so this
>>>> test on '2' is safe. For another client it might not be safe.
>>>>
>>>> So this patch changes check_for_locks() to use the (newish)
>>>> find_any_file_locked() so that it doesn't take a reference on the
>>>> nfs4_file and so never calls nfsd_file_put(), and so never sleeps.
>>>
>>> More to the point, find_any_file_locked() was added by commit
>>> e0aa651068bf ("nfsd: don't call nfsd_file_put from client states
>>> seqfile display"), which was merged several months /after/ commit
>>> ce3c4ad7f4ce ("NFSD: Fix possible sleep during
>>> nfsd4_release_lockowner()").
>>
>> Yes. To flesh out the history:
>> nfsd_file_put() was added in v5.4. In earlier kernels check_for_locks()
>> would never sleep. However the problem patch was backported 4.9, 4.14,
>> and 4.19 and should be reverted.
>
> I don't see "NFSD: Fix possible sleep during nfsd4_release_lockowner()"
> in any of those kernels. All but 4.19 are now EOL.
OK, I see it now. I'll ask stable to remove it from v4.19.y.
>> find_any_file_locked() was added in v6.2 so when this patch is
>> backported to 5.4, 5.10, 5.15, 5.17 - 6.1 it needs to include
>> find_and_file_locked()
>
> I think I'd rather leave those unperturbed until someone hits a real
> problem. Unless you have a distribution kernel that needs to see
> this fix in one of the LTS kernels? The supported stable/LTS kernels
> are 5.4, 5.10, 5.15, and 6.1.
>
>
>> The patch should apply unchanged to stable kernels 6.2 and later.
>
> I can add a Cc: stable #v6.2+
>
>
> --
> Chuck Lever
--
Chuck Lever
