Re: [PATCH v2] lockd: refcount NLM_SHARE access/deny modes

On Wed, Jul 1, 2026, at 1:26 AM, Oscar Ou wrote:
> When an NFSv3/NLM client issues multiple NLM_SHARE calls from a single
> host for the same (file, owner) tuple, the current implementation
> overwrites the recorded access and deny modes with the latest pair.
> A subsequent NLM_UNSHARE then drops the entire entry, even if other
> grants were implicitly subsumed by the most recent SHARE.  This is
> particularly visible to Windows-style clients that map each open of
> a file to a distinct NLM_SHARE, all carrying the same NLM owner
> handle.  For example:
>
>     1. SHARE(access=RW, deny=W)   -> entry [RW, deny W]
>     2. SHARE(access=R,  deny=N)   -> entry [R, deny N]   (RW/W overwritten)
>     3. UNSHARE(access=R, deny=N)  -> entry freed
>     4. UNSHARE(access=RW, deny=W) -> nothing to release
>
> Track each of the four valid fsh_access and fsh_mode values with a
> small per-value refcount.  On SHARE the appropriate buckets are
> incremented; on UNSHARE they are decremented.  s_access / s_mode
> are recomputed in both paths as the union of all positive buckets
> with a non-NONE / non-DN value (indices 1..3), and the entry is
> freed once s_access and s_mode are both zero.
>
> NLM_UNSHARE gains the access and deny modes as arguments so the
> correct buckets can be decremented.  The two callers in svcproc.c
> and svc4proc.c are updated to forward the decoded values.
>
> Signed-off-by: Oscar Ou <oscarou-UelDjCVBxVpWk0Htik3J/[email protected]>
> ---
> Applies on top of "lockd: Regenerate NLMv4 XDR code" by Chuck Lever.
>
> No Fixes: tag.  The fix depends mechanically on the xdrgen
> regeneration, so a Fixes: tag would mislead -stable backporters.
> Happy to add one plus an informal Depends-on: line if preferred.
>
> Changes since v1:
> - Reworded the "buckets" sentence in the commit message: the entry is
>   freed once s_access and s_mode are both zero, not when every bucket
>   has reached zero.  Index 0 (fsa_NONE / fsm_DN) contributes no bits
>   to the union and does not gate freeing.
> - Dropped the incorrect claim that per-file f_mutex serialises the
>   new arrays, from both the commit message and the comment on
>   nlm_recompute_share().  f_mutex is released by nlm_lookup_file()
>   before the share helpers run.
>
>  fs/lockd/share.h    |  6 +++++-
>  fs/lockd/svc4proc.c |  4 +++-
>  fs/lockd/svcproc.c  |  4 +++-
>  fs/lockd/svcshare.c | 40 +++++++++++++++++++++++++++++++++++-----
>  4 files changed, 46 insertions(+), 8 deletions(-)

Hi Oscar -

First, a "process" note, then notes on your patch
follow below that.

Thanks for the "Applies on" and the change log. You've
addressed the concerns from my review of v1.

I want to highlight a couple of subtleties when sending
updated patch versions.

1. New thread per revision — no In-Reply-To: back to the
   old version.

Post v2 as its own top-level thread; don't reply-to the v1
cover or any v1 patch. Per the doc (lines 845–850): "for a
multi-patch series, it is generally best to avoid using
In-Reply-To: to link to older versions of the series …
so multiple versions don't become an unmanageable forest
of references."

This matters for our tooling: b4 and patchwork group a
thread by its reference chain. If v2 is threaded under v1,
b4 sees one giant thread and can pick up stale patches when
someone runs b4 am; maintainers who "apply the latest" can
grab the wrong blobs. A clean top-level thread per version
keeps b4 am/patchwork resolving to exactly the vN the
contributor intended.

2. Version the subject tag, not RESEND. [PATCH v2],
   [PATCH v2 01/27] (lines 716–719, 726–731). RESEND is
   only for re-sending an /unchanged/ series that got no
   response (lines 379–380) — a modified series is never
   RESEND.


> diff --git a/fs/lockd/svcshare.c b/fs/lockd/svcshare.c
> index 5ac0ec25d62d..b7372094d397 100644
> --- a/fs/lockd/svcshare.c
> +++ b/fs/lockd/svcshare.c
> @@ -64,12 +82,15 @@ nlmsvc_share_file(struct nlm_host *host, struct 
> nlm_file *file,
>  	share->s_host       = host;
>  	share->s_owner.data = ohdata;
>  	share->s_owner.len  = oh->len;
> +	memset(share->s_access_counts, 0, sizeof(share->s_access_counts));
> +	memset(share->s_mode_counts, 0, sizeof(share->s_mode_counts));
>  	share->s_next       = file->f_shares;
>  	file->f_shares      = share;
> 
>  update:
> -	share->s_access = access;
> -	share->s_mode = mode;
> +	share->s_access_counts[access]++;
> +	share->s_mode_counts[mode]++;
> +	nlm_recompute_share(share);
>  	return nlm_granted;
>  }
> 

