| Message ID | 20231013-nfsd-cltrack-v1-1-5d8a3a6a39af@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: new Kconfig option for legacy client tracking | expand |
On Fri, Oct 13, 2023 at 09:03:53AM -0400, Jeff Layton wrote: > We've had a number of attempts at different NFSv4 client tracking > methods over the years, but now nfsdcld has emerged as the clear winner > since the others (recoverydir and the usermodehelper upcall) are > problematic. > > As a case in point, the recoverydir backend uses MD5 hashes to encode > long form clientid strings, which means that nfsd repeatedly gets dinged > on FIPS audits, since MD5 isn't considered secure. Its use of MD5 is not > cryptographically significant, so there is no danger there, but allowing > us to compile that out allows us to sidestep the issue entirely. > > As a prelude to eventually removing support for these client tracking > methods, add a new Kconfig option that enables them. Mark it deprecated > and make it default to N. > > Acked-by: NeilBrown <neilb@suse.de> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > Now that we've really settled on nfsdcld being the way forward for > NFSv4 client tracking, put the legacy methods under a new Kconfig option > that defaults to off. > > This should make it easier to eventually deprecate this code and remove > it in the future (maybe in v6.10 or so)? > --- > fs/nfsd/Kconfig | 16 +++++++++ > fs/nfsd/nfs4recover.c | 97 +++++++++++++++++++++++++++++++++------------------ > fs/nfsd/nfsctl.c | 6 ++++ > 3 files changed, 85 insertions(+), 34 deletions(-) LGTM. I've kind of closed out 6.7 at this point for anything but fixes. I've applied this to a private "futures" branch, planning for 6.8. If you think this should go in earlier, let me know. > diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig > index 43b88eaf0673..272ab8d5c4d7 100644 > --- a/fs/nfsd/Kconfig > +++ b/fs/nfsd/Kconfig > @@ -158,3 +158,19 @@ config NFSD_V4_SECURITY_LABEL > > If you do not wish to enable fine-grained security labels SELinux or > Smack policies on NFSv4 files, say N. > + > +config NFSD_LEGACY_CLIENT_TRACKING > + bool "Support legacy NFSv4 client tracking methods (DEPRECATED)" > + depends on NFSD_V4 > + default n > + help > + The NFSv4 server needs to store a small amount of information on > + stable storage in order to handle state recovery after reboot. Most > + modern deployments upcall to a userland daemon for this (nfsdcld), > + but older NFS servers may store information directly in a > + recoverydir, or spawn a process directly using a usermodehelper > + upcall. > + > + These legacy client tracking methods have proven to be probelmatic > + and will be removed in the future. Say Y here if you need support > + for them in the interim. > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c > index 3509e73abe1f..2c060e0b1604 100644 > --- a/fs/nfsd/nfs4recover.c > +++ b/fs/nfsd/nfs4recover.c > @@ -66,6 +66,7 @@ struct nfsd4_client_tracking_ops { > static const struct nfsd4_client_tracking_ops nfsd4_cld_tracking_ops; > static const struct nfsd4_client_tracking_ops nfsd4_cld_tracking_ops_v2; > > +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING > /* Globals */ > static char user_recovery_dirname[PATH_MAX] = "/var/lib/nfs/v4recovery"; > > @@ -720,6 +721,7 @@ static const struct nfsd4_client_tracking_ops nfsd4_legacy_tracking_ops = { > .version = 1, > .msglen = 0, > }; > +#endif /* CONFIG_NFSD_LEGACY_CLIENT_TRACKING */ > > /* Globals */ > #define NFSD_PIPE_DIR "nfsd" > @@ -731,8 +733,10 @@ struct cld_net { > spinlock_t cn_lock; > struct list_head cn_list; > unsigned int cn_xid; > - bool cn_has_legacy; > struct crypto_shash *cn_tfm; > +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING > + bool cn_has_legacy; > +#endif > }; > > struct cld_upcall { > @@ -793,7 +797,6 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg, > uint8_t cmd, princhashlen; > struct xdr_netobj name, princhash = { .len = 0, .data = NULL }; > uint16_t namelen; > - struct cld_net *cn = nn->cld_net; > > if (get_user(cmd, &cmsg->cm_cmd)) { > dprintk("%s: error when copying cmd from userspace", __func__); > @@ -833,11 +836,15 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg, > return PTR_ERR(name.data); > name.len = namelen; > } > +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING > if (name.len > 5 && memcmp(name.data, "hash:", 5) == 0) { > + struct cld_net *cn = nn->cld_net; > + > name.len = name.len - 5; > memmove(name.data, name.data + 5, name.len); > cn->cn_has_legacy = true; > } > +#endif > if (!nfs4_client_to_reclaim(name, princhash, nn)) { > kfree(name.data); > kfree(princhash.data); > @@ -1010,7 +1017,9 @@ __nfsd4_init_cld_pipe(struct net *net) > } > > cn->cn_pipe->dentry = dentry; > +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING > cn->cn_has_legacy = false; > +#endif > nn->cld_net = cn; > return 0; > > @@ -1282,10 +1291,6 @@ nfsd4_cld_check(struct nfs4_client *clp) > { > struct nfs4_client_reclaim *crp; > struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id); > - struct cld_net *cn = nn->cld_net; > - int status; > - char dname[HEXDIR_LEN]; > - struct xdr_netobj name; > > /* did we already find that this client is stable? */ > if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags)) > @@ -1296,7 +1301,12 @@ nfsd4_cld_check(struct nfs4_client *clp) > if (crp) > goto found; > > - if (cn->cn_has_legacy) { > +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING > + if (nn->cld_net->cn_has_legacy) { > + int status; > + char dname[HEXDIR_LEN]; > + struct xdr_netobj name; > + > status = nfs4_make_rec_clidname(dname, &clp->cl_name); > if (status) > return -ENOENT; > @@ -1314,6 +1324,7 @@ nfsd4_cld_check(struct nfs4_client *clp) > goto found; > > } > +#endif > return -ENOENT; > found: > crp->cr_clp = clp; > @@ -1327,8 +1338,6 @@ nfsd4_cld_check_v2(struct nfs4_client *clp) > struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id); > struct cld_net *cn = nn->cld_net; > int status; > - char dname[HEXDIR_LEN]; > - struct xdr_netobj name; > struct crypto_shash *tfm = cn->cn_tfm; > struct xdr_netobj cksum; > char *principal = NULL; > @@ -1342,7 +1351,11 @@ nfsd4_cld_check_v2(struct nfs4_client *clp) > if (crp) > goto found; > > +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING > if (cn->cn_has_legacy) { > + struct xdr_netobj name; > + char dname[HEXDIR_LEN]; > + > status = nfs4_make_rec_clidname(dname, &clp->cl_name); > if (status) > return -ENOENT; > @@ -1360,6 +1373,7 @@ nfsd4_cld_check_v2(struct nfs4_client *clp) > goto found; > > } > +#endif > return -ENOENT; > found: > if (crp->cr_princhash.len) { > @@ -1663,6 +1677,7 @@ static const struct nfsd4_client_tracking_ops nfsd4_cld_tracking_ops_v2 = { > .msglen = sizeof(struct cld_msg_v2), > }; > > +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING > /* upcall via usermodehelper */ > static char cltrack_prog[PATH_MAX] = "/sbin/nfsdcltrack"; > module_param_string(cltrack_prog, cltrack_prog, sizeof(cltrack_prog), > @@ -2007,28 +2022,10 @@ static const struct nfsd4_client_tracking_ops nfsd4_umh_tracking_ops = { > .msglen = 0, > }; > > -int > -nfsd4_client_tracking_init(struct net *net) > +static inline int check_for_legacy_methods(int status, struct net *net) > { > - int status; > - struct path path; > struct nfsd_net *nn = net_generic(net, nfsd_net_id); > - > - /* just run the init if it the method is already decided */ > - if (nn->client_tracking_ops) > - goto do_init; > - > - /* First, try to use nfsdcld */ > - nn->client_tracking_ops = &nfsd4_cld_tracking_ops; > - status = nn->client_tracking_ops->init(net); > - if (!status) > - return status; > - if (status != -ETIMEDOUT) { > - nn->client_tracking_ops = &nfsd4_cld_tracking_ops_v0; > - status = nn->client_tracking_ops->init(net); > - if (!status) > - return status; > - } > + struct path path; > > /* > * Next, try the UMH upcall. > @@ -2045,14 +2042,46 @@ nfsd4_client_tracking_init(struct net *net) > nn->client_tracking_ops = &nfsd4_legacy_tracking_ops; > status = kern_path(nfs4_recoverydir(), LOOKUP_FOLLOW, &path); > if (!status) { > - status = d_is_dir(path.dentry); > + status = !d_is_dir(path.dentry); > path_put(&path); > - if (!status) { > - status = -EINVAL; > - goto out; > - } > + if (status) > + return -ENOTDIR; > + status = nn->client_tracking_ops->init(net); > + } > + return status; > +} > +#else > +static inline int check_for_legacy_methods(int status, struct net *net) > +{ > + return status; > +} > +#endif /* CONFIG_LEGACY_NFSD_CLIENT_TRACKING */ > + > +int > +nfsd4_client_tracking_init(struct net *net) > +{ > + struct nfsd_net *nn = net_generic(net, nfsd_net_id); > + int status; > + > + /* just run the init if it the method is already decided */ > + if (nn->client_tracking_ops) > + goto do_init; > + > + /* First, try to use nfsdcld */ > + nn->client_tracking_ops = &nfsd4_cld_tracking_ops; > + status = nn->client_tracking_ops->init(net); > + if (!status) > + return status; > + if (status != -ETIMEDOUT) { > + nn->client_tracking_ops = &nfsd4_cld_tracking_ops_v0; > + status = nn->client_tracking_ops->init(net); > + if (!status) > + return status; > } > > + status = check_for_legacy_methods(status, net); > + if (status) > + goto out; > do_init: > status = nn->client_tracking_ops->init(net); > out: > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c > index 7ed02fb88a36..48d1dc9cccfb 100644 > --- a/fs/nfsd/nfsctl.c > +++ b/fs/nfsd/nfsctl.c > @@ -75,7 +75,9 @@ static ssize_t write_maxconn(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); > +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING > static ssize_t write_recoverydir(struct file *file, char *buf, size_t size); > +#endif > static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size); > #endif > > @@ -92,7 +94,9 @@ static ssize_t (*const write_op[])(struct file *, char *, size_t) = { > #ifdef CONFIG_NFSD_V4 > [NFSD_Leasetime] = write_leasetime, > [NFSD_Gracetime] = write_gracetime, > +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING > [NFSD_RecoveryDir] = write_recoverydir, > +#endif > [NFSD_V4EndGrace] = write_v4_end_grace, > #endif > }; > @@ -1012,6 +1016,7 @@ static ssize_t write_gracetime(struct file *file, char *buf, size_t size) > return nfsd4_write_time(file, buf, size, &nn->nfsd4_grace, nn); > } > > +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING > static ssize_t __write_recoverydir(struct file *file, char *buf, size_t size, > struct nfsd_net *nn) > { > @@ -1072,6 +1077,7 @@ static ssize_t write_recoverydir(struct file *file, char *buf, size_t size) > mutex_unlock(&nfsd_mutex); > return rv; > } > +#endif > > /* > * write_v4_end_grace - release grace period for nfsd's v4.x lock manager > > --- > base-commit: 401644852d0b2a278811de38081be23f74b5bb04 > change-id: 20231012-nfsd-cltrack-6198814aee58 > > Best regards, > -- > Jeff Layton <jlayton@kernel.org> >
diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig index 43b88eaf0673..272ab8d5c4d7 100644 --- a/fs/nfsd/Kconfig +++ b/fs/nfsd/Kconfig @@ -158,3 +158,19 @@ config NFSD_V4_SECURITY_LABEL If you do not wish to enable fine-grained security labels SELinux or Smack policies on NFSv4 files, say N. + +config NFSD_LEGACY_CLIENT_TRACKING + bool "Support legacy NFSv4 client tracking methods (DEPRECATED)" + depends on NFSD_V4 + default n + help + The NFSv4 server needs to store a small amount of information on + stable storage in order to handle state recovery after reboot. Most + modern deployments upcall to a userland daemon for this (nfsdcld), + but older NFS servers may store information directly in a + recoverydir, or spawn a process directly using a usermodehelper + upcall. + + These legacy client tracking methods have proven to be probelmatic + and will be removed in the future. Say Y here if you need support + for them in the interim. diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c index 3509e73abe1f..2c060e0b1604 100644 --- a/fs/nfsd/nfs4recover.c +++ b/fs/nfsd/nfs4recover.c @@ -66,6 +66,7 @@ struct nfsd4_client_tracking_ops { static const struct nfsd4_client_tracking_ops nfsd4_cld_tracking_ops; static const struct nfsd4_client_tracking_ops nfsd4_cld_tracking_ops_v2; +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING /* Globals */ static char user_recovery_dirname[PATH_MAX] = "/var/lib/nfs/v4recovery"; @@ -720,6 +721,7 @@ static const struct nfsd4_client_tracking_ops nfsd4_legacy_tracking_ops = { .version = 1, .msglen = 0, }; +#endif /* CONFIG_NFSD_LEGACY_CLIENT_TRACKING */ /* Globals */ #define NFSD_PIPE_DIR "nfsd" @@ -731,8 +733,10 @@ struct cld_net { spinlock_t cn_lock; struct list_head cn_list; unsigned int cn_xid; - bool cn_has_legacy; struct crypto_shash *cn_tfm; +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING + bool cn_has_legacy; +#endif }; struct cld_upcall { @@ -793,7 +797,6 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg, uint8_t cmd, princhashlen; struct xdr_netobj name, princhash = { .len = 0, .data = NULL }; uint16_t namelen; - struct cld_net *cn = nn->cld_net; if (get_user(cmd, &cmsg->cm_cmd)) { dprintk("%s: error when copying cmd from userspace", __func__); @@ -833,11 +836,15 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg, return PTR_ERR(name.data); name.len = namelen; } +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING if (name.len > 5 && memcmp(name.data, "hash:", 5) == 0) { + struct cld_net *cn = nn->cld_net; + name.len = name.len - 5; memmove(name.data, name.data + 5, name.len); cn->cn_has_legacy = true; } +#endif if (!nfs4_client_to_reclaim(name, princhash, nn)) { kfree(name.data); kfree(princhash.data); @@ -1010,7 +1017,9 @@ __nfsd4_init_cld_pipe(struct net *net) } cn->cn_pipe->dentry = dentry; +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING cn->cn_has_legacy = false; +#endif nn->cld_net = cn; return 0; @@ -1282,10 +1291,6 @@ nfsd4_cld_check(struct nfs4_client *clp) { struct nfs4_client_reclaim *crp; struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id); - struct cld_net *cn = nn->cld_net; - int status; - char dname[HEXDIR_LEN]; - struct xdr_netobj name; /* did we already find that this client is stable? */ if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags)) @@ -1296,7 +1301,12 @@ nfsd4_cld_check(struct nfs4_client *clp) if (crp) goto found; - if (cn->cn_has_legacy) { +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING + if (nn->cld_net->cn_has_legacy) { + int status; + char dname[HEXDIR_LEN]; + struct xdr_netobj name; + status = nfs4_make_rec_clidname(dname, &clp->cl_name); if (status) return -ENOENT; @@ -1314,6 +1324,7 @@ nfsd4_cld_check(struct nfs4_client *clp) goto found; } +#endif return -ENOENT; found: crp->cr_clp = clp; @@ -1327,8 +1338,6 @@ nfsd4_cld_check_v2(struct nfs4_client *clp) struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id); struct cld_net *cn = nn->cld_net; int status; - char dname[HEXDIR_LEN]; - struct xdr_netobj name; struct crypto_shash *tfm = cn->cn_tfm; struct xdr_netobj cksum; char *principal = NULL; @@ -1342,7 +1351,11 @@ nfsd4_cld_check_v2(struct nfs4_client *clp) if (crp) goto found; +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING if (cn->cn_has_legacy) { + struct xdr_netobj name; + char dname[HEXDIR_LEN]; + status = nfs4_make_rec_clidname(dname, &clp->cl_name); if (status) return -ENOENT; @@ -1360,6 +1373,7 @@ nfsd4_cld_check_v2(struct nfs4_client *clp) goto found; } +#endif return -ENOENT; found: if (crp->cr_princhash.len) { @@ -1663,6 +1677,7 @@ static const struct nfsd4_client_tracking_ops nfsd4_cld_tracking_ops_v2 = { .msglen = sizeof(struct cld_msg_v2), }; +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING /* upcall via usermodehelper */ static char cltrack_prog[PATH_MAX] = "/sbin/nfsdcltrack"; module_param_string(cltrack_prog, cltrack_prog, sizeof(cltrack_prog), @@ -2007,28 +2022,10 @@ static const struct nfsd4_client_tracking_ops nfsd4_umh_tracking_ops = { .msglen = 0, }; -int -nfsd4_client_tracking_init(struct net *net) +static inline int check_for_legacy_methods(int status, struct net *net) { - int status; - struct path path; struct nfsd_net *nn = net_generic(net, nfsd_net_id); - - /* just run the init if it the method is already decided */ - if (nn->client_tracking_ops) - goto do_init; - - /* First, try to use nfsdcld */ - nn->client_tracking_ops = &nfsd4_cld_tracking_ops; - status = nn->client_tracking_ops->init(net); - if (!status) - return status; - if (status != -ETIMEDOUT) { - nn->client_tracking_ops = &nfsd4_cld_tracking_ops_v0; - status = nn->client_tracking_ops->init(net); - if (!status) - return status; - } + struct path path; /* * Next, try the UMH upcall. @@ -2045,14 +2042,46 @@ nfsd4_client_tracking_init(struct net *net) nn->client_tracking_ops = &nfsd4_legacy_tracking_ops; status = kern_path(nfs4_recoverydir(), LOOKUP_FOLLOW, &path); if (!status) { - status = d_is_dir(path.dentry); + status = !d_is_dir(path.dentry); path_put(&path); - if (!status) { - status = -EINVAL; - goto out; - } + if (status) + return -ENOTDIR; + status = nn->client_tracking_ops->init(net); + } + return status; +} +#else +static inline int check_for_legacy_methods(int status, struct net *net) +{ + return status; +} +#endif /* CONFIG_LEGACY_NFSD_CLIENT_TRACKING */ + +int +nfsd4_client_tracking_init(struct net *net) +{ + struct nfsd_net *nn = net_generic(net, nfsd_net_id); + int status; + + /* just run the init if it the method is already decided */ + if (nn->client_tracking_ops) + goto do_init; + + /* First, try to use nfsdcld */ + nn->client_tracking_ops = &nfsd4_cld_tracking_ops; + status = nn->client_tracking_ops->init(net); + if (!status) + return status; + if (status != -ETIMEDOUT) { + nn->client_tracking_ops = &nfsd4_cld_tracking_ops_v0; + status = nn->client_tracking_ops->init(net); + if (!status) + return status; } + status = check_for_legacy_methods(status, net); + if (status) + goto out; do_init: status = nn->client_tracking_ops->init(net); out: diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c index 7ed02fb88a36..48d1dc9cccfb 100644 --- a/fs/nfsd/nfsctl.c +++ b/fs/nfsd/nfsctl.c @@ -75,7 +75,9 @@ static ssize_t write_maxconn(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); +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING static ssize_t write_recoverydir(struct file *file, char *buf, size_t size); +#endif static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size); #endif @@ -92,7 +94,9 @@ static ssize_t (*const write_op[])(struct file *, char *, size_t) = { #ifdef CONFIG_NFSD_V4 [NFSD_Leasetime] = write_leasetime, [NFSD_Gracetime] = write_gracetime, +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING [NFSD_RecoveryDir] = write_recoverydir, +#endif [NFSD_V4EndGrace] = write_v4_end_grace, #endif }; @@ -1012,6 +1016,7 @@ static ssize_t write_gracetime(struct file *file, char *buf, size_t size) return nfsd4_write_time(file, buf, size, &nn->nfsd4_grace, nn); } +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING static ssize_t __write_recoverydir(struct file *file, char *buf, size_t size, struct nfsd_net *nn) { @@ -1072,6 +1077,7 @@ static ssize_t write_recoverydir(struct file *file, char *buf, size_t size) mutex_unlock(&nfsd_mutex); return rv; } +#endif /* * write_v4_end_grace - release grace period for nfsd's v4.x lock manager