Re: [PATCH v2 1/3] NFSD: Add a key for signing filehandles

[Chuck Lever <cel@kernel.org>]

On 1/21/26 5:56 PM, Benjamin Coddington wrote:
> On 21 Jan 2026, at 17:17, Chuck Lever wrote:
> 
>> On 1/21/26 3:54 PM, Benjamin Coddington wrote:
>>> On 21 Jan 2026, at 15:43, Chuck Lever wrote:
>>>
>>>> On Wed, Jan 21, 2026, at 3:24 PM, Benjamin Coddington wrote:
>>>>> A future patch will enable NFSD to sign filehandles by appending a Message
>>>>> Authentication Code(MAC).  To do this, NFSD requires a secret 128-bit key
>>>>> that can persist across reboots.  A persisted key allows the server to
>>>>> accept filehandles after a restart.  Enable NFSD to be configured with this
>>>>> key via both the netlink and nfsd filesystem interfaces.
>>>>>
>>>>> Since key changes will break existing filehandles, the key can only be set
>>>>> once.  After it has been set any attempts to set it will return -EEXIST.
>>>>>
>>>>> Link:
>>>>> https://lore.kernel.org/linux-nfs/cover.1769026777.git.bcodding@hammerspace.com
>>>>> Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
>>>>> ---
>>>>>  Documentation/netlink/specs/nfsd.yaml |  6 ++
>>>>>  fs/nfsd/netlink.c                     |  5 +-
>>>>>  fs/nfsd/netns.h                       |  2 +
>>>>>  fs/nfsd/nfsctl.c                      | 94 +++++++++++++++++++++++++++
>>>>>  fs/nfsd/trace.h                       | 25 +++++++
>>>>>  include/uapi/linux/nfsd_netlink.h     |  1 +
>>>>>  6 files changed, 131 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/netlink/specs/nfsd.yaml
>>>>> b/Documentation/netlink/specs/nfsd.yaml
>>>>> index badb2fe57c98..d348648033d9 100644
>>>>> --- a/Documentation/netlink/specs/nfsd.yaml
>>>>> +++ b/Documentation/netlink/specs/nfsd.yaml
>>>>> @@ -81,6 +81,11 @@ attribute-sets:
>>>>>        -
>>>>>          name: min-threads
>>>>>          type: u32
>>>>> +      -
>>>>> +        name: fh-key
>>>>> +        type: binary
>>>>> +        checks:
>>>>> +            exact-len: 16
>>>>>    -
>>>>>      name: version
>>>>>      attributes:
>>>>> @@ -163,6 +168,7 @@ operations:
>>>>>              - leasetime
>>>>>              - scope
>>>>>              - min-threads
>>>>> +            - fh-key
>>>>>      -
>>>>>        name: threads-get
>>>>>        doc: get the number of running threads
>>>>> diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
>>>>> index 887525964451..81c943345d13 100644
>>>>> --- a/fs/nfsd/netlink.c
>>>>> +++ b/fs/nfsd/netlink.c
>>>>> @@ -24,12 +24,13 @@ const struct nla_policy
>>>>> nfsd_version_nl_policy[NFSD_A_VERSION_ENABLED + 1] = {
>>>>>  };
>>>>>
>>>>>  /* NFSD_CMD_THREADS_SET - do */
>>>>> -static const struct nla_policy
>>>>> nfsd_threads_set_nl_policy[NFSD_A_SERVER_MIN_THREADS + 1] = {
>>>>> +static const struct nla_policy
>>>>> nfsd_threads_set_nl_policy[NFSD_A_SERVER_FH_KEY + 1] = {
>>>>>  	[NFSD_A_SERVER_THREADS] = { .type = NLA_U32, },
>>>>>  	[NFSD_A_SERVER_GRACETIME] = { .type = NLA_U32, },
>>>>>  	[NFSD_A_SERVER_LEASETIME] = { .type = NLA_U32, },
>>>>>  	[NFSD_A_SERVER_SCOPE] = { .type = NLA_NUL_STRING, },
>>>>>  	[NFSD_A_SERVER_MIN_THREADS] = { .type = NLA_U32, },
>>>>> +	[NFSD_A_SERVER_FH_KEY] = NLA_POLICY_EXACT_LEN(16),
>>>>>  };
>>>>>
>>>>>  /* NFSD_CMD_VERSION_SET - do */
>>>>> @@ -58,7 +59,7 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
>>>>>  		.cmd		= NFSD_CMD_THREADS_SET,
>>>>>  		.doit		= nfsd_nl_threads_set_doit,
>>>>>  		.policy		= nfsd_threads_set_nl_policy,
>>>>> -		.maxattr	= NFSD_A_SERVER_MIN_THREADS,
>>>>> +		.maxattr	= NFSD_A_SERVER_FH_KEY,
>>>>>  		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>>>>  	},
>>>>>  	{
>>>>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>>>>> index 9fa600602658..c8ed733240a0 100644
>>>>> --- a/fs/nfsd/netns.h
>>>>> +++ b/fs/nfsd/netns.h
>>>>> @@ -16,6 +16,7 @@
>>>>>  #include <linux/percpu-refcount.h>
>>>>>  #include <linux/siphash.h>
>>>>>  #include <linux/sunrpc/stats.h>
>>>>> +#include <linux/siphash.h>
>>>>>
>>>>>  /* Hash tables for nfs4_clientid state */
>>>>>  #define CLIENT_HASH_BITS                 4
>>>>> @@ -224,6 +225,7 @@ struct nfsd_net {
>>>>>  	spinlock_t              local_clients_lock;
>>>>>  	struct list_head	local_clients;
>>>>>  #endif
>>>>> +	siphash_key_t		*fh_key;
>>>>>  };
>>>>>
>>>>>  /* Simple check to find out if a given net was properly initialized */
>>>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>>>>> index 30caefb2522f..e59639efcf5c 100644
>>>>> --- a/fs/nfsd/nfsctl.c
>>>>> +++ b/fs/nfsd/nfsctl.c
>>>>> @@ -49,6 +49,7 @@ enum {
>>>>>  	NFSD_Ports,
>>>>>  	NFSD_MaxBlkSize,
>>>>>  	NFSD_MinThreads,
>>>>> +	NFSD_Fh_Key,
>>>>>  	NFSD_Filecache,
>>>>>  	NFSD_Leasetime,
>>>>>  	NFSD_Gracetime,
>>>>> @@ -69,6 +70,7 @@ static ssize_t write_versions(struct file *file, char
>>>>> *buf, size_t size);
>>>>>  static ssize_t write_ports(struct file *file, char *buf, size_t size);
>>>>>  static ssize_t write_maxblksize(struct file *file, char *buf, size_t
>>>>> size);
>>>>>  static ssize_t write_minthreads(struct file *file, char *buf, size_t
>>>>> size);
>>>>> +static ssize_t write_fh_key(struct file *file, char *buf, size_t size);
>>>>>  #ifdef CONFIG_NFSD_V4
>>>>>  static ssize_t write_leasetime(struct file *file, char *buf, size_t
>>>>> size);
>>>>>  static ssize_t write_gracetime(struct file *file, char *buf, size_t
>>>>> size);
>>>>> @@ -88,6 +90,7 @@ static ssize_t (*const write_op[])(struct file *,
>>>>> char *, size_t) = {
>>>>>  	[NFSD_Ports] = write_ports,
>>>>>  	[NFSD_MaxBlkSize] = write_maxblksize,
>>>>>  	[NFSD_MinThreads] = write_minthreads,
>>>>> +	[NFSD_Fh_Key] = write_fh_key,
>>>>>  #ifdef CONFIG_NFSD_V4
>>>>>  	[NFSD_Leasetime] = write_leasetime,
>>>>>  	[NFSD_Gracetime] = write_gracetime,
>>>>> @@ -950,6 +953,60 @@ static ssize_t write_minthreads(struct file *file,
>>>>> char *buf, size_t size)
>>>>>  	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%u\n", minthreads);
>>>>>  }
>>>>>
>>>>> +/*
>>>>> + * write_fh_key - Set or report the current NFS filehandle key, the key
>>>>> + * 		can only be set once, else -EEXIST because changing the key
>>>>> + * 		will break existing filehandles.
>>>>
>>>> Do you really need both a /proc/fs/nfsd API and a netlink API? I
>>>> think one or the other would be sufficient, unless you have
>>>> something else in mind (in which case, please elaborate in the
>>>> patch description).
>>>
>>> Yes, some distros use one or the other.  Some try to use both!  Until you
>>> guys deprecate one of the interfaces I think we're stuck expanding them
>>> both.
>>
>> Neil has said he wants to keep /proc/fs/nfsd rather indefinitely, and
>> we have publicly stated we will add only to netlink unless it's
>> unavoidable. I prefer not growing the legacy API.
> 
> Having both is more complete, and doesn't introduce any conflicts or
> problems.

That doesn't tell me why you need it. It just says you want things to
be "tidy".


>> We generally don't backport new features like this one to stable
>> kernels, so IMO tucking this into only netlink is defensible.
> 
> Why only netlink for this one besides your preference?

You might be channeling one of your kids there.

As I stated before: we have said we don't want to continue adding
new APIs to procfs. It's not just NFSD that prefers this, it's a long
term project across the kernel. If you have a clear technical reason
that a new procfs API is needed, let's hear it.


> There's a very good reason for both interfaces - there's been no work to
> deprecate the old interface or co-ordination with distros to ensure they
> have fully adopted the netlink interface.  Up until now new features have
> been added to both interfaces.

I'm not seeing how this is a strong and specific argument for including
a procfs version of this specific interface. It's still saying "tidy" to
me and not explaining why we must have the extra clutter.

An example of a strong technical reason would be "We have legacy user
space applications that expect to find this API in procfs."


>> The procfs API has the ordering requirement that Jeff pointed out. I
>> don't think it's a safe API to allow the server to start up without
>> setting the key first. The netlink API provides a better guarantee
>> there.
> 
> It is harmless to allow the server to start up without setting the
> key first.  The server will refuse to give out filehandles for "sign_fh"
> exports and emit a warning in the log, so "safety" is the wrong word.

Sounds like it will cause spurious stale file handles, which will kill
applications on NFS mounts.


>>>> Also "set once" seems to be ambiguous. Is it "set once" per NFSD
>>>> module load, one per system boot epoch, or set once, _ever_ ?
>>>
>>> Once per nfsd module load - I can clarify next time.
>>>
>>>> While it's good UX safety to prevent reseting the key, there are
>>>> going to be cases where it is both needed and safe to replace the
>>>> FH signing key. Have you considered providing a key rotation
>>>> mechanism or a recipe to do so?
>>>
>>> I've considered it, but we do not need it at this point.
>>
>> I disagree: Admins will need to know how to replace an FH key that was
>> compromised. At the very least your docs should explain how to do that
>> safely.
> 
> Ok, I can add documentation for how to replace the key.
> 
>>> The key can
>>> be replaced today by restarting the server, and you really need to know what
>>> you're doing if you want to replace it.  Writing extra code to help someone
>>> that knows isn't really going to help them.  If a need appears for this, the
>>> work can get done.
>>
>> I cleverly said "a key rotation mechanism _or_ a recipe" so if it's
>> something you prefer only to document, let's take that route.
>>
>> Ensuring all clients have unmounted and then unloading nfsd.ko before
>> setting a fresh key is a lot of juggling. That should be enough to
>> prevent an FH key change by accident.
> 
> Adding instructions to unload the nfsd module would be full of footguns,
> depend on other features/modules and config options, and guaranteed to
> quickly be out of date.  It might be enough to say the system should be
> restarted.  The only reason for replacing the key is (as you've said) that
> it was compromised.  That should be rare and serious enough to justify
> restarting the server.

Again, I disagree. There can be other long-running services on the same
system as an NFS server. A system reboot might be fine for simple cases,
but not for all cases. We get "do I really have to reboot?" questions
all of the frickin time.

It isn't necessary to explain how every distribution handles "unload
nfsd.ko". "Shut down the NFS server using the administrative mechanisms
preferred by your distribution. Ensure that nfsd.ko has been unloaded
using 'lsmod'".

Type something up. We'll help you polish it.


-- 
Chuck Lever