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

On Tue, Jun 30, 2026, at 5:38 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.

> diff --git a/fs/lockd/svcshare.c b/fs/lockd/svcshare.c
> index 5ac0ec25d62d..4a1a09c209ea 100644
> --- a/fs/lockd/svcshare.c
> +++ b/fs/lockd/svcshare.c
> @@ -25,6 +25,24 @@ nlm_cmp_owner(struct lockd_share *share, struct 
> xdr_netobj *oh)
>  	    && !memcmp(share->s_owner.data, oh->data, oh->len);
>  }
> 
> +/*
> + * Recompute s_access / s_mode as the union of all positive refcount
> + * buckets.  Caller must hold the per-file f_mutex.
> + */

Is the f_mutex statement here accurate, and the matching sentence in the
commit message:

  The per-file f_mutex held by callers in fs/lockd/svcsubs.c continues
  to serialise share manipulation, so the new arrays do not require
  their own locking.

nlm_lookup_file() in fs/lockd/svcsubs.c takes file->f_mutex only around
nlm_do_fopen() and drops it before returning:

  fs/lockd/svcsubs.c:nlm_lookup_file() {
        ...
                mutex_lock(&file->f_mutex);
                nfserr = nlm_do_fopen(rqstp, file, mode);
                mutex_unlock(&file->f_mutex);
        ...
  }

By the time nlmsvc_proc_share() / nlmsvc_proc_unshare() call into
nlmsvc_share_file() / nlmsvc_unshare_file(), f_mutex is no longer held.
What actually serialises these requests is that lockd runs a single
service thread (svc_set_num_threads(serv, 0, 1) in fs/lockd/svc.c), so
the conclusion that the arrays need no extra locking holds.  Would it be
worth pointing the comment and commit message at the single-threaded
service rather than f_mutex?

The loop starts at v = 1, so s_access_counts[0] and s_mode_counts[0] are
incremented and decremented but never read.  That is fine for computing
the union, since index 0 (fsa_NONE / fsm_DN) contributes no bits.  It
does mean the free decision keys on the recomputed s_access / s_mode
rather than on every bucket, so the commit message wording

  the entry is freed only when every bucket has reached zero.

is slightly stronger than the code: an entry can be freed while
s_access_counts[0] or s_mode_counts[0] is still non-zero.  Harmless
given NONE/DN enforce nothing, but the stated invariant and the bucket-0
storage do not quite line up.


> +static void nlm_recompute_share(struct lockd_share *share)
> +{
> +	u32 new_access = 0, new_mode = 0, v;
> +
> +	for (v = 1; v < LOCKD_FSH_NR; v++) {
> +		if (share->s_access_counts[v])
> +			new_access |= v;
> +		if (share->s_mode_counts[v])
> +			new_mode |= v;
> +	}
> +	share->s_access = new_access;
> +	share->s_mode = new_mode;
> +}
> +
>  /**
>   * nlmsvc_share_file - create a share
>   * @host: Network client peer
> @@ -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;
>  }
> 

A remote NLMv4 client can drive access or mode out of {0..3} here
and write past s_access_counts[] / s_mode_counts[]. There are one
or two other spots with the same issue.

Thus your patch can't be applied as written.

However, today is your lucky day: I have a patch that regenerates
the XDR decoders to prevent exactly this issue, which I will post
forthwith.

I'm wondering about a Fixes tag. This issue has been around since
1da177e4c3f4 ("Linux-2.6.12-rc2"), but the fix depends mechanically
on the xdrgen conversion, so it certainly will not apply cleanly to
kernels earlier than v7.1, and a proper fix will need both the
regeneration patch and this one to be applied.


-- 
Chuck Lever