Re: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
From: Benjamin Coddington <hidden>
Date: 2026-01-22 15:17:35
Also in:
linux-fsdevel, linux-nfs
On 22 Jan 2026, at 9:49, Chuck Lever wrote:
On Wed, Jan 21, 2026, at 8:22 PM, Benjamin Coddington wrote:quoted
On 21 Jan 2026, at 18:55, Chuck Lever wrote:quoted
On 1/21/26 5:56 PM, Benjamin Coddington wrote:quoted
On 21 Jan 2026, at 17:17, Chuck Lever wrote:quoted
On 1/21/26 3:54 PM, Benjamin Coddington wrote:quoted
On 21 Jan 2026, at 15:43, Chuck Lever wrote:quoted
On Wed, Jan 21, 2026, at 3:24 PM, Benjamin Coddington wrote:quoted
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 (local) Signed-off-by: Benjamin Coddington <redacted> --- 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.yamlb/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 threadsdiff --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_policynfsd_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".quoted
quoted
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.That's unnecessary.Is it? There's no point in asking that question other than as the kind of jab a kid makes when trying to catch a parent in a contradiction (which is exactly what you continue with below).
Wow, yes. It's personal, and unprofessional. It has nothing to do with the merits of the argument at hand. I'm not trying to catch you in a contradiction Chuck. You stated it was your preference, and your reasons had (IMO) a valid counter-argument.
It doesn't make a difference whether it's my preference or not, and frankly, as a contributor, it's not your role to decide whether an interface goes in procfs or not.
It does make a difference, because this is community software. I'm not trying to decide, I'm responding to your words and actions. I was having trouble resolving your desire to make sure the server is never started w/o a key, while you also ask to remove an interface that can create exactly that situation.
Don't argue with me.
Got it - I'm really sad to read this.
Just answer my questions. All I'm asking here is why are you adding it.
You got it, I'm trying to do that. Let's take it down a notch.
quoted
quoted
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.You've just added one to your nfsd-testing branch two weeks ago that you asked me to rebase onto.Sorry for being human. Sometimes I don't notice things. That one doesn't belong there either. But each one of these is decided on a case-by-case basis. It's not appropriate for you to compare your procfs addition to any other as a basis for "permission to add the API".
No need to apologize, it's just confusing. I have a right to point it out.
Again, this is argumentative, not constructive. You're not answering a direct question from a reviewer/maintainer. What is the reason you need this API?
Sorry - I am making a best effort trying to do that. When I see you write "I prefer not growing the legacy API" right after you grew the legacy API it creates dissonance, so I'm pointing it out. Which is good apparently, because now you'll have more of what you want.
quoted
quoted
quoted
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 systemd startup for the nfs-server in RHEL falls back to rpc.nfsd on nfsdctl failure. Without the additional interface you can have systems that start the nfs-server via rpc.nfsd without setting the key - exactly the situation you're so adamant should never happen in your below argument..If your intention is to get distributions to backport signed FHs to kernels that do not have the NFSD netlink interface, you need to have stated that up front. That is the rationale I'm looking for here, and I really should not have to work this hard getting that answer from you.
I'm doing my best, I understand you're unhappy with me.
I might have deleted this while editing my reply yesterday, but as a policy, LTS kernels do not get new features like this. So it was never my intention to plan to target upstream backports for this feature. If this is part of your vision for this feature, you will need to make a similar case to the stable@ folks.
We'll probably work with what we can from what you've decided you want. As I've already let Jeff know - I'll remove that interface on the next posting. Ben