| Message ID | 20231012-nfsd-cltrack-v1-1-4e2e405b2b96@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] nfsd: new Kconfig option for legacy client tracking | expand |
On Fri, 13 Oct 2023, 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 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. This is an excellent idea - thanks. I haven't looked closely at the patch, but if it builds then I suspect it is correct. Acked-by: NeilBrown <neilb@suse.de> Thanks, NeilBrown > > 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(-) > > diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig > index 43b88eaf0673..2d21fbf154c6 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..2d21fbf154c6 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
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 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. 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(-) --- base-commit: 401644852d0b2a278811de38081be23f74b5bb04 change-id: 20231012-nfsd-cltrack-6198814aee58 Best regards,