[PATCH] NFSD: Report whether fh_key was actually updated

[PATCH] NFSD: Report whether fh_key was actually updated

[Chuck Lever]

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
 	)
 );
 
-- 
2.53.0

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

[Benjamin Coddington]

On 21 Apr 2026, at 15:20, 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>

Reviewed-by: Benjamin Coddington <bcodding@hammerspace.com>

Ben

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

[Jeff Layton]

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>