Re: [PATCH] NFSD: Report whether fh_key was actually updated

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2026-04-21 at 15:20 -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> The nfsd_ctl_fh_key_set tracepoint was introduced to capture
> operator activity on the filehandle signing key. Earlier revisions
> logged the key bytes verbatim; the version that landed hashes the
> 16 key bytes through crc32_le and stores the result.
> 
> CRC32 is a linear projection of its input rather than a one-way
> function, and truncating any hash of fixed-size secret material
> leaves the key recoverable under offline brute force when the
> threat model includes an attacker with access to the trace ring.
> 
> The operational question the fingerprint was meant to answer is
> whether a NFSD_CMD_THREADS_SET call that carries an
> NFSD_A_SERVER_FH_KEY attribute actually replaced the active key or
> re-installed the value already in place. Answer that question
> directly: compare the incoming key bytes against the current
> nn->fh_key inside nfsd_nl_fh_key_set() and surface a single bit to
> the tracepoint. The event now prints "updated" when the stored
> key changed and "unmodified" otherwise. A first set that fails
> kmalloc reports "unmodified" because no key was installed.
> 
> Reported-by: jaeyeong <fin@spl.team>
> Fixes: 62346217fd72 ("NFSD: Add a key for signing filehandles")
> Cc: Benjamin Coddington <bcodding@hammerspace.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfsctl.c | 18 ++++++++++++++----
>  fs/nfsd/trace.h  | 16 +++++++---------
>  2 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index c76849f316f7..064a2e749bc9 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1610,16 +1610,27 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
>  static int nfsd_nl_fh_key_set(const struct nlattr *attr, struct nfsd_net *nn)
>  {
>  	siphash_key_t *fh_key = nn->fh_key;
> +	u64 k0, k1;
> +	bool changed;
> +
> +	k0 = get_unaligned_le64(nla_data(attr));
> +	k1 = get_unaligned_le64(nla_data(attr) + 8);
>  
>  	if (!fh_key) {
>  		fh_key = kmalloc(sizeof(siphash_key_t), GFP_KERNEL);
> -		if (!fh_key)
> +		if (!fh_key) {
> +			trace_nfsd_ctl_fh_key_set(false, -ENOMEM);
>  			return -ENOMEM;
> +		}
>  		nn->fh_key = fh_key;
> +		changed = true;
> +	} else {
> +		changed = fh_key->key[0] != k0 || fh_key->key[1] != k1;
>  	}
>  
> -	fh_key->key[0] = get_unaligned_le64(nla_data(attr));
> -	fh_key->key[1] = get_unaligned_le64(nla_data(attr) + 8);
> +	fh_key->key[0] = k0;
> +	fh_key->key[1] = k1;
> +	trace_nfsd_ctl_fh_key_set(changed, 0);
>  	return 0;
>  }
>  
> @@ -1698,7 +1709,6 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
>  		attr = info->attrs[NFSD_A_SERVER_FH_KEY];
>  		if (attr) {
>  			ret = nfsd_nl_fh_key_set(attr, nn);
> -			trace_nfsd_ctl_fh_key_set((const char *)nn->fh_key, ret);
>  			if (ret)
>  				goto out_unlock;
>  		}
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index a13d18447324..1c5a1e50f946 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -2263,23 +2263,21 @@ TRACE_EVENT(nfsd_end_grace,
>  
>  TRACE_EVENT(nfsd_ctl_fh_key_set,
>  	TP_PROTO(
> -		const char *key,
> +		bool changed,
>  		int result
>  	),
> -	TP_ARGS(key, result),
> +	TP_ARGS(changed, result),
>  	TP_STRUCT__entry(
> -		__field(u32, key_hash)
> +		__field(bool, changed)
>  		__field(int, result)
>  	),
>  	TP_fast_assign(
> -		if (key)
> -			__entry->key_hash = ~crc32_le(0xFFFFFFFF, key, 16);
> -		else
> -			__entry->key_hash = 0;
> +		__entry->changed = changed;
>  		__entry->result = result;
>  	),
> -	TP_printk("key=0x%08x result=%d",
> -		__entry->key_hash, __entry->result
> +	TP_printk("key %s, result=%d",
> +		__entry->changed ? "updated" : "unmodified",
> +		__entry->result
>  	)
>  );
>  

Reviewed-by: Jeff Layton <jlayton@kernel.org>