NLM has no duplicate reply cache, so it depends on its request handlers
being idempotent: nlmsvc_dispatch() runs the procedure directly for
every request that arrives, with nothing to absorb a retransmit.

  fs/lockd/svc.c:nlmsvc_dispatch() {
  	...
  	*statp = procp->pc_func(rqstp);
  	...
  }

The rest of the NLM procedures are built around that invariant.  LOCK
reuses an existing block and lets vfs_lock_file(F_SETLK) re-apply a held
lock as a no-op, UNLOCK and CANCEL act on state that a replay simply
finds already gone, and FREE_ALL / SM_NOTIFY discard everything for a
host, so a replayed request lands on the same state.  Before this change
SHARE fit the same pattern: a repeated SHARE for an already-recorded
(host, owner) just reassigned s_access and s_mode.

With the per-value refcount a replayed SHARE instead increments the
bucket a second time.  Over UDP a lost reply followed by a client
retransmit reaches nlmsvc_share_file() twice for one open, and if the
client then sends one matching UNSHARE:

	SHARE(access=RW, deny=W)              -> s_access_counts[3] = 1
	SHARE(access=RW, deny=W) (retransmit) -> s_access_counts[3] = 2
	UNSHARE(access=RW, deny=W)            -> s_access_counts[3] = 1

nlm_recompute_share() still sees s_access_counts[3] > 0, so s_access
stays non-zero and the entry is never freed.  Can this leave a stale
share reservation that denies later conflicting opens until the host is
torn down?


> @@ -93,8 +116,15 @@ nlmsvc_unshare_file(struct nlm_host *host, struct 
> nlm_file *file,
>  	for (shpp = &file->f_shares; (share = *shpp) != NULL;
>  					shpp = &share->s_next) {
>  		if (share->s_host == host && nlm_cmp_owner(share, oh)) {
> -			*shpp = share->s_next;
> -			kfree(share);
> +			if (share->s_access_counts[access])
> +				share->s_access_counts[access]--;
> +			if (share->s_mode_counts[mode])
> +				share->s_mode_counts[mode]--;
> +			nlm_recompute_share(share);
> +			if (!share->s_access && !share->s_mode) {
> +				*shpp = share->s_next;
> +				kfree(share);
> +			}
>  			return nlm_granted;
>  		}
>  	}

The decrement side has the mirror problem.  The old UNSHARE dropped the
whole entry on the first matching call, so a replay found nothing and
returned granted, idempotent.  The refcount version decrements a bucket
that may still be positive, so a replayed UNSHARE keeps going:

	open #1: SHARE(RW, deny=W)     -> s_access_counts[3] = 1
	open #2: SHARE(RW, deny=W)     -> s_access_counts[3] = 2
	close #1: UNSHARE(RW, deny=W)  -> s_access_counts[3] = 1
	retransmit of that UNSHARE     -> s_access_counts[3] = 0  (entry freed)

Here the client still holds open #2, but its reservation is gone, so a
conflicting open from another owner would now be granted.  Can a
retransmitted UNSHARE release a grant the client still holds?

Taken together, is there a way to keep SHARE and UNSHARE idempotent
under retransmission, some per-open identity to match on rather than a
bare count, so a duplicated request lands on the same state the way the
other NLM procedures do?

One direction would be to track the set of held (access, deny)
combinations rather than a count of each.  There are only sixteen
possible pairs, so a single u16 held-mask with one bit per
(access << 2 | deny) covers it:

	SHARE(a, d):   held_mask |=  BIT((a << 2) | d);
	UNSHARE(a, d): held_mask &= ~BIT((a << 2) | d);

s_access and s_mode are then the union of (i >> 2) and (i & 3) over the
set bits, and the entry is freed once held_mask is zero.  Setting or
clearing a bit is idempotent, so a replayed SHARE or UNSHARE lands on
the same mask, while distinct (access, deny) opens from one owner are
still tracked separately.

The tradeoff is that two opens with the identical (access, deny) pair
collapse to one bit, so a count of duplicate identical opens is lost.
RFC 1813 defines SHARE and UNSHARE but does not require that repeated
identical SHAREs from one owner survive an equal number of UNSHAREs,
and its duplicate-request discussion (section 4.5) treats a
non-idempotent request as something a reply cache has to guard, which
lockd does not run for NLM.  Tracking each distinct (access, deny)
combination therefore stays within the protocol while keeping both
operations idempotent.  Does that match your reading?

The bottom line is that after more consideration, IMO the reference
count approach is not going to work, in particular because NLM still
operates on unreliable network transports like UDP. Consider the
above direction not as a mandate but as a possible approach that
might address the issue you've reported while avoiding introducing
non-idempotency.


-- 
Chuck Lever