kNFSD Signed Filehandles
Subsystems (6)
v2 cover kNFSD Signed Filehandles
The following series enables the linux NFS server to add a Message Authentication Code (MAC) to the filehandles it gives to clients. This provides additional protection to the exported filesystem against filehandle guessing attacks.
Filesystems generate their own filehandles through the export_operation "encode_fh" and a filehandle provides sufficient access to open a file without needing to perform a lookup. A trusted NFS client holding a valid filehandle can remotely access the corresponding file without reference to access-path restrictions that might be imposed by the ancestor directories or the server exports.
In order to acquire a filehandle, you must perform lookup operations on the parent directory(ies), and the permissions on those directories may prohibit you from walking into them to find the files within. This would normally be considered sufficient protection on a local filesystem to prohibit users from accessing those files, however when the filesystem is exported via NFS an exported file can be accessed whenever the NFS server is presented with the correct filehandle, which can be guessed or acquired by means other than LOOKUP.
Filehandles are easy to guess because they are well-formed. The open_by_handle_at(2) man page contains an example C program (t_name_to_handle_at.c) that can display a filehandle given a path. Here's an example filehandle from a fairly modern XFS:
# ./t_name_to_handle_at /exports/foo 57 12 129 99 00 00 00 00 00 00 00 b4 10 0b 8c
^--------- filehandle ----------^
^------- inode -------^ ^-- gen --^This filehandle consists of a 64-bit inode number and 32-bit generation number. Because the handle is well-formed, its easy to fabricate filehandles that match other files within the same filesystem. You can simply insert inode numbers and iterate on the generation number. Eventually you'll be able to access the file using open_by_handle_at(2). For a local system, open_by_handle_at(2) requires CAP_DAC_READ_SEARCH, which protects against guessing attacks by unprivileged users.
In contrast to a local user using open_by_handle(2), the NFS server must permissively allow remote clients to open by filehandle without being able to check or trust the remote caller's access. Therefore additional protection against this attack is needed for NFS case. We propose to sign filehandles by appending an 8-byte MAC which is the siphash of the filehandle from a key set from the nfs-utilities. NFS server can then ensure that guessing a valid filehandle+MAC is practically impossible without knowledge of the MAC's key. The NFS server performs optional signing by possessing a key set from userspace and having the "sign_fh" export option.
Because filehandles are long-lived, and there's no method for expiring them, the server's key should be set once and not changed. It also should be persisted across restarts. The methods to set the key allow only setting it once, afterward it cannot be changed. A separate patchset for nfs-utils contains the userspace changes required to set the server's key.
I had planned on adding additional work to enable the server to check whether the 8-byte MAC will overflow maximum filehandle length for the protocol at export time. There could be some filesystems with 40-byte fileid and 24-byte fsid which would break NFSv3's 64-byte filehandle maximum with an 8-byte MAC appended. The server should refuse to export those filesystems when "sign_fh" is requested. However, the way the export caches work (the server may not even be running when a user sets up the export) its impossible to do this check at export time. Instead, the server will refuse to give out filehandles at mount time and emit a pr_warn().
Thanks for any comments and critique.
Changes from encrypt_fh posting:
https://lore.kernel.org/linux-nfs/[email protected]
- sign filehandles instead of encrypt them (Eric Biggers)
- fix the NFSEXP_ macros, specifically NFSEXP_ALLFLAGS (NeilBrown)
- rebase onto cel/nfsd-next (Chuck Lever)
- condensed/clarified problem explantion (thanks Chuck Lever)
- add nfsctl file "fh_key" for rpc.nfsd to also set the key
Changes from v1 posting:
https://lore.kernel.org/linux-nfs/[email protected]
- remove fh_fileid_offset() (Chuck Lever)
- fix pr_warns, fix memcmp (Chuck Lever)
- remove incorrect rootfh comment (NeilBrown)
- make fh_key setting an optional attr to threads verb (Jeff Layton)
- drop BIT() EXP_ flag conversion
- cover-letter tune-ups (NeilBrown, Chuck Lever)
- fix NFSEXP_ALLFLAGS on 2/3
- cast fh->fh_size + sizeof(hash) result to int (avoid x86_64 WARNING)
- move MAC signing into __fh_update() (Chuck Lever)
Benjamin Coddington (3):
NFSD: Add a key for signing filehandles
NFSD/export: Add sign_fh export option
NFSD: Sign filehandles
Documentation/netlink/specs/nfsd.yaml | 6 ++ fs/nfsd/export.c | 5 +- fs/nfsd/netlink.c | 5 +- fs/nfsd/netns.h | 2 + fs/nfsd/nfsctl.c | 94 +++++++++++++++++++++++++++ fs/nfsd/nfsfh.c | 73 ++++++++++++++++++++- fs/nfsd/nfsfh.h | 3 + fs/nfsd/trace.h | 25 +++++++ include/uapi/linux/nfsd/export.h | 4 +- include/uapi/linux/nfsd_netlink.h | 1 + 10 files changed, 209 insertions(+), 9 deletions(-)
base-commit: d22020d8bae386de422692a0957e991843927643 (cel/nfsd-testing)
Signature · 1 line
2.50.1
PATCH v2 1/3 NFSD: Add a key for signing filehandles
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.
--- 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(-)
Documentation/netlink/specs/nfsd.yaml +6 -0
@@ -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
fs/nfsd/netlink.c +3 -2
@@ -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, }, {
fs/nfsd/netns.h +2 -0
@@ -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 */
fs/nfsd/nfsctl.c +94 -0
142 lines not on this page · raw message
fs/nfsd/trace.h +25 -0
@@ -2240,6 +2240,31 @@ TRACE_EVENT(nfsd_end_grace, ) ); +TRACE_EVENT (nfsd_ctl_fh_key_set, +TP_PROTO ( + const char *key, + int result + ), +TP_ARGS (key, result), +TP_STRUCT__entry ( + __array(unsigned char, key, 16) + __field(unsigned long, result) +__field (bool, key_set) + ), +TP_fast_assign ( + __entry->key_set = true; + if (!key) + __entry->key_set = false; + else + memcpy(__entry->key, key, 16); + __entry->result = result; + ), +TP_printk ("key=%s result=%ld" , + __entry->key_set ?__print_hex_str (__entry->key,16 ) :"(null)" , + __entry->result + ) +); + DECLARE_EVENT_CLASS(nfsd_copy_class, TP_PROTO( const struct nfsd4_copy *copy
include/uapi/linux/nfsd_netlink.h +1 -0
@@ -36,6 +36,7 @@ enum {NFSD_A_SERVER_LEASETIME ,NFSD_A_SERVER_SCOPE ,NFSD_A_SERVER_MIN_THREADS , +NFSD_A_SERVER_FH_KEY , __NFSD_A_SERVER_MAX,NFSD_A_SERVER_MAX = (__NFSD_A_SERVER_MAX -1 )
Signature · 1 line
2.50.1
Chuck Lever
maintainerRe: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
Quoted 132 lines · Benjamin Coddington
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/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- 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(-)Documentation/netlink/specs/nfsd.yaml +6
-0@@ -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 threadsfs/nfsd/netlink.c +2
-1@@ -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 */fs/nfsd/netlink.c +1
-1@@ -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, }, {fs/nfsd/netns.h +2
-0@@ -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 */ fs/nfsd/nfsctl.c +2
-0@@ -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
staticssize_twrite_leasetime(struct file *file, char *buf,size_t
size);
staticssize_twrite_gracetime(struct file *file, char *buf,size_t
size);fs/nfsd/nfsctl.c +1
-0@@ -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,fs/nfsd/nfsctl.c +0
-0[NFSD_Gracetime] = write_gracetime,
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, · line 960
Quoted 8 lines
fs/nfsd/nfsctl.c +4
-0@@ -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).
Also "set once" seems to be ambiguous. Is it "set once" per NFSD module load, one per system boot epoch, or set once, _ever_ ?
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?
Quoted 179 lines
fs/nfsd/nfsctl.c +50
-0+* +* Input: +* buf: ignored +* size: zero +* OR +* +* Input: +* buf: C string containing a parseable UUID +* size: non-zero length of C string in @buf +* Output: +* On success: passed-in buffer filled with '\n'-terminated C string +* containing the standard UUID format of the server's fh_key +* return code is the size in bytes of the string +* On error: return code is zero or a negative errno value +*/ +static ssize_t write_fh_key (struct file *file,char *buf,size_t size) +{ +struct nfsd_net *nn =net_generic (netns (file), nfsd_net_id); +int ret = -EEXIST ; + +if (size >35 && size <38 ) { +siphash_key_t *sip_fh_key; +uuid_t uuid_fh_key; + +mutex_lock (&nfsd_mutex); + +/* Is the key already set? */ +if (nn->fh_key) + goto out; + + ret =uuid_parse (buf, &uuid_fh_key); +if (ret) + goto out; + + sip_fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!sip_fh_key) { + ret = -ENOMEM ; + goto out; + } + +memcpy (sip_fh_key, &uuid_fh_key,sizeof (siphash_key_t)); + nn->fh_key = sip_fh_key; + } + ret =scnprintf (buf,SIMPLE_TRANSACTION_LIMIT ,"%pUb\n" , nn->fh_key); +out: +mutex_unlock (&nfsd_mutex); +trace_nfsd_ctl_fh_key_set ((const char *)nn->fh_key, ret); +return ret; +} +#ifdef CONFIG_NFSD_V4 static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_tsize,
fs/nfsd/nfsctl.c +1
-0time64_t *time, struct nfsd_net *nn)@@ -1343,6 +1400,7 @@ static int nfsd_fill_super(struct super_block *sb, struct fs_context *fc) [NFSD_Ports] = {"portlist", &transaction_ops, S_IWUSR|S_IRUGO}, [NFSD_MaxBlkSize] = {"max_block_size" , &transaction_ops, S_IWUSR|S_IRUGO}, [NFSD_MinThreads] = {"min_threads" , &transaction_ops, S_IWUSR|S_IRUGO}, + [NFSD_Fh_Key] = {"fh_key", &transaction_ops, S_IWUSR|S_IRUSR},[
NFSD_Filecache] = {"filecache",&nfsd_file_cache_stats_fops,S_IRUGO},
#ifdefCONFIG_NFSD_V4
[NFSD_Leasetime] = {"nfsv4leasetime",&transaction_ops,
S_IWUSR|S_IRUSR},fs/nfsd/nfsctl.c +6
-0@@ -1615,6 +1673,33 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb, return ret; } +/** +* nfsd_nl_fh_key_set - helper to copy fh_key from userspace +* @attr: nlattr NFSD_A_SERVER_FH_KEY +* @nn: nfsd_net +* +* Callers should hold nfsd_mutex, returns 0 on success or negative errno.
+ */
+static intnfsd_nl_fh_key_set(const struct nlattr *attr, struct nfsd_net *nn)fs/nfsd/nfsctl.c +27
-0+{ +siphash_key_t *fh_key; + +if (nla_len (attr) !=sizeof (siphash_key_t)) +return -EINVAL ; + +/* Is the key already set? */ +if (nn->fh_key) +return -EEXIST ; + + fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!fh_key) +return -ENOMEM ; + +memcpy (fh_key,nla_data (attr),sizeof (siphash_key_t)); + nn->fh_key = fh_key; +return 0 ; +} +/** * nfsd_nl_threads_set_doit - set the number of running threads * @skb: reply buffer @@ -1691,6 +1776,14 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)if (attr) nn->min_threads =nla_get_u32 (attr); + 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 && ret != -EEXIST ) + goto out_unlock; + } + ret =nfsd_svc (nrpools, nthreads, net,get_current_cred (), scope);if (ret >0 )fs/nfsd/nfsctl.c +1
-0ret =0 ;@@ -2284,6 +2377,7 @@ static __net_exit void nfsd_net_exit(struct net *net) {struct nfsd_net *nn =net_generic (net, nfsd_net_id); +kfree_sensitive (nn->fh_key);nfsd_proc_stat_shutdown (net);percpu_counter_destroy_many (nn->counter,NFSD_STATS_COUNTERS_NUM );nfsd_idmap_shutdown (net);fs/nfsd/trace.h +25
-0@@ -2240,6 +2240,31 @@ TRACE_EVENT(nfsd_end_grace, ) ); +TRACE_EVENT (nfsd_ctl_fh_key_set, +TP_PROTO ( + const char *key, + int result + ), +TP_ARGS (key, result), +TP_STRUCT__entry ( + __array(unsigned char, key, 16) + __field(unsigned long, result) +__field (bool, key_set) + ), +TP_fast_assign ( + __entry->key_set = true; + if (!key) + __entry->key_set = false; + else + memcpy(__entry->key, key, 16); + __entry->result = result; + ), +TP_printk ("key=%s result=%ld" , + __entry->key_set ?__print_hex_str (__entry->key,16 ) :"(null)" , + __entry->result + ) +); + DECLARE_EVENT_CLASS(nfsd_copy_class, TP_PROTO( const struct nfsd4_copy *copyinclude/uapi/linux/nfsd_netlink.h +1
-0@@ -36,6 +36,7 @@ enum {NFSD_A_SERVER_LEASETIME ,NFSD_A_SERVER_SCOPE ,NFSD_A_SERVER_MIN_THREADS , +NFSD_A_SERVER_FH_KEY , __NFSD_A_SERVER_MAX,NFSD_A_SERVER_MAX = (__NFSD_A_SERVER_MAX -1 )Signature · 1 line
2.50.1
Signature · 1 line
Chuck Lever
Re: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
Quoted 137 lines · Chuck Lever
Quoted 136 lines · Benjamin Coddington
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/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- 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(-)Documentation/netlink/specs/nfsd.yaml +6
-0@@ -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 threadsfs/nfsd/netlink.c +2
-1@@ -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 */fs/nfsd/netlink.c +1
-1@@ -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, }, {fs/nfsd/netns.h +2
-0@@ -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 */ fs/nfsd/nfsctl.c +2
-0@@ -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
staticssize_twrite_leasetime(struct file *file, char *buf,size_t
size);
staticssize_twrite_gracetime(struct file *file, char *buf,size_t
size);fs/nfsd/nfsctl.c +1
-0@@ -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,fs/nfsd/nfsctl.c +0
-0[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); }
Quoted 9 lines
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, char *buf, size_t size) · line 959
Quoted 4 lines
fs/nfsd/nfsctl.c +4
-0+/* +* 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/nfsdAPI 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.
Quoted 2 lines
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.
Quoted 4 lines
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. 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.
Ben
Chuck Lever
maintainerRe: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
Quoted 149 lines · Benjamin Coddington
Quoted 137 lines · Chuck Lever
Quoted 136 lines · Benjamin Coddington
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/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- 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(-)Documentation/netlink/specs/nfsd.yaml +6
-0@@ -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 threadsfs/nfsd/netlink.c +2
-1@@ -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 */fs/nfsd/netlink.c +1
-1@@ -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, }, {fs/nfsd/netns.h +2
-0@@ -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 */ fs/nfsd/nfsctl.c +2
-0@@ -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
staticssize_twrite_leasetime(struct file *file, char *buf,size_t
size);
staticssize_twrite_gracetime(struct file *file, char *buf,size_t
size);fs/nfsd/nfsctl.c +1
-0@@ -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,fs/nfsd/nfsctl.c +0
-0[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); }Quoted 9 lines
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, char *buf, size_t size) · line 959
Quoted 4 lines
fs/nfsd/nfsctl.c +4
-0+/* +* 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/nfsdAPI 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).
Quoted 3 lines
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.
We generally don't backport new features like this one to stable kernels, so IMO tucking this into only netlink is defensible.
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.
Quoted 11 lines
Quoted 2 lines
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.
Quoted 4 lines
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.
Quoted 5 lines
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.
Signature · 1 line
Chuck Lever
Re: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
Quoted 150 lines · Chuck Lever
Quoted 149 lines · Benjamin Coddington
Quoted 146 lines · Chuck Lever
Quoted 132 lines · Benjamin Coddington
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/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- 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(-)Documentation/netlink/specs/nfsd.yaml +6
-0@@ -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 threadsfs/nfsd/netlink.c +2
-1@@ -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 */fs/nfsd/netlink.c +1
-1@@ -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, }, {fs/nfsd/netns.h +2
-0@@ -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 */ fs/nfsd/nfsctl.c +2
-0@@ -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
staticssize_twrite_leasetime(struct file *file, char *buf,size_t
size);
staticssize_twrite_gracetime(struct file *file, char *buf,size_t
size);fs/nfsd/nfsctl.c +1
-0@@ -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,fs/nfsd/nfsctl.c +0
-0[NFSD_Gracetime] = write_gracetime,fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, · line 960
Quoted 8 lines
fs/nfsd/nfsctl.c +4
-0@@ -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/nfsdAPI 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).
Quoted 7 lines
Quoted 3 lines
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/nfsdrather 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.
Quoted 2 lines
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?
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.
Quoted 4 lines
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.
Quoted 10 lines
Quoted 10 lines
Quoted 2 lines
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.
Quoted 4 lines
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?
Quoted 5 lines
Quoted 1 line
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.
Quoted 6 lines
Quoted 5 lines
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.
Quoted 6 lines
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.
Ben
Chuck Lever
maintainerRe: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
Quoted 156 lines · Benjamin Coddington
Quoted 154 lines · Chuck Lever
Quoted 152 lines · Benjamin Coddington
Quoted 137 lines · Chuck Lever
Quoted 136 lines · Benjamin Coddington
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/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- 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(-)Documentation/netlink/specs/nfsd.yaml +6
-0@@ -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 threadsfs/nfsd/netlink.c +2
-1@@ -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 */fs/nfsd/netlink.c +1
-1@@ -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, }, {fs/nfsd/netns.h +2
-0@@ -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 */ fs/nfsd/nfsctl.c +2
-0@@ -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
staticssize_twrite_leasetime(struct file *file, char *buf,size_t
size);
staticssize_twrite_gracetime(struct file *file, char *buf,size_t
size);fs/nfsd/nfsctl.c +1
-0@@ -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,fs/nfsd/nfsctl.c +0
-0[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); }Quoted 9 lines
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, char *buf, size_t size) · line 959
Quoted 4 lines
fs/nfsd/nfsctl.c +4
-0+/* +* 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/nfsdAPI 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.
Quoted 6 lines
Quoted 3 lines
Neil has said he wants to keep
/proc/fs/nfsdrather 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 4 lines
Quoted 2 lines
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.
Quoted 4 lines
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."
Quoted 8 lines
Quoted 4 lines
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.
Quoted 31 lines
Quoted 10 lines
Quoted 10 lines
Quoted 2 lines
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.
Quoted 4 lines
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?
Quoted 5 lines
Quoted 1 line
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.
Quoted 6 lines
Quoted 5 lines
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.Quoted 6 lines
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.
Quoted 6 lines
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.
Signature · 1 line
Chuck Lever
Re: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
Quoted 168 lines · Chuck Lever
Quoted 156 lines · Benjamin Coddington
Quoted 154 lines · Chuck Lever
Quoted 152 lines · Benjamin Coddington
Quoted 137 lines · Chuck Lever
Quoted 136 lines · Benjamin Coddington
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/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- 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(-)Documentation/netlink/specs/nfsd.yaml +6
-0@@ -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 threadsfs/nfsd/netlink.c +2
-1@@ -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 */fs/nfsd/netlink.c +1
-1@@ -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, }, {fs/nfsd/netns.h +2
-0@@ -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 */ fs/nfsd/nfsctl.c +2
-0@@ -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
staticssize_twrite_leasetime(struct file *file, char *buf,size_t
size);
staticssize_twrite_gracetime(struct file *file, char *buf,size_t
size);fs/nfsd/nfsctl.c +1
-0@@ -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,fs/nfsd/nfsctl.c +0
-0[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); }Quoted 9 lines
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, char *buf, size_t size) · line 959
Quoted 4 lines
fs/nfsd/nfsctl.c +4
-0+/* +* 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/nfsdAPI 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.
Quoted 6 lines
Quoted 3 lines
Neil has said he wants to keep
/proc/fs/nfsdrather 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 6 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 4 lines
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.
Quoted 11 lines
Quoted 4 lines
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..
Quoted 11 lines
Quoted 8 lines
Quoted 4 lines
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.
^^ here.
Ben
Jeff Layton
maintainerRe: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
Quoted 179 lines · Benjamin Coddington
Quoted 168 lines · Chuck Lever
Quoted 156 lines · Benjamin Coddington
Quoted 154 lines · Chuck Lever
Quoted 152 lines · Benjamin Coddington
Quoted 137 lines · Chuck Lever
Quoted 136 lines · Benjamin Coddington
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/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- 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(-)Documentation/netlink/specs/nfsd.yaml +6
-0@@ -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 threadsfs/nfsd/netlink.c +2
-1@@ -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 */fs/nfsd/netlink.c +1
-1@@ -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, }, {fs/nfsd/netns.h +2
-0@@ -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 */ fs/nfsd/nfsctl.c +2
-0@@ -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
staticssize_twrite_leasetime(struct file *file, char *buf,size_t
size);
staticssize_twrite_gracetime(struct file *file, char *buf,size_t
size);fs/nfsd/nfsctl.c +1
-0@@ -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,fs/nfsd/nfsctl.c +0
-0[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); }Quoted 9 lines
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, · line 960
Quoted 4 lines
fs/nfsd/nfsctl.c +4
-0+/* +* 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/nfsdAPI 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.
Quoted 6 lines
Quoted 3 lines
Neil has said he wants to keep
/proc/fs/nfsdrather 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 6 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 4 lines
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.
Mea culpa. I probably should have dropped the min-threads procfile from those patches, but it was convenient when I was doing the development work. Chuck, if you like I can send a patch to remove it before the merge window.
I can't see why we need both interfaces. The old /proc interface is really for the case where you have old nfs-utils and/or an old kernel. In order to use this, you need both new nfs-utils and new kernel. If you have those, then both should support the netlink interface.
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
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.
Quoted 8 lines
Quoted 2 lines
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..
The main reason it would fail is because the kernel doesn't support the netlink interface (or e.g. nfsdctl isn't present at all). If it fails with the netlink interface for some other reason, it's quite likely to have the same failure with procfs.
To be clear, the procfs interface is categorically inferior due to its piecemeal nature. There's little guidance as to how to the changes in nfsdfs should be ordered. We mostly make it work, but the cracks were showing in those interfaces long before. We really don't want to be expanding it.
Signature · 1 line
Jeff Layton <[email protected]>
Re: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
Quoted 194 lines · Jeff Layton
Quoted 179 lines · Benjamin Coddington
Quoted 168 lines · Chuck Lever
Quoted 156 lines · Benjamin Coddington
Quoted 154 lines · Chuck Lever
Quoted 152 lines · Benjamin Coddington
Quoted 137 lines · Chuck Lever
Quoted 136 lines · Benjamin Coddington
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/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- 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(-)Documentation/netlink/specs/nfsd.yaml +6
-0@@ -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 threadsfs/nfsd/netlink.c +2
-1@@ -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 */fs/nfsd/netlink.c +1
-1@@ -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, }, {fs/nfsd/netns.h +2
-0@@ -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 */ fs/nfsd/nfsctl.c +2
-0@@ -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
staticssize_twrite_leasetime(struct file *file, char *buf,size_t
size);
staticssize_twrite_gracetime(struct file *file, char *buf,size_t
size);fs/nfsd/nfsctl.c +1
-0@@ -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,fs/nfsd/nfsctl.c +0
-0[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); }Quoted 9 lines
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, · line 960
Quoted 4 lines
fs/nfsd/nfsctl.c +4
-0+/* +* 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/nfsdAPI 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.
Quoted 6 lines
Quoted 3 lines
Neil has said he wants to keep
/proc/fs/nfsdrather 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 6 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 4 lines
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.
Mea culpa. I probably should have dropped the min-threads procfile from those patches, but it was convenient when I was doing the development work. Chuck, if you like I can send a patch to remove it before the merge window.
Quoted 4 lines
I can't see why we need both interfaces. The old /proc interface is really for the case where you have old nfs-utils and/or an old kernel. In order to use this, you need both new nfs-utils and new kernel. If you have those, then both should support the netlink interface.
I'm not trying to win an argument about how I want it, but I want to just point out one more thing: its possible to have products built out of the server where the tooling so far hasn't been taught to use nfsdctl yet. We're in that situation - we will backport the kernel bits here, and use the /proc interface because the tooling hasn't been converted to nfsdctl yet.
Quoted 18 lines
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
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.
Quoted 8 lines
Quoted 2 lines
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..
Quoted 4 lines
The main reason it would fail is because the kernel doesn't support the netlink interface (or e.g. nfsdctl isn't present at all). If it fails with the netlink interface for some other reason, it's quite likely to have the same failure with procfs.
You're right, but it also could fail for any number of other reasons - admittedly unlikely ones.
Quoted 5 lines
To be clear, the procfs interface is categorically inferior due to its piecemeal nature. There's little guidance as to how to the changes in nfsdfs should be ordered. We mostly make it work, but the cracks were showing in those interfaces long before. We really don't want to be expanding it.
I understand. I'll remove the procfs interface here on the next posting, and thanks for chiming in here.
One thing that is confusing with nfsdctl is that when it reports a failure its not easy to figure out what's going wrong. It ends up having its own ordering stuff - for example:
(fresh boot)
root@bcodding:~# nfsdctl threads 4 nfsdctl: nfsdctl started nfsdctl: failed to resolve nfsd generic netlink family nfsdctl: nfsdctl exiting root@bcodding:~# modprobe nfsd root@bcodding:~# nfsdctl threads 4 nfsdctl: nfsdctl started nfsdctl: Error: Input/output error nfsdctl: nfsdctl exiting
^^ that's an actual situation I encountered, then tried to diagnose with strace (no good), then had to turn on the function tracer in the kernel to find that the error was coming from somewhere in nfs4_state_start(). I think something not getting set (listener?), finally I looked in the log and found:
[ 47.428237] NFSD: Failed to start, no listeners configured.
ah - so then I knew that I had to use "autostart" before being able to use the "threads". Compared with rpc.nfsd in the same situation:
root@bcodding:~# rpc.nfsd 4 rpc.nfsd: knfsd is currently down rpc.nfsd: Writing version string to kernel: -2 +3 rpc.nfsd: Created AF_INET TCP socket. rpc.nfsd: Created AF_INET6 TCP socket. rpc.nfsd: unable to interpret port name n
.. and I know I need to configure the ports. It also loads the module for me.
Would it make sense to have nfsctld load the nfsd module if the netlink interface doesn't exist, then on error it could also suggest checking the system log?
Please don't take this as me whining or complaining - just some observations. Most users will never encounter these problems because the tooling to start the server ensures its done the right way.. So, sorry long read -- maybe we can eventually improve nfsdctl to have better stderr on failure, or maybe the problem was mine because I didn't immediately look in the log. :P
Thanks again,
Ben
Jeff Layton
maintainerRe: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
Quoted 275 lines · Benjamin Coddington
Quoted 194 lines · Jeff Layton
Quoted 179 lines · Benjamin Coddington
Quoted 168 lines · Chuck Lever
Quoted 156 lines · Benjamin Coddington
Quoted 154 lines · Chuck Lever
Quoted 152 lines · Benjamin Coddington
Quoted 137 lines · Chuck Lever
Quoted 136 lines · Benjamin Coddington
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/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- 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(-)Documentation/netlink/specs/nfsd.yaml +6
-0@@ -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 threadsfs/nfsd/netlink.c +2
-1@@ -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 */fs/nfsd/netlink.c +1
-1@@ -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, }, {fs/nfsd/netns.h +2
-0@@ -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 */ fs/nfsd/nfsctl.c +2
-0@@ -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
staticssize_twrite_leasetime(struct file *file, char *buf,size_t
size);
staticssize_twrite_gracetime(struct file *file, char *buf,size_t
size);fs/nfsd/nfsctl.c +1
-0@@ -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,fs/nfsd/nfsctl.c +0
-0[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); }Quoted 9 lines
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, · line 960
Quoted 4 lines
fs/nfsd/nfsctl.c +4
-0+/* +* 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/nfsdAPI 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.
Quoted 6 lines
Quoted 3 lines
Neil has said he wants to keep
/proc/fs/nfsdrather 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 6 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 4 lines
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.
Mea culpa. I probably should have dropped the min-threads procfile from those patches, but it was convenient when I was doing the development work. Chuck, if you like I can send a patch to remove it before the merge window.
Quoted 4 lines
I can't see why we need both interfaces. The old /proc interface is really for the case where you have old nfs-utils and/or an old kernel. In order to use this, you need both new nfs-utils and new kernel. If you have those, then both should support the netlink interface.
I'm not trying to win an argument about how I want it, but I want to just point out one more thing: its possible to have products built out of the server where the tooling so far hasn't been taught to use nfsdctl yet. We're in that situation - we will backport the kernel bits here, and use the /proc interface because the tooling hasn't been converted to nfsdctl yet.
Quoted 18 lines
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
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.
Quoted 8 lines
Quoted 2 lines
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..
Quoted 4 lines
The main reason it would fail is because the kernel doesn't support the netlink interface (or e.g. nfsdctl isn't present at all). If it fails with the netlink interface for some other reason, it's quite likely to have the same failure with procfs.
You're right, but it also could fail for any number of other reasons - admittedly unlikely ones.
Quoted 5 lines
To be clear, the procfs interface is categorically inferior due to its piecemeal nature. There's little guidance as to how to the changes in nfsdfs should be ordered. We mostly make it work, but the cracks were showing in those interfaces long before. We really don't want to be expanding it.
I understand. I'll remove the procfs interface here on the next posting, and thanks for chiming in here.
One thing that is confusing with nfsdctl is that when it reports a failure its not easy to figure out what's going wrong. It ends up having its own ordering stuff - for example:
(fresh boot)
root@bcodding:~# nfsdctl threads 4 nfsdctl: nfsdctl started nfsdctl: failed to resolve nfsd generic netlink family nfsdctl: nfsdctl exiting root@bcodding:~# modprobe nfsd root@bcodding:~# nfsdctl threads 4 nfsdctl: nfsdctl started nfsdctl: Error: Input/output error nfsdctl: nfsdctl exiting^^ that's an actual situation I encountered, then tried to diagnose with strace (no good), then had to turn on the function tracer in the kernel to find that the error was coming from somewhere in
nfs4_state_start(). I think something not getting set (listener?), finally I looked in the log and found:[ 47.428237] NFSD: Failed to start, no listeners configured.ah - so then I knew that I had to use "autostart" before being able to use the "threads". Compared with rpc.nfsd in the same situation:
root@bcodding:~# rpc.nfsd 4 rpc.nfsd: knfsd is currently down rpc.nfsd: Writing version string to kernel: -2 +3 rpc.nfsd: Created AF_INET TCP socket. rpc.nfsd: Created AF_INET6 TCP socket. rpc.nfsd: unable to interpret port name n
Quoted 3 lines
.. and I know I need to configure the ports. It also loads the module for me.
That's a great point. The simplest fix would be to just advise the user to check dmesg for errors, but that's not ideal for containers.
In principle, we could have the kernel pass an error string back in the netlink upcall that nfsdctl could display. The problem is that nfsd startup is spread over a bunch of functions, so passing that string back from the deep call stack would take some refactoring.
In hindsight, I wish I had just added a single "service" netlink command that configured everything (listeners, versions, etc.) all at once. That would have made it a lot easier to ensure proper setup before starting things and we could more easily catch stuff like "no listeners" in userland before sending anything to the kernel.
I guess it's not too late to add one. We could just have nfsdctl fall back to the old commands if necessary. It'd be a fair bit of work though since it'd be a UAPI change.
Quoted 4 lines
Would it make sense to have nfsctld load the nfsd module if the netlink interface doesn't exist, then on error it could also suggest checking the system log?
That'd be a great improvement.
Quoted 6 lines
Please don't take this as me whining or complaining - just some observations. Most users will never encounter these problems because the tooling to start the server ensures its done the right way.. So, sorry long read -- maybe we can eventually improve nfsdctl to have better stderr on failure, or maybe the problem was mine because I didn't immediately look in the log. :P
Not at all! These are great observations.
Most users don't do anything but "autostart" and "threads 0" via systemd, but it definitely has rough edges if you're doing things manually. Chuck and I would welcome improvements.
Signature · 1 line
Jeff Layton <[email protected]>
Chuck Lever
maintainerRe: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
In reply to Jeff LaytonQuoted 8 lines · Jeff Layton
Quoted 7 lines · Benjamin Coddington
Quoted 4 lines · Chuck Lever
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.
Quoted 8 lines
Quoted 3 lines
You've just added one to your nfsd-testing branch two weeks ago that you asked me to rebase onto.
Mea culpa. I probably should have dropped the min-threads procfile from those patches, but it was convenient when I was doing the development work. Chuck, if you like I can send a patch to remove it before the merge window.
Send a patch. I can still squash such a change into the queued patch set.
Quoted 4 lines
I can't see why we need both interfaces. The old /proc interface is really for the case where you have old nfs-utils and/or an old kernel. In order to use this, you need both new nfs-utils and new kernel. If you have those, then both should support the netlink interface.
This is exactly my assessment so far. But I'm open to hearing a rationale for adding a procfs API
Signature · 1 line
Chuck Lever
Chuck Lever
maintainerRe: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
In reply to Benjamin CoddingtonQuoted 170 lines · Benjamin Coddington
Quoted 168 lines · Chuck Lever
Quoted 156 lines · Benjamin Coddington
Quoted 154 lines · Chuck Lever
Quoted 152 lines · Benjamin Coddington
Quoted 137 lines · Chuck Lever
Quoted 136 lines · Benjamin Coddington
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/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- 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(-)Documentation/netlink/specs/nfsd.yaml +6
-0@@ -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 threadsfs/nfsd/netlink.c +2
-1@@ -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 */fs/nfsd/netlink.c +1
-1@@ -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, }, {fs/nfsd/netns.h +2
-0@@ -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 */ fs/nfsd/nfsctl.c +2
-0@@ -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
staticssize_twrite_leasetime(struct file *file, char *buf,size_t
size);
staticssize_twrite_gracetime(struct file *file, char *buf,size_t
size);fs/nfsd/nfsctl.c +1
-0@@ -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,fs/nfsd/nfsctl.c +0
-0[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); }Quoted 9 lines
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, · line 960
Quoted 4 lines
fs/nfsd/nfsctl.c +4
-0+/* +* 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/nfsdAPI 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.
Quoted 6 lines
Quoted 3 lines
Neil has said he wants to keep
/proc/fs/nfsdrather 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 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
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).
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. Don't argue with me. Just answer my questions. All I'm asking here is why are you adding it.
Quoted 7 lines
Quoted 4 lines
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".
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?
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 2 lines
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 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.
Signature · 1 line
Chuck Lever
Re: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
Quoted 176 lines · Chuck Lever
Quoted 175 lines · Benjamin Coddington
Quoted 173 lines · Chuck Lever
Quoted 156 lines · Benjamin Coddington
Quoted 154 lines · Chuck Lever
Quoted 152 lines · Benjamin Coddington
Quoted 137 lines · Chuck Lever
Quoted 136 lines · Benjamin Coddington
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/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- 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(-)Documentation/netlink/specs/nfsd.yaml +6
-0@@ -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 threadsfs/nfsd/netlink.c +2
-1@@ -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 */fs/nfsd/netlink.c +1
-1@@ -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, }, {fs/nfsd/netns.h +2
-0@@ -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 */ fs/nfsd/nfsctl.c +2
-0@@ -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
staticssize_twrite_leasetime(struct file *file, char *buf,size_t
size);
staticssize_twrite_gracetime(struct file *file, char *buf,size_t
size);fs/nfsd/nfsctl.c +1
-0@@ -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,fs/nfsd/nfsctl.c +0
-0[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); }Quoted 9 lines
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, · line 960
Quoted 4 lines
fs/nfsd/nfsctl.c +4
-0+/* +* 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/nfsdAPI 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.
Quoted 6 lines
Quoted 3 lines
Neil has said he wants to keep
/proc/fs/nfsdrather 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 4 lines
Quoted 2 lines
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?
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
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.
Quoted 3 lines
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.
Quoted 1 line
Don't argue with me.
Got it - I'm really sad to read this.
Quoted 1 line
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 5 lines
Quoted 5 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 2 lines
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.
Quoted 3 lines
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 17 lines
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 2 lines
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..
Quoted 5 lines
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.
Quoted 7 lines
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
Re: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
Quoted 11 lines · Benjamin Coddington
Quoted 9 lines · Chuck Lever
Quoted 8 lines · Benjamin Coddington
Quoted 5 lines · Chuck Lever
Quoted 2 lines · Benjamin Coddington
Why only netlink for this one besides your preference?
You might be channeling one of your kids there.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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.
For the record I agree with Ben. It was reasonable question for Ben to ask, and an unhelpful and unprofessional response from Chuck.
NeilBrown
Jeff Layton
maintainerRe: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
In reply to Benjamin CoddingtonQuoted 8 lines · Benjamin Coddington
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.
This sounds like crazy-pants talk.
Why do we need to unload nfsd.ko to change the key? Also, what will you do about folks who don't build nfsd as a module?
Personally, I think just disallowing key changes while the nfs server is running should be sufficient. If someone wants to shut down the threads and then change the key on the next startup, then I don't see why that shouldn't be allowed.
We'll want to document _why_ you generally wouldn't want to do that, but in the case of a compromised key, it might be necessary.
Signature · 1 line
Jeff Layton <[email protected]>
Re: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
Quoted 12 lines · Jeff Layton
Quoted 8 lines · Benjamin Coddington
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.
This sounds like crazy-pants talk.
Quoted 7 lines
Why do we need to unload nfsd.ko to change the key? Also, what will you do about folks who don't build nfsd as a module?
Personally, I think just disallowing key changes while the nfs server is running should be sufficient. If someone wants to shut down the threads and then change the key on the next startup, then I don't see why that shouldn't be allowed.
Sounds good. Chuck are you alright with this?
Quoted 2 lines
We'll want to document _why_ you generally wouldn't want to do that, but in the case of a compromised key, it might be necessary.
Ben
Chuck Lever
maintainerRe: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
Quoted 17 lines · Benjamin Coddington
Quoted 15 lines · Jeff Layton
Quoted 8 lines · Benjamin Coddington
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.
This sounds like crazy-pants talk.
Why do we need to unload nfsd.ko to change the key? Also, what will you do about folks who don't build nfsd as a module?
Quoted 6 lines
Quoted 4 lines
Personally, I think just disallowing key changes while the nfs server is running should be sufficient. If someone wants to shut down the threads and then change the key on the next startup, then I don't see why that shouldn't be allowed.
Sounds good. Chuck are you alright with this?
I hadn't thought of the "nfsd.ko is built in" case. Oops.
Yes, I think it's fine to keep the fh_key value locked only while the server is running, much like most of the other NFSD settings. It might be the best we can reasonably do, and it will certainly be easier to document!
Signature · 1 line
Chuck Lever
Re: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
In reply to Benjamin Coddingtonfs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, char *buf, size_t size) · line 1000
Quoted 7 lines · Benjamin Coddington
fs/nfsd/nfsctl.c +7
-0+ sip_fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!sip_fh_key) { + ret = -ENOMEM ; + goto out; + } + +memcpy (sip_fh_key, &uuid_fh_key,sizeof (siphash_key_t));
Note that siphash_key_t consists of a pair of native-endian u64's:
typedef struct {
u64 key[2];
} siphash_key_t;If you copy a byte array into it, the result will differ on little endian vs. big endian CPUs.
You may want to do le64_to_cpus() on each u64, like what fscrypt_derive_siphash_key() does.
- Eric
Re: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
Quoted 15 lines · Eric Biggers
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, char *buf, size_t size) · line 1000
Quoted 7 lines · Benjamin Coddington
fs/nfsd/nfsctl.c +7
-0+ sip_fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!sip_fh_key) { + ret = -ENOMEM ; + goto out; + } + +memcpy (sip_fh_key, &uuid_fh_key,sizeof (siphash_key_t));Note that
siphash_key_tconsists of a pair of native-endian u64's:typedef struct { u64 key[2]; } siphash_key_t;
Quoted 5 lines
If you copy a byte array into it, the result will differ on little endian vs. big endian CPUs.
You may want to do
le64_to_cpus()on each u64, like whatfscrypt_derive_siphash_key()does.
Great catch - thanks Eric.
Ben
PATCH v2 2/3 NFSD/export: Add sign_fh export option
In reply to Benjamin CoddingtonIn order to signal that filehandles on this export should be signed, add a "sign_fh" export option. Filehandle signing can help the server defend against certain filehandle guessing attacks.
Setting the "sign_fh" export option sets NFSEXP_SIGN_FH. In a future patch NFSD uses this signal to append a MAC onto filehandles for that export.
While we're in here, tidy a few stray expflags to more closely align to the export flag order.
--- fs/nfsd/export.c | 5 +++-- include/uapi/linux/nfsd/export.h | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-)
fs/nfsd/export.c +3 -2
@@ -1349,13 +1349,14 @@ static struct flags { { NFSEXP_ASYNC, {"async", "sync"}}, { NFSEXP_GATHERED_WRITES, {"wdelay", "no_wdelay"}}, { NFSEXP_NOREADDIRPLUS, {"nordirplus", ""}}, + { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, + { NFSEXP_SIGN_FH, {"sign_fh", ""}}, { NFSEXP_NOHIDE, {"nohide", ""}}, - { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_NOSUBTREECHECK, {"no_subtree_check", ""}}, { NFSEXP_NOAUTHNLM, {"insecure_locks", ""}}, + { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_V4ROOT, {"v4root", ""}}, { NFSEXP_PNFS, {"pnfs", ""}}, - { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, { 0, {"", ""}} };
include/uapi/linux/nfsd/export.h +2 -2
@@ -34,7 +34,7 @@#define NFSEXP_GATHERED_WRITES 0x0020#define NFSEXP_NOREADDIRPLUS 0x0040#define NFSEXP_SECURITY_LABEL 0x0080 -/* 0x100 currently unused */ +#define NFSEXP_SIGN_FH 0x0100#define NFSEXP_NOHIDE 0x0200#define NFSEXP_NOSUBTREECHECK 0x0400#define NFSEXP_NOAUTHNLM 0x0800/* Don't authenticate NLM requests - just trust */
@@ -55,7 +55,7 @@#define NFSEXP_PNFS 0x20000/* All flags that we claim to support. (Note we don't support NOACL.) */ -#define NFSEXP_ALLFLAGS 0x3FEFF +#define NFSEXP_ALLFLAGS 0x3FFFF/* The flags that may vary depending on security flavor: */ #define NFSEXP_SECINFO_FLAGS (NFSEXP_READONLY | NFSEXP_ROOTSQUASH \
Signature · 1 line
2.50.1
Jeff Layton
maintainerRe: [PATCH v2 2/3] NFSD/export: Add sign_fh export option
Quoted 52 lines · Benjamin Coddington
In order to signal that filehandles on this export should be signed, add a "
sign_fh" export option. Filehandle signing can help the server defend against certain filehandle guessing attacks.Setting the "
sign_fh" export option setsNFSEXP_SIGN_FH. In a future patch NFSD uses this signal to append a MAC onto filehandles for that export.While we're in here, tidy a few stray expflags to more closely align to the export flag order.
Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/export.c | 5 +++-- include/uapi/linux/nfsd/export.h | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-)fs/nfsd/export.c +3
-2@@ -1349,13 +1349,14 @@ static struct flags { { NFSEXP_ASYNC, {"async", "sync"}}, { NFSEXP_GATHERED_WRITES, {"wdelay", "no_wdelay"}}, { NFSEXP_NOREADDIRPLUS, {"nordirplus", ""}}, + { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, + { NFSEXP_SIGN_FH, {"sign_fh", ""}}, { NFSEXP_NOHIDE, {"nohide", ""}}, - { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_NOSUBTREECHECK, {"no_subtree_check", ""}}, { NFSEXP_NOAUTHNLM, {"insecure_locks", ""}}, + { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_V4ROOT, {"v4root", ""}}, { NFSEXP_PNFS, {"pnfs", ""}}, - { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, { 0, {"", ""}} };include/uapi/linux/nfsd/export.h +1
-1@@ -34,7 +34,7 @@#define NFSEXP_GATHERED_WRITES 0x0020#define NFSEXP_NOREADDIRPLUS 0x0040#define NFSEXP_SECURITY_LABEL 0x0080 -/* 0x100 currently unused */ +#define NFSEXP_SIGN_FH 0x0100#define NFSEXP_NOHIDE 0x0200#define NFSEXP_NOSUBTREECHECK 0x0400#define NFSEXP_NOAUTHNLM 0x0800/* Don't authenticate NLM requests - just trust */
include/uapi/linux/nfsd/export.h line 61
Quoted 8 lines
include/uapi/linux/nfsd/export.h +1
-1@@ -55,7 +55,7 @@#define NFSEXP_PNFS 0x20000/* All flags that we claim to support. (Note we don't support NOACL.) */ -#define NFSEXP_ALLFLAGS 0x3FEFF +#define NFSEXP_ALLFLAGS 0x3FFFF/* The flags that may vary depending on security flavor: */ #define NFSEXP_SECINFO_FLAGS (NFSEXP_READONLY | NFSEXP_ROOTSQUASH \
One thing that needs to be understood and documented is how things will behave when this flag changes. For instance:
Support we start with sign_fh enabled, and client gets a signed filehandle. The server then reboots and the export options change such that sign_fh is disabled. What happens when the client tries to present that fh to the server? Does it ignore the signature (since sign_fh is now disabled), or does it reject the filehandle because it's not expecting a signature?
Signature · 1 line
Jeff Layton <[email protected]>
Re: [PATCH v2 2/3] NFSD/export: Add sign_fh export option
Quoted 65 lines · Jeff Layton
Quoted 52 lines · Benjamin Coddington
In order to signal that filehandles on this export should be signed, add a "
sign_fh" export option. Filehandle signing can help the server defend against certain filehandle guessing attacks.Setting the "
sign_fh" export option setsNFSEXP_SIGN_FH. In a future patch NFSD uses this signal to append a MAC onto filehandles for that export.While we're in here, tidy a few stray expflags to more closely align to the export flag order.
Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/export.c | 5 +++-- include/uapi/linux/nfsd/export.h | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-)fs/nfsd/export.c +3
-2@@ -1349,13 +1349,14 @@ static struct flags { { NFSEXP_ASYNC, {"async", "sync"}}, { NFSEXP_GATHERED_WRITES, {"wdelay", "no_wdelay"}}, { NFSEXP_NOREADDIRPLUS, {"nordirplus", ""}}, + { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, + { NFSEXP_SIGN_FH, {"sign_fh", ""}}, { NFSEXP_NOHIDE, {"nohide", ""}}, - { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_NOSUBTREECHECK, {"no_subtree_check", ""}}, { NFSEXP_NOAUTHNLM, {"insecure_locks", ""}}, + { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_V4ROOT, {"v4root", ""}}, { NFSEXP_PNFS, {"pnfs", ""}}, - { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, { 0, {"", ""}} };include/uapi/linux/nfsd/export.h +1
-1@@ -34,7 +34,7 @@#define NFSEXP_GATHERED_WRITES 0x0020#define NFSEXP_NOREADDIRPLUS 0x0040#define NFSEXP_SECURITY_LABEL 0x0080 -/* 0x100 currently unused */ +#define NFSEXP_SIGN_FH 0x0100#define NFSEXP_NOHIDE 0x0200#define NFSEXP_NOSUBTREECHECK 0x0400#define NFSEXP_NOAUTHNLM 0x0800/* Don't authenticate NLM requests - just trust */ include/uapi/linux/nfsd/export.h line 61
Quoted 8 lines
include/uapi/linux/nfsd/export.h +1
-1@@ -55,7 +55,7 @@#define NFSEXP_PNFS 0x20000/* All flags that we claim to support. (Note we don't support NOACL.) */ -#define NFSEXP_ALLFLAGS 0x3FEFF +#define NFSEXP_ALLFLAGS 0x3FFFF/* The flags that may vary depending on security flavor: */ #define NFSEXP_SECINFO_FLAGS (NFSEXP_READONLY | NFSEXP_ROOTSQUASH \One thing that needs to be understood and documented is how things will behave when this flag changes. For instance:
Quoted 6 lines
Support we start with
sign_fhenabled, and client gets a signed filehandle. The server then reboots and the export options change such thatsign_fhis disabled. What happens when the client tries to present that fh to the server? Does it ignore the signature (sincesign_fhis now disabled), or does it reject the filehandle because it's not expecting a signature?
That's great question - right now it will first look up the export, see that NFSEXP_SIGN_FH is not set, then bypass verifying (and truncating) the MAC from the end of the filehadle before sending the filehandle off to exportfs
- the end result will be will be -ESTALE.
Would it be a good idea to allow the server to see that the filehandle has FH_AT_MAC set, and just trim off the MAC without verifying it? That would allow the signed fh to still function on that export.
Might need to audit the cases where fh_match() is used in that case, or make fh_match() signed-aware. I'm less familiar with those cases, but I can look into them.
Ben
Jeff Layton
maintainerRe: [PATCH v2 2/3] NFSD/export: Add sign_fh export option
Quoted 79 lines · Benjamin Coddington
Quoted 65 lines · Jeff Layton
Quoted 52 lines · Benjamin Coddington
In order to signal that filehandles on this export should be signed, add a "
sign_fh" export option. Filehandle signing can help the server defend against certain filehandle guessing attacks.Setting the "
sign_fh" export option setsNFSEXP_SIGN_FH. In a future patch NFSD uses this signal to append a MAC onto filehandles for that export.While we're in here, tidy a few stray expflags to more closely align to the export flag order.
Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/export.c | 5 +++-- include/uapi/linux/nfsd/export.h | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-)fs/nfsd/export.c +3
-2@@ -1349,13 +1349,14 @@ static struct flags { { NFSEXP_ASYNC, {"async", "sync"}}, { NFSEXP_GATHERED_WRITES, {"wdelay", "no_wdelay"}}, { NFSEXP_NOREADDIRPLUS, {"nordirplus", ""}}, + { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, + { NFSEXP_SIGN_FH, {"sign_fh", ""}}, { NFSEXP_NOHIDE, {"nohide", ""}}, - { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_NOSUBTREECHECK, {"no_subtree_check", ""}}, { NFSEXP_NOAUTHNLM, {"insecure_locks", ""}}, + { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_V4ROOT, {"v4root", ""}}, { NFSEXP_PNFS, {"pnfs", ""}}, - { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, { 0, {"", ""}} };include/uapi/linux/nfsd/export.h +1
-1@@ -34,7 +34,7 @@#define NFSEXP_GATHERED_WRITES 0x0020#define NFSEXP_NOREADDIRPLUS 0x0040#define NFSEXP_SECURITY_LABEL 0x0080 -/* 0x100 currently unused */ +#define NFSEXP_SIGN_FH 0x0100#define NFSEXP_NOHIDE 0x0200#define NFSEXP_NOSUBTREECHECK 0x0400#define NFSEXP_NOAUTHNLM 0x0800/* Don't authenticate NLM requests - just trust */ include/uapi/linux/nfsd/export.h line 61
Quoted 8 lines
include/uapi/linux/nfsd/export.h +1
-1@@ -55,7 +55,7 @@#define NFSEXP_PNFS 0x20000/* All flags that we claim to support. (Note we don't support NOACL.) */ -#define NFSEXP_ALLFLAGS 0x3FEFF +#define NFSEXP_ALLFLAGS 0x3FFFF/* The flags that may vary depending on security flavor: */ #define NFSEXP_SECINFO_FLAGS (NFSEXP_READONLY | NFSEXP_ROOTSQUASH \One thing that needs to be understood and documented is how things will behave when this flag changes. For instance:
Quoted 6 lines
Support we start with
sign_fhenabled, and client gets a signed filehandle. The server then reboots and the export options change such thatsign_fhis disabled. What happens when the client tries to present that fh to the server? Does it ignore the signature (sincesign_fhis now disabled), or does it reject the filehandle because it's not expecting a signature?That's great question - right now it will first look up the export, see that
NFSEXP_SIGN_FHis not set, then bypass verifying (and truncating) the MAC from the end of the filehadle before sending the filehandle off to exportfs
- the end result will be will be-ESTALE.
Quoted 8 lines
Would it be a good idea to allow the server to see that the filehandle has
FH_AT_MACset, and just trim off the MAC without verifying it? That would allow the signed fh to still function on that export.Might need to audit the cases where
fh_match()is used in that case, or makefh_match()signed-aware. I'm less familiar with those cases, but I can look into them.
No, I think -ESTALE is fine in this situation. I don't think we need to go to any great lengths to make this scenario actually work. We just need to understand what happens if it does, and make sure that it's documented.
Cheers,
Signature · 1 line
Jeff Layton <[email protected]>
Re: [PATCH v2 2/3] NFSD/export: Add sign_fh export option
Quoted 89 lines · Jeff Layton
Quoted 79 lines · Benjamin Coddington
Quoted 65 lines · Jeff Layton
Quoted 52 lines · Benjamin Coddington
In order to signal that filehandles on this export should be signed, add a "
sign_fh" export option. Filehandle signing can help the server defend against certain filehandle guessing attacks.Setting the "
sign_fh" export option setsNFSEXP_SIGN_FH. In a future patch NFSD uses this signal to append a MAC onto filehandles for that export.While we're in here, tidy a few stray expflags to more closely align to the export flag order.
Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/export.c | 5 +++-- include/uapi/linux/nfsd/export.h | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-)fs/nfsd/export.c +3
-2@@ -1349,13 +1349,14 @@ static struct flags { { NFSEXP_ASYNC, {"async", "sync"}}, { NFSEXP_GATHERED_WRITES, {"wdelay", "no_wdelay"}}, { NFSEXP_NOREADDIRPLUS, {"nordirplus", ""}}, + { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, + { NFSEXP_SIGN_FH, {"sign_fh", ""}}, { NFSEXP_NOHIDE, {"nohide", ""}}, - { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_NOSUBTREECHECK, {"no_subtree_check", ""}}, { NFSEXP_NOAUTHNLM, {"insecure_locks", ""}}, + { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_V4ROOT, {"v4root", ""}}, { NFSEXP_PNFS, {"pnfs", ""}}, - { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, { 0, {"", ""}} };include/uapi/linux/nfsd/export.h +1
-1@@ -34,7 +34,7 @@#define NFSEXP_GATHERED_WRITES 0x0020#define NFSEXP_NOREADDIRPLUS 0x0040#define NFSEXP_SECURITY_LABEL 0x0080 -/* 0x100 currently unused */ +#define NFSEXP_SIGN_FH 0x0100#define NFSEXP_NOHIDE 0x0200#define NFSEXP_NOSUBTREECHECK 0x0400#define NFSEXP_NOAUTHNLM 0x0800/* Don't authenticate NLM requests - just trust */ include/uapi/linux/nfsd/export.h line 61
Quoted 8 lines
include/uapi/linux/nfsd/export.h +1
-1@@ -55,7 +55,7 @@#define NFSEXP_PNFS 0x20000/* All flags that we claim to support. (Note we don't support NOACL.) */ -#define NFSEXP_ALLFLAGS 0x3FEFF +#define NFSEXP_ALLFLAGS 0x3FFFF/* The flags that may vary depending on security flavor: */ #define NFSEXP_SECINFO_FLAGS (NFSEXP_READONLY | NFSEXP_ROOTSQUASH \One thing that needs to be understood and documented is how things will behave when this flag changes. For instance:
Quoted 6 lines
Support we start with
sign_fhenabled, and client gets a signed filehandle. The server then reboots and the export options change such thatsign_fhis disabled. What happens when the client tries to present that fh to the server? Does it ignore the signature (sincesign_fhis now disabled), or does it reject the filehandle because it's not expecting a signature?That's great question - right now it will first look up the export, see that
NFSEXP_SIGN_FHis not set, then bypass verifying (and truncating) the MAC from the end of the filehadle before sending the filehandle off to exportfs
- the end result will be will be-ESTALE.Quoted 8 lines
Would it be a good idea to allow the server to see that the filehandle has
FH_AT_MACset, and just trim off the MAC without verifying it? That would allow the signed fh to still function on that export.Might need to audit the cases where
fh_match()is used in that case, or makefh_match()signed-aware. I'm less familiar with those cases, but I can look into them.
Quoted 4 lines
No, I think
-ESTALEis fine in this situation. I don't think we need to go to any great lengths to make this scenario actually work. We just need to understand what happens if it does, and make sure that it's documented.
Got it - I will document this behavior in the commit message of the last patch on the next posting. I think I'll probably also add a check for this case -- no need to send the filehandle off to the filesystems if we know its going to fail to resolve to a dentry.
Thanks,
Ben
Jeff Layton
maintainerRe: [PATCH v2 2/3] NFSD/export: Add sign_fh export option
Quoted 96 lines · Benjamin Coddington
Quoted 89 lines · Jeff Layton
Quoted 79 lines · Benjamin Coddington
Quoted 65 lines · Jeff Layton
Quoted 52 lines · Benjamin Coddington
In order to signal that filehandles on this export should be signed, add a "
sign_fh" export option. Filehandle signing can help the server defend against certain filehandle guessing attacks.Setting the "
sign_fh" export option setsNFSEXP_SIGN_FH. In a future patch NFSD uses this signal to append a MAC onto filehandles for that export.While we're in here, tidy a few stray expflags to more closely align to the export flag order.
Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/export.c | 5 +++-- include/uapi/linux/nfsd/export.h | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-)fs/nfsd/export.c +3
-2@@ -1349,13 +1349,14 @@ static struct flags { { NFSEXP_ASYNC, {"async", "sync"}}, { NFSEXP_GATHERED_WRITES, {"wdelay", "no_wdelay"}}, { NFSEXP_NOREADDIRPLUS, {"nordirplus", ""}}, + { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, + { NFSEXP_SIGN_FH, {"sign_fh", ""}}, { NFSEXP_NOHIDE, {"nohide", ""}}, - { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_NOSUBTREECHECK, {"no_subtree_check", ""}}, { NFSEXP_NOAUTHNLM, {"insecure_locks", ""}}, + { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_V4ROOT, {"v4root", ""}}, { NFSEXP_PNFS, {"pnfs", ""}}, - { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, { 0, {"", ""}} };include/uapi/linux/nfsd/export.h +1
-1@@ -34,7 +34,7 @@#define NFSEXP_GATHERED_WRITES 0x0020#define NFSEXP_NOREADDIRPLUS 0x0040#define NFSEXP_SECURITY_LABEL 0x0080 -/* 0x100 currently unused */ +#define NFSEXP_SIGN_FH 0x0100#define NFSEXP_NOHIDE 0x0200#define NFSEXP_NOSUBTREECHECK 0x0400#define NFSEXP_NOAUTHNLM 0x0800/* Don't authenticate NLM requests - just trust */ include/uapi/linux/nfsd/export.h line 61
Quoted 8 lines
include/uapi/linux/nfsd/export.h +1
-1@@ -55,7 +55,7 @@#define NFSEXP_PNFS 0x20000/* All flags that we claim to support. (Note we don't support NOACL.) */ -#define NFSEXP_ALLFLAGS 0x3FEFF +#define NFSEXP_ALLFLAGS 0x3FFFF/* The flags that may vary depending on security flavor: */ #define NFSEXP_SECINFO_FLAGS (NFSEXP_READONLY | NFSEXP_ROOTSQUASH \One thing that needs to be understood and documented is how things will behave when this flag changes. For instance:
Quoted 6 lines
Support we start with
sign_fhenabled, and client gets a signed filehandle. The server then reboots and the export options change such thatsign_fhis disabled. What happens when the client tries to present that fh to the server? Does it ignore the signature (sincesign_fhis now disabled), or does it reject the filehandle because it's not expecting a signature?That's great question - right now it will first look up the export, see that
NFSEXP_SIGN_FHis not set, then bypass verifying (and truncating) the MAC from the end of the filehadle before sending the filehandle off to exportfs
- the end result will be will be-ESTALE.Quoted 8 lines
Would it be a good idea to allow the server to see that the filehandle has
FH_AT_MACset, and just trim off the MAC without verifying it? That would allow the signed fh to still function on that export.Might need to audit the cases where
fh_match()is used in that case, or makefh_match()signed-aware. I'm less familiar with those cases, but I can look into them.Quoted 4 lines
No, I think
-ESTALEis fine in this situation. I don't think we need to go to any great lengths to make this scenario actually work. We just need to understand what happens if it does, and make sure that it's documented.
Quoted 5 lines
Got it - I will document this behavior in the commit message of the last patch on the next posting. I think I'll probably also add a check for this case -- no need to send the filehandle off to the filesystems if we know its going to fail to resolve to a dentry.
Consider adding a new section in Documentation/filesystems/nfs/exporting.rst. Commit messages will be harder to dig out years from now.
Signature · 1 line
Jeff Layton <[email protected]>
Chuck Lever
maintainerRe: [PATCH v2 2/3] NFSD/export: Add sign_fh export option
In reply to Jeff LaytonQuoted 84 lines · Jeff Layton
Quoted 83 lines · Benjamin Coddington
Quoted 65 lines · Jeff Layton
Quoted 52 lines · Benjamin Coddington
In order to signal that filehandles on this export should be signed, add a "
sign_fh" export option. Filehandle signing can help the server defend against certain filehandle guessing attacks.Setting the "
sign_fh" export option setsNFSEXP_SIGN_FH. In a future patch NFSD uses this signal to append a MAC onto filehandles for that export.While we're in here, tidy a few stray expflags to more closely align to the export flag order.
Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/export.c | 5 +++-- include/uapi/linux/nfsd/export.h | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-)fs/nfsd/export.c +3
-2@@ -1349,13 +1349,14 @@ static struct flags { { NFSEXP_ASYNC, {"async", "sync"}}, { NFSEXP_GATHERED_WRITES, {"wdelay", "no_wdelay"}}, { NFSEXP_NOREADDIRPLUS, {"nordirplus", ""}}, + { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, + { NFSEXP_SIGN_FH, {"sign_fh", ""}}, { NFSEXP_NOHIDE, {"nohide", ""}}, - { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_NOSUBTREECHECK, {"no_subtree_check", ""}}, { NFSEXP_NOAUTHNLM, {"insecure_locks", ""}}, + { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_V4ROOT, {"v4root", ""}}, { NFSEXP_PNFS, {"pnfs", ""}}, - { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, { 0, {"", ""}} };include/uapi/linux/nfsd/export.h +1
-1@@ -34,7 +34,7 @@#define NFSEXP_GATHERED_WRITES 0x0020#define NFSEXP_NOREADDIRPLUS 0x0040#define NFSEXP_SECURITY_LABEL 0x0080 -/* 0x100 currently unused */ +#define NFSEXP_SIGN_FH 0x0100#define NFSEXP_NOHIDE 0x0200#define NFSEXP_NOSUBTREECHECK 0x0400#define NFSEXP_NOAUTHNLM 0x0800/* Don't authenticate NLM requests - just trust */ include/uapi/linux/nfsd/export.h line 61
Quoted 8 lines
include/uapi/linux/nfsd/export.h +1
-1@@ -55,7 +55,7 @@#define NFSEXP_PNFS 0x20000/* All flags that we claim to support. (Note we don't support NOACL.) */ -#define NFSEXP_ALLFLAGS 0x3FEFF +#define NFSEXP_ALLFLAGS 0x3FFFF/* The flags that may vary depending on security flavor: */ #define NFSEXP_SECINFO_FLAGS (NFSEXP_READONLY | NFSEXP_ROOTSQUASH \One thing that needs to be understood and documented is how things will behave when this flag changes. For instance:
Quoted 6 lines
Support we start with
sign_fhenabled, and client gets a signed filehandle. The server then reboots and the export options change such thatsign_fhis disabled. What happens when the client tries to present that fh to the server? Does it ignore the signature (sincesign_fhis now disabled), or does it reject the filehandle because it's not expecting a signature?That's great question - right now it will first look up the export, see that
NFSEXP_SIGN_FHis not set, then bypass verifying (and truncating) the MAC from the end of the filehadle before sending the filehandle off to exportfs
- the end result will be will be-ESTALE.Would it be a good idea to allow the server to see that the filehandle has
FH_AT_MACset, and just trim off the MAC without verifying it? That would allow the signed fh to still function on that export.
Quoted 6 lines
Quoted 4 lines
Might need to audit the cases where
fh_match()is used in that case, or makefh_match()signed-aware. I'm less familiar with those cases, but I can look into them.No, I think
-ESTALEis fine in this situation.
I agree that NFS[34]ERR_STALE is the correct server response when a client presents a file handle that does not pass the configured security policy (ie, unsigned when NFSD wants signed, signed when NFSD doesn't, or MAC that does not match the FH).
Quoted 4 lines
I don't think we need to
go to any great lengths to make this scenario actually work. We just need to understand what happens if it does, and make sure that it's documented.
The documentation for "changing from signed to unsigned and back" goes along with what happens when the fh_key needs to change, I would think.
And, IMHO we also want to craft some pynfs tests to ensure that these cases work as we expect, and to ensure that we capture all of the cases that need to be checked (including the cases involving transitioning from sign_fh to not sign_fh and vice versa).
Basically, exploring the corner cases to look for regressions.
I've had good luck constructing such unit tests with Claude Code, fwiw.
Signature · 1 line
Chuck Lever
PATCH v2 3/3 NFSD: Sign filehandles
In reply to Benjamin CoddingtonNFS clients may bypass restrictive directory permissions by using open_by_handle() (or other available OS system call) to guess the filehandles for files below that directory.
In order to harden knfsd servers against this attack, create a method to sign and verify filehandles using siphash as a MAC (Message Authentication Code). Filehandles that have been signed cannot be tampered with, nor can clients reasonably guess correct filehandles and hashes that may exist in parts of the filesystem they cannot access due to directory permissions.
Append the 8 byte siphash to encoded filehandles for exports that have set the "sign_fh" export option. The filehandle's fh_auth_type is set to FH_AT_MAC(1) to indicate the filehandle is signed. Filehandles received from clients are verified by comparing the appended hash to the expected hash. If the MAC does not match the server responds with NFS error _BADHANDLE. If unsigned filehandles are received for an export with "sign_fh" they are rejected with NFS error _BADHANDLE.
--- fs/nfsd/nfsfh.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++-- fs/nfsd/nfsfh.h | 3 ++ 2 files changed, 73 insertions(+), 3 deletions(-)
fs/nfsd/nfsfh.c +70 -3
@@ -11,6 +11,7 @@#include <linux/exportfs.h> #include <linux/sunrpc/svcauth_gss.h> +#include <crypto/utils.h> #include "nfsd.h" #include "vfs.h" #include "auth.h"
@@ -137,6 +138,61 @@ static inline __be32 check_pseudo_root(struct dentry *dentry,return nfs_ok; } +/* +* Append an 8-byte MAC to the filehandle hashed from the server's fh_key: +*/ +static int fh_append_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (!(fhp->fh_export->ex_flags &NFSEXP_SIGN_FH )) +return 0 ; + +if (!fh_key) { +pr_warn_ratelimited ("NFSD: unable to sign filehandles, fh_key not set.\n" ); +return -EINVAL ; + } + +if (fh->fh_size +sizeof (hash) > fhp->fh_maxsize) { +pr_warn_ratelimited ("NFSD: unable to sign filehandles, fh_size %d would be greater" +" than fh_maxsize %d.\n" , (int )(fh->fh_size +sizeof (hash)), fhp->fh_maxsize); +return -EINVAL ; + } + + fh->fh_auth_type =FH_AT_MAC ; + hash =siphash (&fh->fh_raw, fh->fh_size, fh_key); +memcpy (&fh->fh_raw[fh->fh_size], &hash,sizeof (hash)); + fh->fh_size +=sizeof (hash); + +return 0 ; +} + +/* +* Verify that the the filehandle's MAC was hashed from this filehandle +* given the server's fh_key: +*/ +static int fh_verify_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (fhp->fh_handle.fh_auth_type !=FH_AT_MAC ) +return -EINVAL ; + +if (!fh_key) { +pr_warn_ratelimited ("NFSD: unable to verify signed filehandles, fh_key not set.\n" ); +return -EINVAL ; + } + + hash =siphash (&fh->fh_raw, fh->fh_size -sizeof (hash), fh_key); +return crypto_memneq (&fh->fh_raw[fh->fh_size -sizeof (hash)], &hash,sizeof (hash)); +} +/* * Use the given filehandle to look up the corresponding export and * dentry. On success, the results are used to set fh_export and
@@ -166,8 +222,11 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net,if (--data_left <0 )return error; -if (fh->fh_auth_type !=0 ) + +/* either FH_AT_NONE or FH_AT_MAC */ +if (fh->fh_auth_type >1 )return error; + len =key_len (fh->fh_fsid_type) /4 ;if (len ==0 )return error;
@@ -237,9 +296,14 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, fileid_type = fh->fh_fileid_type; -if (fileid_type ==FILEID_ROOT ) +if (fileid_type ==FILEID_ROOT ) { dentry =dget (exp->ex_path.dentry); -else { + }else { +if (exp->ex_flags &NFSEXP_SIGN_FH &&fh_verify_mac (fhp, net)) { +trace_nfsd_set_fh_dentry_badhandle (rqstp, fhp, -EKEYREJECTED ); + goto out; + } + dentry =exportfs_decode_fh_raw (exp->ex_path.mnt, fid, data_left, fileid_type,0 , nfsd_acceptable, exp);
@@ -495,6 +559,9 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp, fhp->fh_handle.fh_fileid_type = fileid_type >0 ? fileid_type :FILEID_INVALID ; fhp->fh_handle.fh_size += maxsize *4 ; + +if (fh_append_mac (fhp, exp->cd->net)) + fhp->fh_handle.fh_fileid_type =FILEID_INVALID ; }else { fhp->fh_handle.fh_fileid_type =FILEID_ROOT ; }
fs/nfsd/nfsfh.h +3 -0
@@ -59,6 +59,9 @@ struct knfsd_fh {#define fh_fsid_type fh_raw[2]#define fh_fileid_type fh_raw[3] +#define FH_AT_NONE 0 +#define FH_AT_MAC 1 +static inline u32 *fh_fsid (const struct knfsd_fh *fh) {return (u32 *)&fh->fh_raw[4 ];
Signature · 1 line
2.50.1
Chuck Lever
maintainerRe: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 155 lines · Benjamin Coddington
NFS clients may bypass restrictive directory permissions by using
open_by_handle()(or other available OS system call) to guess the filehandles for files below that directory.In order to harden knfsd servers against this attack, create a method to sign and verify filehandles using siphash as a MAC (Message Authentication Code). Filehandles that have been signed cannot be tampered with, nor can clients reasonably guess correct filehandles and hashes that may exist in parts of the filesystem they cannot access due to directory permissions.
Append the 8 byte siphash to encoded filehandles for exports that have set the "
sign_fh" export option. The filehandle'sfh_auth_typeis set toFH_AT_MAC(1)to indicate the filehandle is signed. Filehandles received from clients are verified by comparing the appended hash to the expected hash. If the MAC does not match the server responds with NFS error_BADHANDLE. If unsigned filehandles are received for an export with "sign_fh" they are rejected with NFS error_BADHANDLE.Link:
https://lore.kernel.org/linux-nfs/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/nfsfh.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++-- fs/nfsd/nfsfh.h | 3 ++ 2 files changed, 73 insertions(+), 3 deletions(-)fs/nfsd/nfsfh.c +3
-0@@ -11,6 +11,7 @@#include <linux/exportfs.h> #include <linux/sunrpc/svcauth_gss.h> +#include <crypto/utils.h> #include "nfsd.h" #include "vfs.h" #include "auth.h" @@ -137,6 +138,61 @@ static inline __be32 check_pseudo_root(structdentry *dentry, return nfs_ok; } +/* +* Append an 8-byte MAC to the filehandle hashed from the server's
fh_key:
+ */fs/nfsd/nfsfh.c +12
-0+static int fh_append_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (!(fhp->fh_export->ex_flags &NFSEXP_SIGN_FH )) +return 0 ; + +if (!fh_key) { + pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_key notset.\n");
fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + +if (fh->fh_size +sizeof (hash) > fhp->fh_maxsize) { + pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_size %dwould be greater"
fs/nfsd/nfsfh.c +1
-0+ " than fh_maxsize %d.\n", (int)(fh->fh_size + sizeof(hash)),
fhp->fh_maxsize);fs/nfsd/nfsfh.c +27
-0+return -EINVAL ; + } + + fh->fh_auth_type =FH_AT_MAC ; + hash =siphash (&fh->fh_raw, fh->fh_size, fh_key); +memcpy (&fh->fh_raw[fh->fh_size], &hash,sizeof (hash)); + fh->fh_size +=sizeof (hash); + +return 0 ; +} + +/* +* Verify that the the filehandle's MAC was hashed from this filehandle +* given the server's fh_key: +*/ +static int fh_verify_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (fhp->fh_handle.fh_auth_type !=FH_AT_MAC ) +return -EINVAL ; + +if (!fh_key) { + pr_warn_ratelimited("NFSD: unable to verify signed filehandles,
fh_keynot set.\n");fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + + hash =siphash (&fh->fh_raw, fh->fh_size -sizeof (hash), fh_key); + return crypto_memneq(&fh->fh_raw[fh->fh_size - sizeof(hash)], &hash,sizeof(hash));
fs/nfsd/nfsfh.c +6
-1+} +/* * Use the given filehandle to look up the corresponding export and * dentry. On success, the results are used to set fh_export and @@ -166,8 +222,11 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, if (--data_left < 0)return error; -if (fh->fh_auth_type !=0 ) + +/* either FH_AT_NONE or FH_AT_MAC */ +if (fh->fh_auth_type >1 )return error; + len =key_len (fh->fh_fsid_type) /4 ;if (len ==0 )fs/nfsd/nfsfh.c +7
-2return error;@@ -237,9 +296,14 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, fileid_type = fh->fh_fileid_type; -if (fileid_type ==FILEID_ROOT ) +if (fileid_type ==FILEID_ROOT ) { dentry =dget (exp->ex_path.dentry); -else { + }else { +if (exp->ex_flags &NFSEXP_SIGN_FH &&fh_verify_mac (fhp, net)) { +trace_nfsd_set_fh_dentry_badhandle (rqstp, fhp, -EKEYREJECTED ); + goto out; + } + dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, fid, data_left, fileid_type, 0,fs/nfsd/nfsfh.c +3
-0nfsd_acceptable, exp);@@ -495,6 +559,9 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp, fhp->fh_handle.fh_fileid_type = fileid_type >0 ? fileid_type :FILEID_INVALID ; fhp->fh_handle.fh_size += maxsize *4 ; + +if (fh_append_mac (fhp, exp->cd->net)) + fhp->fh_handle.fh_fileid_type =FILEID_INVALID ; }else { fhp->fh_handle.fh_fileid_type =FILEID_ROOT ;fs/nfsd/nfsfh.c +0
-0}fs/nfsd/nfsfh.h +0
-0
fs/nfsd/nfsfh.h struct knfsd_fh { · line 63
Quoted 5 lines
fs/nfsd/nfsfh.h +2
-0@@ -59,6 +59,9 @@ struct knfsd_fh {#define fh_fsid_type fh_raw[2]#define fh_fileid_type fh_raw[3] +#define FH_AT_NONE 0 +#define FH_AT_MAC 1
I'm pleased at how much this patch has shrunk since v1.
This might not be an actionable review comment, but help me understand this particular point. Why do you need both a sign_fh export option and a new FH auth type? Shouldn't the server just look for and validate FH signatures whenever the sign_fh export option is present?
It seems a little subtle, so perhaps a code comment somewhere could explain the need for both.
Quoted 6 lines
fs/nfsd/nfsfh.h +1
-0+static inline u32 *fh_fsid (const struct knfsd_fh *fh) {return (u32 *)&fh->fh_raw[4 ];Signature · 1 line
2.50.1
Signature · 1 line
Chuck Lever
Re: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 165 lines · Chuck Lever
Quoted 155 lines · Benjamin Coddington
NFS clients may bypass restrictive directory permissions by using
open_by_handle()(or other available OS system call) to guess the filehandles for files below that directory.In order to harden knfsd servers against this attack, create a method to sign and verify filehandles using siphash as a MAC (Message Authentication Code). Filehandles that have been signed cannot be tampered with, nor can clients reasonably guess correct filehandles and hashes that may exist in parts of the filesystem they cannot access due to directory permissions.
Append the 8 byte siphash to encoded filehandles for exports that have set the "
sign_fh" export option. The filehandle'sfh_auth_typeis set toFH_AT_MAC(1)to indicate the filehandle is signed. Filehandles received from clients are verified by comparing the appended hash to the expected hash. If the MAC does not match the server responds with NFS error_BADHANDLE. If unsigned filehandles are received for an export with "sign_fh" they are rejected with NFS error_BADHANDLE.Link:
https://lore.kernel.org/linux-nfs/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/nfsfh.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++-- fs/nfsd/nfsfh.h | 3 ++ 2 files changed, 73 insertions(+), 3 deletions(-)fs/nfsd/nfsfh.c +3
-0@@ -11,6 +11,7 @@#include <linux/exportfs.h> #include <linux/sunrpc/svcauth_gss.h> +#include <crypto/utils.h> #include "nfsd.h" #include "vfs.h" #include "auth.h" @@ -137,6 +138,61 @@ static inline __be32 check_pseudo_root(structdentry *dentry, return nfs_ok; } +/* +* Append an 8-byte MAC to the filehandle hashed from the server's
fh_key:
+ */fs/nfsd/nfsfh.c +12
-0+static int fh_append_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (!(fhp->fh_export->ex_flags &NFSEXP_SIGN_FH )) +return 0 ; + +if (!fh_key) { + pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_key notset.\n");
fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + +if (fh->fh_size +sizeof (hash) > fhp->fh_maxsize) { + pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_size %dwould be greater"
fs/nfsd/nfsfh.c +1
-0+ " than fh_maxsize %d.\n", (int)(fh->fh_size + sizeof(hash)),
fhp->fh_maxsize);fs/nfsd/nfsfh.c +27
-0+return -EINVAL ; + } + + fh->fh_auth_type =FH_AT_MAC ; + hash =siphash (&fh->fh_raw, fh->fh_size, fh_key); +memcpy (&fh->fh_raw[fh->fh_size], &hash,sizeof (hash)); + fh->fh_size +=sizeof (hash); + +return 0 ; +} + +/* +* Verify that the the filehandle's MAC was hashed from this filehandle +* given the server's fh_key: +*/ +static int fh_verify_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (fhp->fh_handle.fh_auth_type !=FH_AT_MAC ) +return -EINVAL ; + +if (!fh_key) { + pr_warn_ratelimited("NFSD: unable to verify signed filehandles,
fh_keynot set.\n");fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + + hash =siphash (&fh->fh_raw, fh->fh_size -sizeof (hash), fh_key); + return crypto_memneq(&fh->fh_raw[fh->fh_size - sizeof(hash)], &hash,sizeof(hash));
fs/nfsd/nfsfh.c +6
-1+} +/* * Use the given filehandle to look up the corresponding export and * dentry. On success, the results are used to set fh_export and @@ -166,8 +222,11 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, if (--data_left < 0)return error; -if (fh->fh_auth_type !=0 ) + +/* either FH_AT_NONE or FH_AT_MAC */ +if (fh->fh_auth_type >1 )return error; + len =key_len (fh->fh_fsid_type) /4 ;if (len ==0 )fs/nfsd/nfsfh.c +7
-2return error;@@ -237,9 +296,14 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, fileid_type = fh->fh_fileid_type; -if (fileid_type ==FILEID_ROOT ) +if (fileid_type ==FILEID_ROOT ) { dentry =dget (exp->ex_path.dentry); -else { + }else { +if (exp->ex_flags &NFSEXP_SIGN_FH &&fh_verify_mac (fhp, net)) { +trace_nfsd_set_fh_dentry_badhandle (rqstp, fhp, -EKEYREJECTED ); + goto out; + } + dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, fid, data_left, fileid_type, 0,fs/nfsd/nfsfh.c +3
-0nfsd_acceptable, exp);@@ -495,6 +559,9 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp, fhp->fh_handle.fh_fileid_type = fileid_type >0 ? fileid_type :FILEID_INVALID ; fhp->fh_handle.fh_size += maxsize *4 ; + +if (fh_append_mac (fhp, exp->cd->net)) + fhp->fh_handle.fh_fileid_type =FILEID_INVALID ; }else { fhp->fh_handle.fh_fileid_type =FILEID_ROOT ;fs/nfsd/nfsfh.c +0
-0}fs/nfsd/nfsfh.h +0
-0fs/nfsd/nfsfh.h struct knfsd_fh { · line 63
Quoted 5 lines
fs/nfsd/nfsfh.h +2
-0@@ -59,6 +59,9 @@ struct knfsd_fh {#define fh_fsid_type fh_raw[2]#define fh_fileid_type fh_raw[3] +#define FH_AT_NONE 0 +#define FH_AT_MAC 1I'm pleased at how much this patch has shrunk since v1.
Quoted 5 lines
This might not be an actionable review comment, but help me understand this particular point. Why do you need both a
sign_fhexport option and a new FH auth type? Shouldn't the server just look for and validate FH signatures whenever thesign_fhexport option is present?
...and also generate valid signatures on outgoing file handles.
What does the server do to "look for" an FH signature so that it can "validate" it? Answer: it inspects the fh_auth_type to see if it is FT_AT_MAC.
Quoted 3 lines
It seems a little subtle, so perhaps a code comment somewhere could explain the need for both.
/*
* FT_AT_MAC allows the server to detect if a signature is expected
* in the last 8 bytes of the file handle.
*/
I wonder if it is really "last 8" for NFSv2 ... or even if v2 is supported. I should check the code I guess.
NeilBrown
Quoted 12 lines
Quoted 8 lines
fs/nfsd/nfsfh.h +1
-0+static inline u32 *fh_fsid (const struct knfsd_fh *fh) {return (u32 *)&fh->fh_raw[4 ];Signature · 1 line
2.50.1Signature · 1 line
Chuck Lever
Chuck Lever
maintainerRe: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 172 lines · NeilBrown
Quoted 165 lines · Chuck Lever
Quoted 155 lines · Benjamin Coddington
NFS clients may bypass restrictive directory permissions by using
open_by_handle()(or other available OS system call) to guess the filehandles for files below that directory.In order to harden knfsd servers against this attack, create a method to sign and verify filehandles using siphash as a MAC (Message Authentication Code). Filehandles that have been signed cannot be tampered with, nor can clients reasonably guess correct filehandles and hashes that may exist in parts of the filesystem they cannot access due to directory permissions.
Append the 8 byte siphash to encoded filehandles for exports that have set the "
sign_fh" export option. The filehandle'sfh_auth_typeis set toFH_AT_MAC(1)to indicate the filehandle is signed. Filehandles received from clients are verified by comparing the appended hash to the expected hash. If the MAC does not match the server responds with NFS error_BADHANDLE. If unsigned filehandles are received for an export with "sign_fh" they are rejected with NFS error_BADHANDLE.Link:
https://lore.kernel.org/linux-nfs/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/nfsfh.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++-- fs/nfsd/nfsfh.h | 3 ++ 2 files changed, 73 insertions(+), 3 deletions(-)fs/nfsd/nfsfh.c +3
-0@@ -11,6 +11,7 @@#include <linux/exportfs.h> #include <linux/sunrpc/svcauth_gss.h> +#include <crypto/utils.h> #include "nfsd.h" #include "vfs.h" #include "auth.h" @@ -137,6 +138,61 @@ static inline __be32 check_pseudo_root(structdentry *dentry, return nfs_ok; } +/* +* Append an 8-byte MAC to the filehandle hashed from the server's
fh_key:
+ */fs/nfsd/nfsfh.c +12
-0+static int fh_append_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (!(fhp->fh_export->ex_flags &NFSEXP_SIGN_FH )) +return 0 ; + +if (!fh_key) { + pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_key notset.\n");
fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + +if (fh->fh_size +sizeof (hash) > fhp->fh_maxsize) { + pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_size %dwould be greater"
fs/nfsd/nfsfh.c +1
-0+ " than fh_maxsize %d.\n", (int)(fh->fh_size + sizeof(hash)),
fhp->fh_maxsize);fs/nfsd/nfsfh.c +27
-0+return -EINVAL ; + } + + fh->fh_auth_type =FH_AT_MAC ; + hash =siphash (&fh->fh_raw, fh->fh_size, fh_key); +memcpy (&fh->fh_raw[fh->fh_size], &hash,sizeof (hash)); + fh->fh_size +=sizeof (hash); + +return 0 ; +} + +/* +* Verify that the the filehandle's MAC was hashed from this filehandle +* given the server's fh_key: +*/ +static int fh_verify_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (fhp->fh_handle.fh_auth_type !=FH_AT_MAC ) +return -EINVAL ; + +if (!fh_key) { + pr_warn_ratelimited("NFSD: unable to verify signed filehandles,
fh_keynot set.\n");fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + + hash =siphash (&fh->fh_raw, fh->fh_size -sizeof (hash), fh_key); + return crypto_memneq(&fh->fh_raw[fh->fh_size - sizeof(hash)], &hash,sizeof(hash));
fs/nfsd/nfsfh.c +6
-1+} +/* * Use the given filehandle to look up the corresponding export and * dentry. On success, the results are used to set fh_export and @@ -166,8 +222,11 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, if (--data_left < 0)return error; -if (fh->fh_auth_type !=0 ) + +/* either FH_AT_NONE or FH_AT_MAC */ +if (fh->fh_auth_type >1 )return error; + len =key_len (fh->fh_fsid_type) /4 ;if (len ==0 )fs/nfsd/nfsfh.c +7
-2return error;@@ -237,9 +296,14 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, fileid_type = fh->fh_fileid_type; -if (fileid_type ==FILEID_ROOT ) +if (fileid_type ==FILEID_ROOT ) { dentry =dget (exp->ex_path.dentry); -else { + }else { +if (exp->ex_flags &NFSEXP_SIGN_FH &&fh_verify_mac (fhp, net)) { +trace_nfsd_set_fh_dentry_badhandle (rqstp, fhp, -EKEYREJECTED ); + goto out; + } + dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, fid, data_left, fileid_type, 0,fs/nfsd/nfsfh.c +3
-0nfsd_acceptable, exp);@@ -495,6 +559,9 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp, fhp->fh_handle.fh_fileid_type = fileid_type >0 ? fileid_type :FILEID_INVALID ; fhp->fh_handle.fh_size += maxsize *4 ; + +if (fh_append_mac (fhp, exp->cd->net)) + fhp->fh_handle.fh_fileid_type =FILEID_INVALID ; }else { fhp->fh_handle.fh_fileid_type =FILEID_ROOT ;fs/nfsd/nfsfh.c +0
-0}fs/nfsd/nfsfh.h +0
-0fs/nfsd/nfsfh.h struct knfsd_fh { · line 63
Quoted 5 lines
fs/nfsd/nfsfh.h +2
-0@@ -59,6 +59,9 @@ struct knfsd_fh {#define fh_fsid_type fh_raw[2]#define fh_fileid_type fh_raw[3] +#define FH_AT_NONE 0 +#define FH_AT_MAC 1I'm pleased at how much this patch has shrunk since v1.
Quoted 5 lines
This might not be an actionable review comment, but help me understand this particular point. Why do you need both a
sign_fhexport option and a new FH auth type? Shouldn't the server just look for and validate FH signatures whenever thesign_fhexport option is present?
Quoted 5 lines
...and also generate valid signatures on outgoing file handles.
What does the server do to "look for" an FH signature so that it can "validate" it? Answer: it inspects the
fh_auth_typeto see if it isFT_AT_MAC.
No, NFSD checks the sign_fh export option. At first glance the two seem redundant, and I might hesitate to inspect or not inspect depending on information content received from a remote system. The security policy is defined precisely by the "sign_fh" export option I would think?
Quoted 10 lines
Quoted 2 lines
It seems a little subtle, so perhaps a code comment somewhere could explain the need for both.
/*
*FT_AT_MACallows the server to detect if a signature is expected
* in the last 8 bytes of the file handle.
*/I wonder if it is really "last 8" for NFSv2 ... or even if v2 is supported. I should check the code I guess.
I believe NFSv2 is not supported.
Quoted 12 lines
Quoted 11 lines
Quoted 7 lines
fs/nfsd/nfsfh.h +1
-0+static inline u32 *fh_fsid (const struct knfsd_fh *fh) {return (u32 *)&fh->fh_raw[4 ];Signature · 1 line
2.50.1Signature · 1 line
Chuck Lever
Signature · 1 line
Chuck Lever
Re: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 179 lines · Chuck Lever
Quoted 172 lines · NeilBrown
Quoted 165 lines · Chuck Lever
Quoted 155 lines · Benjamin Coddington
NFS clients may bypass restrictive directory permissions by using
open_by_handle()(or other available OS system call) to guess the filehandles for files below that directory.In order to harden knfsd servers against this attack, create a method to sign and verify filehandles using siphash as a MAC (Message Authentication Code). Filehandles that have been signed cannot be tampered with, nor can clients reasonably guess correct filehandles and hashes that may exist in parts of the filesystem they cannot access due to directory permissions.
Append the 8 byte siphash to encoded filehandles for exports that have set the "
sign_fh" export option. The filehandle'sfh_auth_typeis set toFH_AT_MAC(1)to indicate the filehandle is signed. Filehandles received from clients are verified by comparing the appended hash to the expected hash. If the MAC does not match the server responds with NFS error_BADHANDLE. If unsigned filehandles are received for an export with "sign_fh" they are rejected with NFS error_BADHANDLE.Link:
https://lore.kernel.org/linux-nfs/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/nfsfh.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++-- fs/nfsd/nfsfh.h | 3 ++ 2 files changed, 73 insertions(+), 3 deletions(-)fs/nfsd/nfsfh.c +3
-0@@ -11,6 +11,7 @@#include <linux/exportfs.h> #include <linux/sunrpc/svcauth_gss.h> +#include <crypto/utils.h> #include "nfsd.h" #include "vfs.h" #include "auth.h" @@ -137,6 +138,61 @@ static inline __be32 check_pseudo_root(structdentry *dentry, return nfs_ok; } +/* +* Append an 8-byte MAC to the filehandle hashed from the server's
fh_key:
+ */fs/nfsd/nfsfh.c +12
-0+static int fh_append_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (!(fhp->fh_export->ex_flags &NFSEXP_SIGN_FH )) +return 0 ; + +if (!fh_key) { + pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_key notset.\n");
fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + +if (fh->fh_size +sizeof (hash) > fhp->fh_maxsize) { + pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_size %dwould be greater"
fs/nfsd/nfsfh.c +1
-0+ " than fh_maxsize %d.\n", (int)(fh->fh_size + sizeof(hash)),
fhp->fh_maxsize);fs/nfsd/nfsfh.c +27
-0+return -EINVAL ; + } + + fh->fh_auth_type =FH_AT_MAC ; + hash =siphash (&fh->fh_raw, fh->fh_size, fh_key); +memcpy (&fh->fh_raw[fh->fh_size], &hash,sizeof (hash)); + fh->fh_size +=sizeof (hash); + +return 0 ; +} + +/* +* Verify that the the filehandle's MAC was hashed from this filehandle +* given the server's fh_key: +*/ +static int fh_verify_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (fhp->fh_handle.fh_auth_type !=FH_AT_MAC ) +return -EINVAL ; + +if (!fh_key) { + pr_warn_ratelimited("NFSD: unable to verify signed filehandles,
fh_keynot set.\n");fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + + hash =siphash (&fh->fh_raw, fh->fh_size -sizeof (hash), fh_key); + return crypto_memneq(&fh->fh_raw[fh->fh_size - sizeof(hash)], &hash,sizeof(hash));
fs/nfsd/nfsfh.c +6
-1+} +/* * Use the given filehandle to look up the corresponding export and * dentry. On success, the results are used to set fh_export and @@ -166,8 +222,11 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, if (--data_left < 0)return error; -if (fh->fh_auth_type !=0 ) + +/* either FH_AT_NONE or FH_AT_MAC */ +if (fh->fh_auth_type >1 )return error; + len =key_len (fh->fh_fsid_type) /4 ;if (len ==0 )fs/nfsd/nfsfh.c +7
-2return error;@@ -237,9 +296,14 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, fileid_type = fh->fh_fileid_type; -if (fileid_type ==FILEID_ROOT ) +if (fileid_type ==FILEID_ROOT ) { dentry =dget (exp->ex_path.dentry); -else { + }else { +if (exp->ex_flags &NFSEXP_SIGN_FH &&fh_verify_mac (fhp, net)) { +trace_nfsd_set_fh_dentry_badhandle (rqstp, fhp, -EKEYREJECTED ); + goto out; + } + dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, fid, data_left, fileid_type, 0,fs/nfsd/nfsfh.c +3
-0nfsd_acceptable, exp);@@ -495,6 +559,9 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp, fhp->fh_handle.fh_fileid_type = fileid_type >0 ? fileid_type :FILEID_INVALID ; fhp->fh_handle.fh_size += maxsize *4 ; + +if (fh_append_mac (fhp, exp->cd->net)) + fhp->fh_handle.fh_fileid_type =FILEID_INVALID ; }else { fhp->fh_handle.fh_fileid_type =FILEID_ROOT ;fs/nfsd/nfsfh.c +0
-0}fs/nfsd/nfsfh.h +0
-0fs/nfsd/nfsfh.h struct knfsd_fh { · line 63
Quoted 5 lines
fs/nfsd/nfsfh.h +2
-0@@ -59,6 +59,9 @@ struct knfsd_fh {#define fh_fsid_type fh_raw[2]#define fh_fileid_type fh_raw[3] +#define FH_AT_NONE 0 +#define FH_AT_MAC 1I'm pleased at how much this patch has shrunk since v1.
Quoted 5 lines
This might not be an actionable review comment, but help me understand this particular point. Why do you need both a
sign_fhexport option and a new FH auth type? Shouldn't the server just look for and validate FH signatures whenever thesign_fhexport option is present?Quoted 5 lines
...and also generate valid signatures on outgoing file handles.
What does the server do to "look for" an FH signature so that it can "validate" it? Answer: it inspects the
fh_auth_typeto see if it isFT_AT_MAC.
Quoted 5 lines
No, NFSD checks the
sign_fhexport option. At first glance the two seem redundant, and I might hesitate to inspect or not inspect depending on information content received from a remote system. The security policy is defined precisely by the "sign_fh" export option I would think?
So maybe you are thinking that, when sign_fh, is in effect - nfsd could always strip off the last 8 bytes, hash the remainder, and check the result matches the stripped bytes.
Yes - that could work.
Another reason is that it helps people who are looking at network packets captures to try to work out what is going wrong. Seeing a flag to say "there is a signature" could help.
Thanks,
NeilBrown
Quoted 33 lines
Quoted 5 lines
Quoted 4 lines
It seems a little subtle, so perhaps a code comment somewhere could explain the need for both.
Quoted 7 lines
/*
*FT_AT_MACallows the server to detect if a signature is expected
* in the last 8 bytes of the file handle.
*/I wonder if it is really "last 8" for NFSv2 ... or even if v2 is supported. I should check the code I guess.
I believe NFSv2 is not supported.
Quoted 12 lines
Quoted 11 lines
Quoted 7 lines
fs/nfsd/nfsfh.h +1
-0+static inline u32 *fh_fsid (const struct knfsd_fh *fh) {return (u32 *)&fh->fh_raw[4 ];Signature · 1 line
2.50.1Signature · 1 line
Chuck LeverSignature · 1 line
Chuck Lever
Chuck Lever
maintainerRe: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 186 lines · NeilBrown
Quoted 179 lines · Chuck Lever
Quoted 172 lines · NeilBrown
Quoted 165 lines · Chuck Lever
Quoted 155 lines · Benjamin Coddington
NFS clients may bypass restrictive directory permissions by using
open_by_handle()(or other available OS system call) to guess the filehandles for files below that directory.In order to harden knfsd servers against this attack, create a method to sign and verify filehandles using siphash as a MAC (Message Authentication Code). Filehandles that have been signed cannot be tampered with, nor can clients reasonably guess correct filehandles and hashes that may exist in parts of the filesystem they cannot access due to directory permissions.
Append the 8 byte siphash to encoded filehandles for exports that have set the "
sign_fh" export option. The filehandle'sfh_auth_typeis set toFH_AT_MAC(1)to indicate the filehandle is signed. Filehandles received from clients are verified by comparing the appended hash to the expected hash. If the MAC does not match the server responds with NFS error_BADHANDLE. If unsigned filehandles are received for an export with "sign_fh" they are rejected with NFS error_BADHANDLE.Link:
https://lore.kernel.org/linux-nfs/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/nfsfh.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++-- fs/nfsd/nfsfh.h | 3 ++ 2 files changed, 73 insertions(+), 3 deletions(-)fs/nfsd/nfsfh.c +3
-0@@ -11,6 +11,7 @@#include <linux/exportfs.h> #include <linux/sunrpc/svcauth_gss.h> +#include <crypto/utils.h> #include "nfsd.h" #include "vfs.h" #include "auth.h" @@ -137,6 +138,61 @@ static inline __be32 check_pseudo_root(structdentry *dentry, return nfs_ok; } +/* +* Append an 8-byte MAC to the filehandle hashed from the server's
fh_key:
+ */fs/nfsd/nfsfh.c +12
-0+static int fh_append_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (!(fhp->fh_export->ex_flags &NFSEXP_SIGN_FH )) +return 0 ; + +if (!fh_key) { + pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_key notset.\n");
fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + +if (fh->fh_size +sizeof (hash) > fhp->fh_maxsize) { + pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_size %dwould be greater"
fs/nfsd/nfsfh.c +1
-0+ " than fh_maxsize %d.\n", (int)(fh->fh_size + sizeof(hash)),
fhp->fh_maxsize);fs/nfsd/nfsfh.c +27
-0+return -EINVAL ; + } + + fh->fh_auth_type =FH_AT_MAC ; + hash =siphash (&fh->fh_raw, fh->fh_size, fh_key); +memcpy (&fh->fh_raw[fh->fh_size], &hash,sizeof (hash)); + fh->fh_size +=sizeof (hash); + +return 0 ; +} + +/* +* Verify that the the filehandle's MAC was hashed from this filehandle +* given the server's fh_key: +*/ +static int fh_verify_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (fhp->fh_handle.fh_auth_type !=FH_AT_MAC ) +return -EINVAL ; + +if (!fh_key) { + pr_warn_ratelimited("NFSD: unable to verify signed filehandles,
fh_keynot set.\n");fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + + hash =siphash (&fh->fh_raw, fh->fh_size -sizeof (hash), fh_key); + return crypto_memneq(&fh->fh_raw[fh->fh_size - sizeof(hash)], &hash,sizeof(hash));
fs/nfsd/nfsfh.c +6
-1+} +/* * Use the given filehandle to look up the corresponding export and * dentry. On success, the results are used to set fh_export and @@ -166,8 +222,11 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, if (--data_left < 0)return error; -if (fh->fh_auth_type !=0 ) + +/* either FH_AT_NONE or FH_AT_MAC */ +if (fh->fh_auth_type >1 )return error; + len =key_len (fh->fh_fsid_type) /4 ;if (len ==0 )fs/nfsd/nfsfh.c +7
-2return error;@@ -237,9 +296,14 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, fileid_type = fh->fh_fileid_type; -if (fileid_type ==FILEID_ROOT ) +if (fileid_type ==FILEID_ROOT ) { dentry =dget (exp->ex_path.dentry); -else { + }else { +if (exp->ex_flags &NFSEXP_SIGN_FH &&fh_verify_mac (fhp, net)) { +trace_nfsd_set_fh_dentry_badhandle (rqstp, fhp, -EKEYREJECTED ); + goto out; + } + dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, fid, data_left, fileid_type, 0,fs/nfsd/nfsfh.c +3
-0nfsd_acceptable, exp);@@ -495,6 +559,9 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp, fhp->fh_handle.fh_fileid_type = fileid_type >0 ? fileid_type :FILEID_INVALID ; fhp->fh_handle.fh_size += maxsize *4 ; + +if (fh_append_mac (fhp, exp->cd->net)) + fhp->fh_handle.fh_fileid_type =FILEID_INVALID ; }else { fhp->fh_handle.fh_fileid_type =FILEID_ROOT ;fs/nfsd/nfsfh.c +0
-0}fs/nfsd/nfsfh.h +0
-0fs/nfsd/nfsfh.h struct knfsd_fh { · line 63
Quoted 5 lines
fs/nfsd/nfsfh.h +2
-0@@ -59,6 +59,9 @@ struct knfsd_fh {#define fh_fsid_type fh_raw[2]#define fh_fileid_type fh_raw[3] +#define FH_AT_NONE 0 +#define FH_AT_MAC 1I'm pleased at how much this patch has shrunk since v1.
Quoted 5 lines
This might not be an actionable review comment, but help me understand this particular point. Why do you need both a
sign_fhexport option and a new FH auth type? Shouldn't the server just look for and validate FH signatures whenever thesign_fhexport option is present?Quoted 5 lines
...and also generate valid signatures on outgoing file handles.
What does the server do to "look for" an FH signature so that it can "validate" it? Answer: it inspects the
fh_auth_typeto see if it isFT_AT_MAC.Quoted 5 lines
No, NFSD checks the
sign_fhexport option. At first glance the two seem redundant, and I might hesitate to inspect or not inspect depending on information content received from a remote system. The security policy is defined precisely by the "sign_fh" export option I would think?
Quoted 3 lines
So maybe you are thinking that, when
sign_fh, is in effect - nfsd could always strip off the last 8 bytes, hash the remainder, and check the result matches the stripped bytes.
I’m wondering why there is both — the purpose of having these two seemingly redundant signals is worth documenting. There was some discussion a few days ago about whether the root FH could be signed or not. I thought for a moment or two that maybe when sign_fh is enabled, there will be one or more file handles on that export that won’t have a signature, and FT_AT_NONE would set those apart from the signed FHs. Again, I’d like to see that documented if that is the case.
In addition, I’ve always been told that what comes off the network is completely untrusted. So, I want some assurance that using the incoming FH’s auth type as part of the decision to check the signature conforms with known best practices.
Quoted 3 lines
Another reason is that it helps people who are looking at network packets captures to try to work out what is going wrong. Seeing a flag to say "there is a signature" could help.
Sure. But unconditionally trusting that flag is another question.
Signature · 1 line
Chuck Lever
Re: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 192 lines · Chuck Lever
Quoted 186 lines · NeilBrown
Quoted 179 lines · Chuck Lever
Quoted 172 lines · NeilBrown
Quoted 165 lines · Chuck Lever
Quoted 155 lines · Benjamin Coddington
NFS clients may bypass restrictive directory permissions by using
open_by_handle()(or other available OS system call) to guess the filehandles for files below that directory.In order to harden knfsd servers against this attack, create a method to sign and verify filehandles using siphash as a MAC (Message Authentication Code). Filehandles that have been signed cannot be tampered with, nor can clients reasonably guess correct filehandles and hashes that may exist in parts of the filesystem they cannot access due to directory permissions.
Append the 8 byte siphash to encoded filehandles for exports that have set the "
sign_fh" export option. The filehandle'sfh_auth_typeis set toFH_AT_MAC(1)to indicate the filehandle is signed. Filehandles received from clients are verified by comparing the appended hash to the expected hash. If the MAC does not match the server responds with NFS error_BADHANDLE. If unsigned filehandles are received for an export with "sign_fh" they are rejected with NFS error_BADHANDLE.Link:
https://lore.kernel.org/linux-nfs/[email protected]Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/nfsfh.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++-- fs/nfsd/nfsfh.h | 3 ++ 2 files changed, 73 insertions(+), 3 deletions(-)fs/nfsd/nfsfh.c +3
-0@@ -11,6 +11,7 @@#include <linux/exportfs.h> #include <linux/sunrpc/svcauth_gss.h> +#include <crypto/utils.h> #include "nfsd.h" #include "vfs.h" #include "auth.h" @@ -137,6 +138,61 @@ static inline __be32 check_pseudo_root(structdentry *dentry, return nfs_ok; } +/* +* Append an 8-byte MAC to the filehandle hashed from the server's
fh_key:
+ */fs/nfsd/nfsfh.c +12
-0+static int fh_append_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (!(fhp->fh_export->ex_flags &NFSEXP_SIGN_FH )) +return 0 ; + +if (!fh_key) { + pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_key notset.\n");
fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + +if (fh->fh_size +sizeof (hash) > fhp->fh_maxsize) { + pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_size %dwould be greater"
fs/nfsd/nfsfh.c +1
-0+ " than fh_maxsize %d.\n", (int)(fh->fh_size + sizeof(hash)),
fhp->fh_maxsize);fs/nfsd/nfsfh.c +27
-0+return -EINVAL ; + } + + fh->fh_auth_type =FH_AT_MAC ; + hash =siphash (&fh->fh_raw, fh->fh_size, fh_key); +memcpy (&fh->fh_raw[fh->fh_size], &hash,sizeof (hash)); + fh->fh_size +=sizeof (hash); + +return 0 ; +} + +/* +* Verify that the the filehandle's MAC was hashed from this filehandle +* given the server's fh_key: +*/ +static int fh_verify_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (fhp->fh_handle.fh_auth_type !=FH_AT_MAC ) +return -EINVAL ; + +if (!fh_key) { + pr_warn_ratelimited("NFSD: unable to verify signed filehandles,
fh_keynot set.\n");fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + + hash =siphash (&fh->fh_raw, fh->fh_size -sizeof (hash), fh_key); + return crypto_memneq(&fh->fh_raw[fh->fh_size - sizeof(hash)], &hash,sizeof(hash));
fs/nfsd/nfsfh.c +6
-1+} +/* * Use the given filehandle to look up the corresponding export and * dentry. On success, the results are used to set fh_export and @@ -166,8 +222,11 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, if (--data_left < 0)return error; -if (fh->fh_auth_type !=0 ) + +/* either FH_AT_NONE or FH_AT_MAC */ +if (fh->fh_auth_type >1 )return error; + len =key_len (fh->fh_fsid_type) /4 ;if (len ==0 )fs/nfsd/nfsfh.c +7
-2return error;@@ -237,9 +296,14 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, fileid_type = fh->fh_fileid_type; -if (fileid_type ==FILEID_ROOT ) +if (fileid_type ==FILEID_ROOT ) { dentry =dget (exp->ex_path.dentry); -else { + }else { +if (exp->ex_flags &NFSEXP_SIGN_FH &&fh_verify_mac (fhp, net)) { +trace_nfsd_set_fh_dentry_badhandle (rqstp, fhp, -EKEYREJECTED ); + goto out; + } + dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, fid, data_left, fileid_type, 0,fs/nfsd/nfsfh.c +3
-0nfsd_acceptable, exp);@@ -495,6 +559,9 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp, fhp->fh_handle.fh_fileid_type = fileid_type >0 ? fileid_type :FILEID_INVALID ; fhp->fh_handle.fh_size += maxsize *4 ; + +if (fh_append_mac (fhp, exp->cd->net)) + fhp->fh_handle.fh_fileid_type =FILEID_INVALID ; }else { fhp->fh_handle.fh_fileid_type =FILEID_ROOT ;fs/nfsd/nfsfh.c +0
-0}fs/nfsd/nfsfh.h +0
-0fs/nfsd/nfsfh.h struct knfsd_fh { · line 63
Quoted 5 lines
fs/nfsd/nfsfh.h +2
-0@@ -59,6 +59,9 @@ struct knfsd_fh {#define fh_fsid_type fh_raw[2]#define fh_fileid_type fh_raw[3] +#define FH_AT_NONE 0 +#define FH_AT_MAC 1I'm pleased at how much this patch has shrunk since v1.
Quoted 5 lines
This might not be an actionable review comment, but help me understand this particular point. Why do you need both a
sign_fhexport option and a new FH auth type? Shouldn't the server just look for and validate FH signatures whenever thesign_fhexport option is present?Quoted 5 lines
...and also generate valid signatures on outgoing file handles.
What does the server do to "look for" an FH signature so that it can "validate" it? Answer: it inspects the
fh_auth_typeto see if it isFT_AT_MAC.Quoted 5 lines
No, NFSD checks the
sign_fhexport option. At first glance the two seem redundant, and I might hesitate to inspect or not inspect depending on information content received from a remote system. The security policy is defined precisely by the "sign_fh" export option I would think?Quoted 3 lines
So maybe you are thinking that, when
sign_fh, is in effect - nfsd could always strip off the last 8 bytes, hash the remainder, and check the result matches the stripped bytes.
Quoted 8 lines
I’m wondering why there is both — the purpose of having these two seemingly redundant signals is worth documenting. There was some discussion a few days ago about whether the root FH could be signed or not. I thought for a moment or two that maybe when
sign_fhis enabled, there will be one or more file handles on that export that won’t have a signature, andFT_AT_NONEwould set those apart from the signed FHs. Again, I’d like to see that documented if that is the case.
I would document it as:
sign_fh is needs to configure server policy
FT_AT_MAC, while technically redundant with sign_fh, is valuable
whehn interpreting NFS packet captures.
Quoted 11 lines
In addition, I’ve always been told that what comes off the network is completely untrusted. So, I want some assurance that using the incoming FH’s auth type as part of the decision to check the signature conforms with known best practices.
Quoted 3 lines
Another reason is that it helps people who are looking at network packets captures to try to work out what is going wrong. Seeing a flag to say "there is a signature" could help.
Sure. But unconditionally trusting that flag is another question.
By the time the code has reached this point it has already unconditionally trusted the RPC header, the NFS opcode, the '1' in fh_version, the fh_fsid_type and the fsid itself.
Going further to trust fh_auth_type to the extent that we reject the request if it is 0, and check the MAC if it is 1 - is not significant.
NeilBrown
Re: [PATCH v2 3/3] NFSD: Sign filehandles
Hey Chuck and Neil - Sorry to be late responding here..
Quoted 8 lines · NeilBrown
Quoted 7 lines · Chuck Lever
Quoted 5 lines · NeilBrown
Quoted 4 lines · Chuck Lever
Quoted 3 lines · NeilBrown
Quoted 2 lines · Chuck Lever
On Wed, Jan 21, 2026, at 3:24 PM, Benjamin Coddington wrote:
...
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+#define FH_AT_NONE 0 +#define FH_AT_MAC 1I'm pleased at how much this patch has shrunk since v1.
Me too, thanks for all the help refining it.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
This might not be an actionable review comment, but help me understand this particular point. Why do you need both a
sign_fhexport option and a new FH auth type? Shouldn't the server just look for and validate FH signatures whenever thesign_fhexport option is present?
Its vestigial from the encrypted_fh version which required it because the fsid might be encrypted, so NFSD couldn't look up the export to see if it was set to encrypt until decrypting the fsid, and needed the auth type to know if it was encrypted.
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 5 lines
...and also generate valid signatures on outgoing file handles.
What does the server do to "look for" an FH signature so that it can "validate" it? Answer: it inspects the
fh_auth_typeto see if it isFT_AT_MAC.No, NFSD checks the
sign_fhexport option. At first glance the two seem redundant, and I might hesitate to inspect or not inspect depending on information content received from a remote system. The security policy is defined precisely by the "sign_fh" export option I would think?
Yes, now its a bit redundant - but it could be used to still accept filehandles that are signed after removing a "sign_fh" from an export. In other words, it might be useful to be "be liberal in what you accept from others". It would be essential if future patches wanted to "drain" and "fill" clients with signed/plain filehandles using more permissive policies. *waves hands wildly*
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
So maybe you are thinking that, when
sign_fh, is in effect - nfsd could always strip off the last 8 bytes, hash the remainder, and check the result matches the stripped bytes.
Quoted 8 lines
Quoted 8 lines
I’m wondering why there is both — the purpose of having these two seemingly redundant signals is worth documenting. There was some discussion a few days ago about whether the root FH could be signed or not. I thought for a moment or two that maybe when
sign_fhis enabled, there will be one or more file handles on that export that won’t have a signature, andFT_AT_NONEwould set those apart from the signed FHs. Again, I’d like to see that documented if that is the case.
Right now no, not that I know of - the root filehandle is the only one, and its easy to detect.
Quoted 5 lines
I would document it as:
sign_fhis needs to configure server policy
FT_AT_MAC, while technically redundant withsign_fh, is valuable
whehn interpreting NFS packet captures.
Yes, it would allow a network dissector to locate and parse the MAC.
Quoted 9 lines
Quoted 9 lines
In addition, I’ve always been told that what comes off the network is completely untrusted. So, I want some assurance that using the incoming FH’s auth type as part of the decision to check the signature conforms with known best practices.
Quoted 3 lines
Another reason is that it helps people who are looking at network packets captures to try to work out what is going wrong. Seeing a flag to say "there is a signature" could help.
Quoted 8 lines
Quoted 1 line
Sure. But unconditionally trusting that flag is another question.
By the time the code has reached this point it has already unconditionally trusted the RPC header, the NFS opcode, the '1' in
fh_version, thefh_fsid_typeand the fsid itself.Going further to trust
fh_auth_typeto the extent that we reject the request if it is 0, and check the MAC if it is 1 - is not significant.
Not a great argument, I know, but I think its nice to keep the standard that filehandles are independently self-describing.
We're building server systems that pass around filehandles generated by NFSD and it can be a useful signal to those 3rd-party systems that there's a signature. Trond might know more about whether its essential - I'll ask him to weigh in here.
All said - please let me know if the next version should keep it.
Ben
Chuck Lever
maintainerRe: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 68 lines · Benjamin Coddington
Hey Chuck and Neil - Sorry to be late responding here..
Quoted 8 lines · NeilBrown
Quoted 7 lines · Chuck Lever
Quoted 5 lines · NeilBrown
Quoted 4 lines · Chuck Lever
Quoted 3 lines · NeilBrown
Quoted 2 lines · Chuck Lever
On Wed, Jan 21, 2026, at 3:24 PM, Benjamin Coddington wrote:
...
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+#define FH_AT_NONE 0 +#define FH_AT_MAC 1I'm pleased at how much this patch has shrunk since v1.
Me too, thanks for all the help refining it.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
This might not be an actionable review comment, but help me understand this particular point. Why do you need both a
sign_fhexport option and a new FH auth type? Shouldn't the server just look for and validate FH signatures whenever thesign_fhexport option is present?Its vestigial from the
encrypted_fhversion which required it because the fsid might be encrypted, so NFSD couldn't look up the export to see if it was set to encrypt until decrypting the fsid, and needed the auth type to know if it was encrypted.Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 5 lines
...and also generate valid signatures on outgoing file handles.
What does the server do to "look for" an FH signature so that it can "validate" it? Answer: it inspects the
fh_auth_typeto see if it isFT_AT_MAC.No, NFSD checks the
sign_fhexport option. At first glance the two seem redundant, and I might hesitate to inspect or not inspect depending on information content received from a remote system. The security policy is defined precisely by the "sign_fh" export option I would think?Yes, now its a bit redundant - but it could be used to still accept filehandles that are signed after removing a "
sign_fh" from an export. In other words, it might be useful to be "be liberal in what you accept from others". It would be essential if future patches wanted to "drain" and "fill" clients with signed/plain filehandles using more permissive policies. *waves hands wildly*Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
So maybe you are thinking that, when
sign_fh, is in effect - nfsd could always strip off the last 8 bytes, hash the remainder, and check the result matches the stripped bytes.Quoted 8 lines
Quoted 8 lines
I’m wondering why there is both — the purpose of having these two seemingly redundant signals is worth documenting. There was some discussion a few days ago about whether the root FH could be signed or not. I thought for a moment or two that maybe when
sign_fhis enabled, there will be one or more file handles on that export that won’t have a signature, andFT_AT_NONEwould set those apart from the signed FHs. Again, I’d like to see that documented if that is the case.Right now no, not that I know of - the root filehandle is the only one, and its easy to detect.
Quoted 25 lines
Quoted 5 lines
I would document it as:
sign_fhis needs to configure server policy
FT_AT_MAC, while technically redundant withsign_fh, is valuable
whehn interpreting NFS packet captures.Yes, it would allow a network dissector to locate and parse the MAC.
Quoted 17 lines
Quoted 10 lines
In addition, I’ve always been told that what comes off the network is completely untrusted. So, I want some assurance that using the incoming FH’s auth type as part of the decision to check the signature conforms with known best practices.
Quoted 3 lines
Another reason is that it helps people who are looking at network packets captures to try to work out what is going wrong. Seeing a flag to say "there is a signature" could help.
Sure. But unconditionally trusting that flag is another question.
By the time the code has reached this point it has already unconditionally trusted the RPC header, the NFS opcode, the '1' in
fh_version, thefh_fsid_typeand the fsid itself.Going further to trust
fh_auth_typeto the extent that we reject the request if it is 0, and check the MAC if it is 1 - is not significant.
What I'm saying is that if it makes no difference to the security level, then let's not bother to check it at all.
Quoted 7 lines
Not a great argument, I know, but I think its nice to keep the standard that filehandles are independently self-describing.
We're building server systems that pass around filehandles generated by NFSD and it can be a useful signal to those 3rd-party systems that there's a signature. Trond might know more about whether its essential - I'll ask him to weigh in here.
Thanks, yes, let's hear from Trond.
Quoted 1 line
All said - please let me know if the next version should keep it.
There are really two question marks:
1. If I were a security reviewer, I would say that NFSD shouldn't rely on network input like this to decide whether or not to validate the MAC. Either the server expects a MAC and uses it to validate, or it doesn't. For me as a maintainer, that is a risk we probably can deal with immediately -- would it be OK at least to change the FH verification code to not use the auth_type to decide when to validate the FH's MAC?
2. Is setting FH_AT_MAC still useful for other reasons? I think we don't really know whether to keep the auth_type or how to document it until we've decided on how exactly NFSD will deal with changing the sign_fh setting while clients have the export mounted.
So, let's leave the field in place and we'll come back to it. If you want, add a comment like /* XXX is FH_AT_MAC still needed? */
Signature · 1 line
Chuck Lever
Trond Myklebust
maintainerRe: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 36 lines · Chuck Lever
Quoted 9 lines · Benjamin Coddington
Hey Chuck and Neil - Sorry to be late responding here..
On 23 Jan 2026, at 20:56, NeilBrown wrote:
Not a great argument, I know, but I think its nice to keep the standard that
filehandles are independently self-describing.Quoted 7 lines
We're building server systems that pass around filehandles generated by NFSD
and it can be a useful signal to those 3rd-party systems that there's a
signature. Trond might know more about whether its essential - I'll ask him
to weigh in here.Thanks, yes, let's hear from Trond.
Quoted 1 line
All said - please let me know if the next version should keep it.
There are really two question marks:
1. If I were a security reviewer, I would say that NFSD shouldn't rely
on network input like this to decide whether or not to validate the MAC.
Either the server expects a MAC and uses it to validate, or it doesn't.
For me as a maintainer, that is a risk we probably can deal with immediately -- would it be OK at least to change the FH verification code to not use theauth_typeto decide when to validate the FH's MAC?
Quoted 9 lines
2. Is setting
FH_AT_MACstill useful for other reasons? I think we don't
really know whether to keep theauth_typeor how to document it until we've decided on how exactly NFSD will deal with changing thesign_fhsetting while clients have the export mounted.So, let's leave the field in place and we'll come back to it. If you want, add a comment like /* XXX is
FH_AT_MACstill needed? */
I fully agree with the argument that policy decisions about whether to check for a MAC need to be driven by the /etc/exports configuration, and not by the filehandle itself. The only use I can think of for a flag in that context would be to expedite the rejection of the filehandle in the case where that policy was set, however that would seem to be optimising for what should be a rare corner case.
I did speculate a little in some internal conversations whether or not a metadata server that is using knfsd as a flex files data server might be able to re-sign a filehandle after the secret key were changed, rather than having to look up the file again using its path. However that too is very much a corner case that we have no plans to optimise for at this time. Even if we did, we wouldn't need a flag in the filehandle to know whether or not to sign it, because we'd want to determine the policy through other means (if for no other reason that the metadata server needs to know the secret key as well).
So personally, I'm neutral about the need for that flag. I don't think it is harmful (provided it isn't being used by knfsd to determine MAC policy enforcement) however I don't see a strong argument for it being needed either.
Signature · 3 lines
Trond Myklebust Linux NFS client maintainer, Hammerspace [email protected], [email protected]
Chuck Lever
maintainerRe: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 64 lines · Trond Myklebust
Quoted 36 lines · Chuck Lever
Quoted 9 lines · Benjamin Coddington
Hey Chuck and Neil - Sorry to be late responding here..
On 23 Jan 2026, at 20:56, NeilBrown wrote:
Not a great argument, I know, but I think its nice to keep the standard that
filehandles are independently self-describing.Quoted 7 lines
We're building server systems that pass around filehandles generated by NFSD
and it can be a useful signal to those 3rd-party systems that there's a
signature. Trond might know more about whether its essential - I'll ask him
to weigh in here.Thanks, yes, let's hear from Trond.
Quoted 1 line
All said - please let me know if the next version should keep it.
There are really two question marks:
1. If I were a security reviewer, I would say that NFSD shouldn't rely
on network input like this to decide whether or not to validate the MAC.
Either the server expects a MAC and uses it to validate, or it doesn't.
For me as a maintainer, that is a risk we probably can deal with immediately -- would it be OK at least to change the FH verification code to not use theauth_typeto decide when to validate the FH's MAC?Quoted 9 lines
2. Is setting
FH_AT_MACstill useful for other reasons? I think we don't
really know whether to keep theauth_typeor how to document it until we've decided on how exactly NFSD will deal with changing thesign_fhsetting while clients have the export mounted.So, let's leave the field in place and we'll come back to it. If you want, add a comment like /* XXX is
FH_AT_MACstill needed? */I fully agree with the argument that policy decisions about whether to check for a MAC need to be driven by the
/etc/exportsconfiguration, and not by the filehandle itself. The only use I can think of for a flag in that context would be to expedite the rejection of the filehandle in the case where that policy was set, however that would seem to be optimising for what should be a rare corner case.I did speculate a little in some internal conversations whether or not a metadata server that is using knfsd as a flex files data server might be able to re-sign a filehandle after the secret key were changed, rather than having to look up the file again using its path. However that too is very much a corner case that we have no plans to optimise for at this time. Even if we did, we wouldn't need a flag in the filehandle to know whether or not to sign it, because we'd want to determine the policy through other means (if for no other reason that the metadata server needs to know the secret key as well).
Quoted 5 lines
So personally, I'm neutral about the need for that flag. I don't think it is harmful (provided it isn't being used by knfsd to determine MAC policy enforcement) however I don't see a strong argument for it being needed either.
Thanks for clarifying.
My feeling is that if FH_AT_MAC remains, those who continue maintaining the sign_fh feature will want to understand its purpose so they don't break anything when updating code. So documentation is essential.
But it doesn't seem necessary to keep FH_AT_MAC for good security and proper function.
I can't recall if Wireshark is smart enough to introspect Linux NFSD file handles (I thought it could). It would be sensible to have some Wireshark update code in hand before making the final decision about keeping the new auth_type.
Signature · 1 line
Chuck Lever
Re: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 4 lines · Chuck Lever
I can't recall if Wireshark is smart enough to introspect Linux NFSD file handles (I thought it could). It would be sensible to have some Wireshark update code in hand before making the final decision about keeping the new
auth_type.
I've gone digging and wireshark has a surprising amount of filehandle dissection code - it currently can "Decode As:"
dissect_fhandle_data_SVR4dissect_fhandle_data_LINUX_KNFSD_LEdissect_fhandle_data_LINUX_NFSD_LEdissect_fhandle_data_NETAPPdissect_fhandle_data_NETAPP_V4dissect_fhandle_data_NETAPP_GX_v3dissect_fhandle_data_LINUX_KNFSD_NEWdissect_fhandle_data_GLUSTERdissect_fhandle_data_DCACHEdissect_fhandle_data_PRIMARY_DATAdissect_fhandle_data_CELERRA_VNXdissect_fhandle_data_unknown
.. almost all with finer grained filehandle components. I certainly can add patches to parse FH_AT_MAC for the linux(s) decoders, but I admit I don't have any use case.
I'm completely neutral on keeping FH_AT_MAC at this point.
Ben
Chuck Lever
maintainerRe: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 23 lines · Benjamin Coddington
Quoted 4 lines · Chuck Lever
I can't recall if Wireshark is smart enough to introspect Linux NFSD file handles (I thought it could). It would be sensible to have some Wireshark update code in hand before making the final decision about keeping the new
auth_type.I've gone digging and wireshark has a surprising amount of filehandle dissection code - it currently can "Decode As:"
dissect_fhandle_data_SVR4dissect_fhandle_data_LINUX_KNFSD_LEdissect_fhandle_data_LINUX_NFSD_LEdissect_fhandle_data_NETAPPdissect_fhandle_data_NETAPP_V4dissect_fhandle_data_NETAPP_GX_v3dissect_fhandle_data_LINUX_KNFSD_NEWdissect_fhandle_data_GLUSTERdissect_fhandle_data_DCACHEdissect_fhandle_data_PRIMARY_DATAdissect_fhandle_data_CELERRA_VNXdissect_fhandle_data_unknown
Quoted 5 lines
.. almost all with finer grained filehandle components. I certainly can add patches to parse
FH_AT_MACfor the linux(s) decoders, but I admit I don't have any use case.I'm completely neutral on keeping
FH_AT_MACat this point.
If we can't find a use case or need for something, the usual practice is to remove it from your patches until we have one.
Is anyone working on Wireshark patches to handle signed Linux file handles in some kind of sensible way?
Signature · 1 line
Chuck Lever
Re: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 28 lines · Chuck Lever
Quoted 27 lines · Benjamin Coddington
Quoted 4 lines · Chuck Lever
I can't recall if Wireshark is smart enough to introspect Linux NFSD file handles (I thought it could). It would be sensible to have some Wireshark update code in hand before making the final decision about keeping the new
auth_type.I've gone digging and wireshark has a surprising amount of filehandle dissection code - it currently can "Decode As:"
dissect_fhandle_data_SVR4dissect_fhandle_data_LINUX_KNFSD_LEdissect_fhandle_data_LINUX_NFSD_LEdissect_fhandle_data_NETAPPdissect_fhandle_data_NETAPP_V4dissect_fhandle_data_NETAPP_GX_v3dissect_fhandle_data_LINUX_KNFSD_NEWdissect_fhandle_data_GLUSTERdissect_fhandle_data_DCACHEdissect_fhandle_data_PRIMARY_DATAdissect_fhandle_data_CELERRA_VNXdissect_fhandle_data_unknown.. almost all with finer grained filehandle components. I certainly can add patches to parse
FH_AT_MACfor the linux(s) decoders, but I admit I don't have any use case.
Quoted 7 lines
Quoted 1 line
I'm completely neutral on keeping
FH_AT_MACat this point.If we can't find a use case or need for something, the usual practice is to remove it from your patches until we have one.
Is anyone working on Wireshark patches to handle signed Linux file handles in some kind of sensible way?
Well the other use-case might just be for those pynfs tests you were asking for. Without FH_AT_MAC, its tricky for pynfs to know if its getting signed filehandles or not.
The other thing about those pynfs tests is that they get awfully linux specific - requiring new tooling to setup and change the export option.
Ben
Chuck Lever
maintainerRe: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 2 lines · Benjamin Coddington
The other thing about those pynfs tests is that they get awfully linux specific - requiring new tooling to setup and change the export option.
pynfs is for unit testing protocol compliance. Since signed file handles are a feature of the Linux implementation and not part of the NFS protocol standard, pynfs won't be an appropriate home for unit testing the signed FH framework in Linux. Sigh. We'll have to look for another framework that can host such testing.
Signature · 1 line
Chuck Lever
Re: [PATCH v2 3/3] NFSD: Sign filehandles
In reply to Benjamin CoddingtonQuoted 11 lines · Benjamin Coddington
NFS clients may bypass restrictive directory permissions by using
open_by_handle()(or other available OS system call) to guess the filehandles for files below that directory.In order to harden knfsd servers against this attack, create a method to sign and verify filehandles using siphash as a MAC (Message Authentication Code). Filehandles that have been signed cannot be tampered with, nor can clients reasonably guess correct filehandles and hashes that may exist in parts of the filesystem they cannot access due to directory permissions.
Quoted 7 lines
Append the 8 byte siphash to encoded filehandles for exports that have set the "
sign_fh" export option. The filehandle'sfh_auth_typeis set toFH_AT_MAC(1)to indicate the filehandle is signed. Filehandles received from clients are verified by comparing the appended hash to the expected hash. If the MAC does not match the server responds with NFS error_BADHANDLE. If unsigned filehandles are received for an export with "sign_fh" they are rejected with NFS error_BADHANDLE.
Random questions:
1. CPU load: Linux NFSv4 servers consume LOTS of CPU time, which has become a HUGE problem for hosting them on embedded hardware (so no realistic NFSv4 server performance on an i.mx6 or RISC/V machine). And this has become much worse in the last two years. Did anyone measure the impact of this patch series?
2. Do NFS clients require any changes for this?
Lionel
Re: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 23 lines · Lionel Cons
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
Quoted 18 lines · Benjamin Coddington
NFS clients may bypass restrictive directory permissions by using
open_by_handle()(or other available OS system call) to guess the filehandles for files below that directory.In order to harden knfsd servers against this attack, create a method to sign and verify filehandles using siphash as a MAC (Message Authentication Code). Filehandles that have been signed cannot be tampered with, nor can clients reasonably guess correct filehandles and hashes that may exist in parts of the filesystem they cannot access due to directory permissions.
Append the 8 byte siphash to encoded filehandles for exports that have set the "
sign_fh" export option. The filehandle'sfh_auth_typeis set toFH_AT_MAC(1)to indicate the filehandle is signed. Filehandles received from clients are verified by comparing the appended hash to the expected hash. If the MAC does not match the server responds with NFS error_BADHANDLE. If unsigned filehandles are received for an export with "sign_fh" they are rejected with NFS error_BADHANDLE.
Hi Lionel,
Quoted 6 lines
Random questions:
1. CPU load: Linux NFSv4 servers consume LOTS of CPU time, which has become a HUGE problem for hosting them on embedded hardware (so no realistic NFSv4 server performance on an i.mx6 or RISC/V machine). And this has become much worse in the last two years. Did anyone measure the impact of this patch series?
We're essentially adding a siphash operation for every encode and decode of a filehandle. Siphash is lauded as "faster than sha255, slower than xxhash". Measuring the performance impact might look like crafting huge compounds of GETATTR, but I honestly don't think (after network latency) the performance impact will be measurable.
I attempted to measure a time difference between runs of fstests suite -- there were no significant measurable effects in my crude total time calculations.
I could, if it pleases everyone, do a function profile for fh_append_mac and fh_verify_mac - but the users of this option do not care about gating it behind strict performance optimizations because we're fixing a security problem that matters much more to those users.
Quoted 1 line
2. Do NFS clients require any changes for this?
No - the filehandle is opaque.
Ben
Chuck Lever
maintainerRe: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 2 lines · Benjamin Coddington
I could, if it pleases everyone, do a function profile for
fh_append_macandfh_verify_mac
Trond or Mike can demonstrate for you how to capture flame graphs to very graphically illustrate how much CPU utilization is introduced when using this feature. It would be valuable to confirm our expectation that the additional CPU consumption will essentially be in the noise.
Signature · 1 line
Chuck Lever
Re: [PATCH v2 3/3] NFSD: Sign filehandles
Quoted 8 lines · Chuck Lever
Quoted 2 lines · Benjamin Coddington
I could, if it pleases everyone, do a function profile for
fh_append_macandfh_verify_macTrond or Mike can demonstrate for you how to capture flame graphs to very graphically illustrate how much CPU utilization is introduced when using this feature. It would be valuable to confirm our expectation that the additional CPU consumption will essentially be in the noise.
Here's a flame graph of a client doing 10 passes at a directory with 10k files, looking them up and then doing GETATTR on each. Each pass starts with an empty cache.
https://bcodding.com/signfh_cpu_flame_graph/perf.svg
You'll want to look for fh_append_mac and fh_verify_mac.
Easier to find fh_append_mac by zooming to nfsd4_proc_compound:
https://bcodding.com/signfh_cpu_flame_graph/perf.svg?s=fh_append_mac&x=65.2&y=757
Easier to find fh_verify_mac by zooming to nfsd4_putfh:
https://bcodding.com/signfh_cpu_flame_graph/perf.svg?s=fh_verify_mac&x=537.9&y=741
This profile was filtered to only show events for the nfsd threads, and the two functions needed to be annotated to remove the compiler optimizations in order to show them in the profile. This on a VM running in VMWare Fusion on an arm64 Macbook Pro.
Ben
Re: [PATCH v2 3/3] NFSD: Sign filehandles
In reply to Benjamin CoddingtonQuoted 73 lines · Benjamin Coddington
Quoted 4 lines · Lionel Cons
[You don't often get email from [email protected]. Learn why this is important at
https://aka.ms/LearnAboutSenderIdentification ]Quoted 32 lines
Quoted 29 lines · Benjamin Coddington
NFS clients may bypass restrictive directory permissions by using
open_by_handle()(or other available OS system call) to guess the filehandles for files below that directory.In order to harden knfsd servers against this attack, create a method to
sign and verify filehandles using siphash as a MAC (Message Authentication
Code). Filehandles that have been signed cannot be tampered with, nor can
clients reasonably guess correct filehandles and hashes that may exist in
parts of the filesystem they cannot access due to directory permissions.Append the 8 byte siphash to encoded filehandles for exports that have set
the "sign_fh" export option. The filehandle'sfh_auth_typeis set toFH_AT_MAC(1)to indicate the filehandle is signed. Filehandles received from
clients are verified by comparing the appended hash to the expected hash.
If the MAC does not match the server responds with NFS error_BADHANDLE.
If unsigned filehandles are received for an export with "sign_fh" they are
rejected with NFS error_BADHANDLE.Hi Lionel,
Quoted 9 lines
Random questions:
1. CPU load: Linux NFSv4 servers consume LOTS of CPU time, which has
become a HUGE problem for hosting them on embedded hardware (so no realistic NFSv4 server performance on an i.mx6 or RISC/V machine). And
this has become much worse in the last two years. Did anyone measure
the impact of this patch series?We're essentially adding a siphash operation for every encode and decode of
a filehandle. Siphash is lauded as "faster than sha255, slower than xxhash". Measuring the performance impact might look like crafting huge
compounds of GETATTR, but I honestly don't think (after network latency) the
performance impact will be measurable.I attempted to measure a time difference between runs of fstests suite --
there were no significant measurable effects in my crude total time calculations.I could, if it pleases everyone, do a function profile for
fh_append_macandfh_verify_mac- but the users of this option do not care about gating it
behind strict performance optimizations because we're fixing a security
problem that matters much more to those users.
Quoted 5 lines
Quoted 1 line
2. Do NFS clients require any changes for this?
No - the filehandle is opaque.
Ben
The other thing to note is that this is an opt-in feature. If you don't want to use it for fear of CPU load, you are free not to change your /etc/exports config file.
Signature · 3 lines
Trond Myklebust Linux NFS client maintainer, Hammerspace [email protected], [email protected]
Chuck Lever
maintainerRe: [PATCH v2 3/3] NFSD: Sign filehandles
In reply to Lionel ConsQuoted 4 lines · Lionel Cons
1. CPU load: Linux NFSv4 servers consume LOTS of CPU time, which has become a HUGE problem for hosting them on embedded hardware (so no realistic NFSv4 server performance on an i.mx6 or RISC/V machine). And this has become much worse in the last two years.
If this is a general concern, please take the time to collect some data and file a full regression report.
Also note that signed file handles are enabled by administrative settings that default "off". There will be little to no additional CPU load if you choose not to use them.
Signature · 1 line
Chuck Lever
Quoted 33 lines · Benjamin Coddington
The following series enables the linux NFS server to add a Message Authentication Code (MAC) to the filehandles it gives to clients. This provides additional protection to the exported filesystem against filehandle guessing attacks.
Filesystems generate their own filehandles through the
export_operation"encode_fh" and a filehandle provides sufficient access to open a file without needing to perform a lookup. A trusted NFS client holding a valid filehandle can remotely access the corresponding file without reference to access-path restrictions that might be imposed by the ancestor directories or the server exports.In order to acquire a filehandle, you must perform lookup operations on the parent directory(ies), and the permissions on those directories may prohibit you from walking into them to find the files within. This would normally be considered sufficient protection on a local filesystem to prohibit users from accessing those files, however when the filesystem is exported via NFS an exported file can be accessed whenever the NFS server is presented with the correct filehandle, which can be guessed or acquired by means other than LOOKUP.
Filehandles are easy to guess because they are well-formed. The
open_by_handle_at(2)man page contains an example C program (t_name_to_handle_at.c) that can display a filehandle given a path. Here's an example filehandle from a fairly modern XFS:# ./t_name_to_handle_at /exports/foo 57 12 129 99 00 00 00 00 00 00 00 b4 10 0b 8c^--------- filehandle ----------^ ^------- inode -------^ ^-- gen --^
Quoted 7 lines
This filehandle consists of a 64-bit inode number and 32-bit generation number. Because the handle is well-formed, its easy to fabricate filehandles that match other files within the same filesystem. You can simply insert inode numbers and iterate on the generation number. Eventually you'll be able to access the file using
open_by_handle_at(2). For a local system,open_by_handle_at(2)requiresCAP_DAC_READ_SEARCH, which protects against guessing attacks by unprivileged users.
"Simple testing confirms that the correct generation number can be found withing XXX minutes using open_by_handle_at() and it is estimated that adding network delay with genuine NFS calls would only increase this to YYY minutes (ZZZ hours)".
Quoted 28 lines
In contrast to a local user using
open_by_handle(2), the NFS server must permissively allow remote clients to open by filehandle without being able to check or trust the remote caller's access. Therefore additional protection against this attack is needed for NFS case. We propose to sign filehandles by appending an 8-byte MAC which is the siphash of the filehandle from a key set from the nfs-utilities. NFS server can then ensure that guessing a valid filehandle+MAC is practically impossible without knowledge of the MAC's key. The NFS server performs optional signing by possessing a key set from userspace and having the "sign_fh" export option.Because filehandles are long-lived, and there's no method for expiring them, the server's key should be set once and not changed. It also should be persisted across restarts. The methods to set the key allow only setting it once, afterward it cannot be changed. A separate patchset for nfs-utils contains the userspace changes required to set the server's key.
I had planned on adding additional work to enable the server to check whether the 8-byte MAC will overflow maximum filehandle length for the protocol at export time. There could be some filesystems with 40-byte fileid and 24-byte fsid which would break NFSv3's 64-byte filehandle maximum with an 8-byte MAC appended. The server should refuse to export those filesystems when "
sign_fh" is requested. However, the way the export caches work (the server may not even be running when a user sets up the export) its impossible to do this check at export time. Instead, the server will refuse to give out filehandles at mount time and emit apr_warn().
Quoted 1 line
Thanks for any comments and critique.
Thanks,
NeilBrown
Re: [PATCH v2 1/3] NFSD: Add a key for signing filehandles
In reply to Benjamin CoddingtonQuoted 64 lines · Benjamin Coddington
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.Signed-off-by Benjamin Coddington <[email protected]>--- 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(-)Documentation/netlink/specs/nfsd.yaml +6
-0@@ -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 threadsfs/nfsd/netlink.c +2
-1@@ -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 */
fs/nfsd/netlink.c static const struct genl_split_ops nfsd_nl_ops[] = { · line 62
Quoted 5 lines
fs/nfsd/netlink.c +1
-1@@ -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,
Please make this NFSD_A_SERVER_MAX
Quoted 70 lines
fs/nfsd/netlink.c +0
-0.flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, }, {fs/nfsd/netns.h +2
-0@@ -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 */ fs/nfsd/nfsctl.c +18
-0@@ -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. +* +* Input: +* buf: ignored +* size: zero +* OR +* +* Input: +* buf: C string containing a parseable UUID +* size: non-zero length of C string in @buf +* Output: +* On success: passed-in buffer filled with '\n'-terminated C string
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, char *buf, size_t size) · line 980
Quoted 10 lines
fs/nfsd/nfsctl.c +10
-0+* containing the standard UUID format of the server's fh_key +* return code is the size in bytes of the string +* On error: return code is zero or a negative errno value +*/ +static ssize_t write_fh_key (struct file *file,char *buf,size_t size) +{ +struct nfsd_net *nn =net_generic (netns (file), nfsd_net_id); +int ret = -EEXIST ; + +if (size >35 && size <38 ) {
I know Chuck wants you to drop this, so maybe this comment isn't relevant, but those two magic numbers need an explanation. Though
if (size == 36 || size == 37) {would mean the samr thing and be a little clearer.
Thanks,
NeilBrown
Quoted 149 lines
fs/nfsd/nfsctl.c +66
-0+siphash_key_t *sip_fh_key; +uuid_t uuid_fh_key; + +mutex_lock (&nfsd_mutex); + +/* Is the key already set? */ +if (nn->fh_key) + goto out; + + ret =uuid_parse (buf, &uuid_fh_key); +if (ret) + goto out; + + sip_fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!sip_fh_key) { + ret = -ENOMEM ; + goto out; + } + +memcpy (sip_fh_key, &uuid_fh_key,sizeof (siphash_key_t)); + nn->fh_key = sip_fh_key; + } + ret =scnprintf (buf,SIMPLE_TRANSACTION_LIMIT ,"%pUb\n" , nn->fh_key); +out: +mutex_unlock (&nfsd_mutex); +trace_nfsd_ctl_fh_key_set ((const char *)nn->fh_key, ret); +return ret; +} +#ifdef CONFIG_NFSD_V4 static ssize_t __nfsd4_write_time (struct file *file,char *buf,size_t size,time64_t *time,struct nfsd_net *nn)@@ -1343,6 +1400,7 @@ static int nfsd_fill_super(struct super_block *sb, struct fs_context *fc) [NFSD_Ports] = {"portlist", &transaction_ops, S_IWUSR|S_IRUGO}, [NFSD_MaxBlkSize] = {"max_block_size", &transaction_ops, S_IWUSR|S_IRUGO}, [NFSD_MinThreads] = {"min_threads", &transaction_ops, S_IWUSR|S_IRUGO}, + [NFSD_Fh_Key] = {"fh_key", &transaction_ops, S_IWUSR|S_IRUSR}, [NFSD_Filecache] = {"filecache" , &nfsd_file_cache_stats_fops,S_IRUGO }, #ifdef CONFIG_NFSD_V4 [NFSD_Leasetime] = {"nfsv4leasetime", &transaction_ops, S_IWUSR|S_IRUSR},@@ -1615,6 +1673,33 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,return ret; } +/** +* nfsd_nl_fh_key_set - helper to copy fh_key from userspace +* @attr: nlattr NFSD_A_SERVER_FH_KEY +* @nn: nfsd_net +* +* Callers should hold nfsd_mutex, returns 0 on success or negative errno. +*/ +static int nfsd_nl_fh_key_set (const struct nlattr *attr,struct nfsd_net *nn) +{ +siphash_key_t *fh_key; + +if (nla_len (attr) !=sizeof (siphash_key_t)) +return -EINVAL ; + +/* Is the key already set? */ +if (nn->fh_key) +return -EEXIST ; + + fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!fh_key) +return -ENOMEM ; + +memcpy (fh_key,nla_data (attr),sizeof (siphash_key_t)); + nn->fh_key = fh_key; +return 0 ; +} +/** * nfsd_nl_threads_set_doit - set the number of running threads * @skb: reply buffer @@ -1691,6 +1776,14 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)if (attr) nn->min_threads =nla_get_u32 (attr); + 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 && ret != -EEXIST ) + goto out_unlock; + } + ret =nfsd_svc (nrpools, nthreads, net,get_current_cred (), scope);if (ret >0 ) ret =0 ;@@ -2284,6 +2377,7 @@ static __net_exit void nfsd_net_exit(struct net *net) {struct nfsd_net *nn =net_generic (net, nfsd_net_id); +kfree_sensitive (nn->fh_key);nfsd_proc_stat_shutdown (net);percpu_counter_destroy_many (nn->counter,NFSD_STATS_COUNTERS_NUM );nfsd_idmap_shutdown (net);fs/nfsd/trace.h +25
-0@@ -2240,6 +2240,31 @@ TRACE_EVENT(nfsd_end_grace, ) ); +TRACE_EVENT (nfsd_ctl_fh_key_set, +TP_PROTO ( + const char *key, + int result + ), +TP_ARGS (key, result), +TP_STRUCT__entry ( + __array(unsigned char, key, 16) + __field(unsigned long, result) +__field (bool, key_set) + ), +TP_fast_assign ( + __entry->key_set = true; + if (!key) + __entry->key_set = false; + else + memcpy(__entry->key, key, 16); + __entry->result = result; + ), +TP_printk ("key=%s result=%ld" , + __entry->key_set ?__print_hex_str (__entry->key,16 ) :"(null)" , + __entry->result + ) +); + DECLARE_EVENT_CLASS(nfsd_copy_class, TP_PROTO( const struct nfsd4_copy *copyinclude/uapi/linux/nfsd_netlink.h +1
-0@@ -36,6 +36,7 @@ enum {NFSD_A_SERVER_LEASETIME ,NFSD_A_SERVER_SCOPE ,NFSD_A_SERVER_MIN_THREADS , +NFSD_A_SERVER_FH_KEY , __NFSD_A_SERVER_MAX,NFSD_A_SERVER_MAX = (__NFSD_A_SERVER_MAX -1 )Signature · 1 line
2.50.1