[v5,2/3] NFSD: convert write_version to netlink command

Commit Message

Lorenzo Bianconi Nov. 29, 2023, 5:12 p.m. UTC

Introduce write_version netlink command similar to the ones available
through the procfs.

Signed-off-by: Lorenzo Bianconi <[e-mail]>
---
 Documentation/netlink/specs/nfsd.yaml |  32 ++++++++
 fs/nfsd/netlink.c                     |  19 +++++
 fs/nfsd/netlink.h                     |   3 +
 fs/nfsd/nfsctl.c                      | 105 ++++++++++++++++++++++++++
 include/uapi/linux/nfsd_netlink.h     |  11 +++
 tools/net/ynl/generated/nfsd-user.c   |  81 ++++++++++++++++++++
 tools/net/ynl/generated/nfsd-user.h   |  55 ++++++++++++++
 7 files changed, 306 insertions(+)

Comments

Jeff Layton Nov. 29, 2023, 6:23 p.m. UTC | #1

On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> Introduce write_version netlink command similar to the ones available
> through the procfs.
> 
> Signed-off-by: Lorenzo Bianconi <[e-mail]>
> ---
>  Documentation/netlink/specs/nfsd.yaml |  32 ++++++++
>  fs/nfsd/netlink.c                     |  19 +++++
>  fs/nfsd/netlink.h                     |   3 +
>  fs/nfsd/nfsctl.c                      | 105 ++++++++++++++++++++++++++
>  include/uapi/linux/nfsd_netlink.h     |  11 +++
>  tools/net/ynl/generated/nfsd-user.c   |  81 ++++++++++++++++++++
>  tools/net/ynl/generated/nfsd-user.h   |  55 ++++++++++++++
>  7 files changed, 306 insertions(+)
> 
> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> index c92e1425d316..6c5e42bb20f6 100644
> --- a/Documentation/netlink/specs/nfsd.yaml
> +++ b/Documentation/netlink/specs/nfsd.yaml
> @@ -68,6 +68,18 @@ attribute-sets:
>        -
>          name: threads
>          type: u32
> +  -
> +    name: server-version
> +    attributes:
> +      -
> +        name: major
> +        type: u32
> +      -
> +        name: minor
> +        type: u32
> +      -
> +        name: status
> +        type: u8
>  
>  operations:
>    list:
> @@ -110,3 +122,23 @@ operations:
>          reply:
>            attributes:
>              - threads
> +    -
> +      name: version-set
> +      doc: enable/disable server version
> +      attribute-set: server-version
> +      flags: [ admin-perm ]
> +      do:
> +        request:
> +          attributes:
> +            - major
> +            - minor
> +            - status
> +    -
> +      name: version-get
> +      doc: dump server versions
> +      attribute-set: server-version
> +      dump:
> +        reply:
> +          attributes:
> +            - major
> +            - minor
> diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> index 1a59a8e6c7e2..0608a7bd193b 100644
> --- a/fs/nfsd/netlink.c
> +++ b/fs/nfsd/netlink.c
> @@ -15,6 +15,13 @@ static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
>  	[NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
>  };
>  
> +/* NFSD_CMD_VERSION_SET - do */
> +static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
> +	[NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
> +	[NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
> +	[NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> +};
> +
>  /* Ops table for nfsd */
>  static const struct genl_split_ops nfsd_nl_ops[] = {
>  	{
> @@ -36,6 +43,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
>  		.doit	= nfsd_nl_threads_get_doit,
>  		.flags	= GENL_CMD_CAP_DO,
>  	},
> +	{
> +		.cmd		= NFSD_CMD_VERSION_SET,
> +		.doit		= nfsd_nl_version_set_doit,
> +		.policy		= nfsd_version_set_nl_policy,
> +		.maxattr	= NFSD_A_SERVER_VERSION_STATUS,
> +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> +	},
> +	{
> +		.cmd	= NFSD_CMD_VERSION_GET,
> +		.dumpit	= nfsd_nl_version_get_dumpit,
> +		.flags	= GENL_CMD_CAP_DUMP,
> +	},
>  };
>  
>  struct genl_family nfsd_nl_family __ro_after_init = {
> diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h
> index 4137fac477e4..7d203cec08e4 100644
> --- a/fs/nfsd/netlink.h
> +++ b/fs/nfsd/netlink.h
> @@ -18,6 +18,9 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
>  				  struct netlink_callback *cb);
>  int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
>  int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> +			       struct netlink_callback *cb);
>  
>  extern struct genl_family nfsd_nl_family;
>  
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 130b3d937a79..f04430f79687 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1757,6 +1757,111 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
>  	return err;
>  }
>  
> +/**
> + * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
> + * @skb: reply buffer
> + * @info: netlink metadata and command arguments
> + *
> + * Return 0 on success or a negative errno.
> + */
> +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> +	enum vers_op cmd;
> +	u32 major, minor;
> +	u8 status;
> +	int ret;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
> +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
> +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
> +		return -EINVAL;
> +
> +	major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
> +	minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
> +
> +	status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
> +	cmd = !!status ? NFSD_SET : NFSD_CLEAR;
> +
> +	mutex_lock(&nfsd_mutex);
> +	switch (major) {
> +	case 4:
> +		ret = nfsd_minorversion(nn, minor, cmd);
> +		break;
> +	case 2:
> +	case 3:
> +		if (!minor) {
> +			ret = nfsd_vers(nn, major, cmd);
> +			break;
> +		}
> +		fallthrough;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +	mutex_unlock(&nfsd_mutex);
> +
> +	return ret;
> +}
> +
> +/**
> + * nfsd_nl_version_get_doit - Handle verion_get dumpit
> + * @skb: reply buffer
> + * @cb: netlink metadata and command arguments
> + *
> + * Returns the size of the reply or a negative errno.
> + */
> +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> +			       struct netlink_callback *cb)
> +{
> +	struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> +	int i, ret = -ENOMEM;
> +
> +	mutex_lock(&nfsd_mutex);
> +
> +	for (i = 2; i <= 4; i++) {
> +		int j;
> +
> +		if (i < cb->args[0]) /* already consumed */
> +			continue;
> +
> +		if (!nfsd_vers(nn, i, NFSD_AVAIL))
> +			continue;
> +
> +		for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
> +			void *hdr;
> +
> +			if (!nfsd_vers(nn, i, NFSD_TEST))
> +				continue;
> +
> +			/* NFSv{2,3} does not support minor numbers */
> +			if (i < 4 && j)
> +				continue;
> +
> +			if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> +				continue;
> +
> +			hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> +					  cb->nlh->nlmsg_seq, &nfsd_nl_family,
> +					  0, NFSD_CMD_VERSION_GET);
> +			if (!hdr)
> +				goto out;
> +
> +			if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> +			    nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> +				goto out;
> +
> +			genlmsg_end(skb, hdr);
> +		}
> +	}
> +	cb->args[0] = i;
> +	ret = skb->len;
> +out:
> +	mutex_unlock(&nfsd_mutex);
> +
> +	return ret;
> +}
> +
>  /**
>   * nfsd_net_init - Prepare the nfsd_net portion of a new net namespace
>   * @net: a freshly-created network namespace
> diff --git a/include/uapi/linux/nfsd_netlink.h b/include/uapi/linux/nfsd_netlink.h
> index 1b6fe1f9ed0e..1b3340f31baa 100644
> --- a/include/uapi/linux/nfsd_netlink.h
> +++ b/include/uapi/linux/nfsd_netlink.h
> @@ -36,10 +36,21 @@ enum {
>  	NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
>  };
>  
> +enum {
> +	NFSD_A_SERVER_VERSION_MAJOR = 1,
> +	NFSD_A_SERVER_VERSION_MINOR,
> +	NFSD_A_SERVER_VERSION_STATUS,
> +
> +	__NFSD_A_SERVER_VERSION_MAX,
> +	NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> +};
> +
>  enum {
>  	NFSD_CMD_RPC_STATUS_GET = 1,
>  	NFSD_CMD_THREADS_SET,
>  	NFSD_CMD_THREADS_GET,
> +	NFSD_CMD_VERSION_SET,
> +	NFSD_CMD_VERSION_GET,
>  
>  	__NFSD_CMD_MAX,
>  	NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> index 9768328a7751..4cb71c3cd18d 100644
> --- a/tools/net/ynl/generated/nfsd-user.c
> +++ b/tools/net/ynl/generated/nfsd-user.c
> @@ -17,6 +17,8 @@ static const char * const nfsd_op_strmap[] = {
>  	[NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
>  	[NFSD_CMD_THREADS_SET] = "threads-set",
>  	[NFSD_CMD_THREADS_GET] = "threads-get",
> +	[NFSD_CMD_VERSION_SET] = "version-set",
> +	[NFSD_CMD_VERSION_GET] = "version-get",
>  };
>  
>  const char *nfsd_op_str(int op)
> @@ -58,6 +60,17 @@ struct ynl_policy_nest nfsd_server_worker_nest = {
>  	.table = nfsd_server_worker_policy,
>  };
>  
> +struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
> +	[NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
> +	[NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
> +	[NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
> +};
> +
> +struct ynl_policy_nest nfsd_server_version_nest = {
> +	.max_attr = NFSD_A_SERVER_VERSION_MAX,
> +	.table = nfsd_server_version_policy,
> +};
> +
>  /* Common nested types */
>  /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
>  /* NFSD_CMD_RPC_STATUS_GET - dump */
> @@ -290,6 +303,74 @@ struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
>  	return NULL;
>  }
>  
> +/* ============== NFSD_CMD_VERSION_SET ============== */
> +/* NFSD_CMD_VERSION_SET - do */
> +void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
> +{
> +	free(req);
> +}
> +
> +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
> +{
> +	struct nlmsghdr *nlh;
> +	int err;
> +
> +	nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
> +	ys->req_policy = &nfsd_server_version_nest;
> +
> +	if (req->_present.major)
> +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
> +	if (req->_present.minor)
> +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
> +	if (req->_present.status)
> +		mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
> +
> +	err = ynl_exec(ys, nlh, NULL);
> +	if (err < 0)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +/* ============== NFSD_CMD_VERSION_GET ============== */
> +/* NFSD_CMD_VERSION_GET - dump */
> +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
> +{
> +	struct nfsd_version_get_list *next = rsp;
> +
> +	while ((void *)next != YNL_LIST_END) {
> +		rsp = next;
> +		next = rsp->next;
> +
> +		free(rsp);
> +	}
> +}
> +
> +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> +{
> +	struct ynl_dump_state yds = {};
> +	struct nlmsghdr *nlh;
> +	int err;
> +
> +	yds.ys = ys;
> +	yds.alloc_sz = sizeof(struct nfsd_version_get_list);
> +	yds.cb = nfsd_version_get_rsp_parse;
> +	yds.rsp_cmd = NFSD_CMD_VERSION_GET;
> +	yds.rsp_policy = &nfsd_server_version_nest;
> +
> +	nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
> +
> +	err = ynl_exec_dump(ys, nlh, &yds);
> +	if (err < 0)
> +		goto free_list;
> +
> +	return yds.first;
> +
> +free_list:
> +	nfsd_version_get_list_free(yds.first);
> +	return NULL;
> +}
> +
>  const struct ynl_family ynl_nfsd_family =  {
>  	.name		= "nfsd",
>  };
> diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> index e162a4f20d91..e61c5a9e46fb 100644
> --- a/tools/net/ynl/generated/nfsd-user.h
> +++ b/tools/net/ynl/generated/nfsd-user.h
> @@ -111,4 +111,59 @@ void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
>   */
>  struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
>  
> +/* ============== NFSD_CMD_VERSION_SET ============== */
> +/* NFSD_CMD_VERSION_SET - do */
> +struct nfsd_version_set_req {
> +	struct {
> +		__u32 major:1;
> +		__u32 minor:1;
> +		__u32 status:1;
> +	} _present;
> +
> +	__u32 major;
> +	__u32 minor;
> +	__u8 status;
> +};
> +

This more or less mirrors how the "versions" file works today, but that
interface is quite klunky.  We don't have a use case that requires that
we do this piecemeal like this. I think we'd be better served with a
more declarative interface that reconfigures the supported versions in
one shot:

Instead of having "major,minor,status" and potentially having to call
this command several times from userland, it seems like it would be
nicer to just have userland send down a list "major,minor" that should
be enabled, and then just let the kernel figure out whether to enable or
disable each. An empty list could mean "disable everything".

That's simpler to reason out as an interface from userland too. Trying
to keep track of the enabled and disabled versions and twiddle it is
really tricky in rpc.nfsd today.
 
> +static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
> +{
> +	return calloc(1, sizeof(struct nfsd_version_set_req));
> +}
> +void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
> +
> +static inline void
> +nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
> +{
> +	req->_present.major = 1;
> +	req->major = major;
> +}
> +static inline void
> +nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
> +{
> +	req->_present.minor = 1;
> +	req->minor = minor;
> +}
> +static inline void
> +nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
> +{
> +	req->_present.status = 1;
> +	req->status = status;
> +}
> +
> +/*
> + * enable/disable server version
> + */
> +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
> +
> +/* ============== NFSD_CMD_VERSION_GET ============== */
> +/* NFSD_CMD_VERSION_GET - dump */
> +struct nfsd_version_get_list {
> +	struct nfsd_version_get_list *next;
> +	struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
> +};
> +
> +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
> +
> +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
> +
>  #endif /* _LINUX_NFSD_GEN_H */

Jakub Kicinski Nov. 30, 2023, 12:28 a.m. UTC | #2

On Wed, 29 Nov 2023 18:12:44 +0100 Lorenzo Bianconi wrote:
> +      -
> +        name: status
> +        type: u8

u8? I guess...

> +/**
> + * nfsd_nl_version_get_doit - Handle verion_get dumpit

doesn't match the function name (do -> dump)

> +			/* NFSv{2,3} does not support minor numbers */
> +			if (i < 4 && j)
> +				continue;
> +
> +			if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> +				continue;
> +
> +			hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> +					  cb->nlh->nlmsg_seq, &nfsd_nl_family,
> +					  0, NFSD_CMD_VERSION_GET);

Why not iput()?

> +			if (!hdr)
> +				goto out;
> +
> +			if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> +			    nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> +				goto out;
> +
> +			genlmsg_end(skb, hdr);
> +		}
> +	}

Lorenzo Bianconi Nov. 30, 2023, 9:45 a.m. UTC | #3

> On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> > Introduce write_version netlink command similar to the ones available
> > through the procfs.
> > 
> > Signed-off-by: Lorenzo Bianconi <[e-mail]>
> > ---
> >  Documentation/netlink/specs/nfsd.yaml |  32 ++++++++
> >  fs/nfsd/netlink.c                     |  19 +++++
> >  fs/nfsd/netlink.h                     |   3 +
> >  fs/nfsd/nfsctl.c                      | 105 ++++++++++++++++++++++++++
> >  include/uapi/linux/nfsd_netlink.h     |  11 +++
> >  tools/net/ynl/generated/nfsd-user.c   |  81 ++++++++++++++++++++
> >  tools/net/ynl/generated/nfsd-user.h   |  55 ++++++++++++++
> >  7 files changed, 306 insertions(+)
> > 
> > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > index c92e1425d316..6c5e42bb20f6 100644
> > --- a/Documentation/netlink/specs/nfsd.yaml
> > +++ b/Documentation/netlink/specs/nfsd.yaml
> > @@ -68,6 +68,18 @@ attribute-sets:
> >        -
> >          name: threads
> >          type: u32
> > +  -
> > +    name: server-version
> > +    attributes:
> > +      -
> > +        name: major
> > +        type: u32
> > +      -
> > +        name: minor
> > +        type: u32
> > +      -
> > +        name: status
> > +        type: u8
> >  
> >  operations:
> >    list:
> > @@ -110,3 +122,23 @@ operations:
> >          reply:
> >            attributes:
> >              - threads
> > +    -
> > +      name: version-set
> > +      doc: enable/disable server version
> > +      attribute-set: server-version
> > +      flags: [ admin-perm ]
> > +      do:
> > +        request:
> > +          attributes:
> > +            - major
> > +            - minor
> > +            - status
> > +    -
> > +      name: version-get
> > +      doc: dump server versions
> > +      attribute-set: server-version
> > +      dump:
> > +        reply:
> > +          attributes:
> > +            - major
> > +            - minor
> > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > index 1a59a8e6c7e2..0608a7bd193b 100644
> > --- a/fs/nfsd/netlink.c
> > +++ b/fs/nfsd/netlink.c
> > @@ -15,6 +15,13 @@ static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
> >  	[NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
> >  };
> >  
> > +/* NFSD_CMD_VERSION_SET - do */
> > +static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
> > +	[NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
> > +	[NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
> > +	[NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> > +};
> > +
> >  /* Ops table for nfsd */
> >  static const struct genl_split_ops nfsd_nl_ops[] = {
> >  	{
> > @@ -36,6 +43,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> >  		.doit	= nfsd_nl_threads_get_doit,
> >  		.flags	= GENL_CMD_CAP_DO,
> >  	},
> > +	{
> > +		.cmd		= NFSD_CMD_VERSION_SET,
> > +		.doit		= nfsd_nl_version_set_doit,
> > +		.policy		= nfsd_version_set_nl_policy,
> > +		.maxattr	= NFSD_A_SERVER_VERSION_STATUS,
> > +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > +	},
> > +	{
> > +		.cmd	= NFSD_CMD_VERSION_GET,
> > +		.dumpit	= nfsd_nl_version_get_dumpit,
> > +		.flags	= GENL_CMD_CAP_DUMP,
> > +	},
> >  };
> >  
> >  struct genl_family nfsd_nl_family __ro_after_init = {
> > diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h
> > index 4137fac477e4..7d203cec08e4 100644
> > --- a/fs/nfsd/netlink.h
> > +++ b/fs/nfsd/netlink.h
> > @@ -18,6 +18,9 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> >  				  struct netlink_callback *cb);
> >  int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> >  int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > +			       struct netlink_callback *cb);
> >  
> >  extern struct genl_family nfsd_nl_family;
> >  
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 130b3d937a79..f04430f79687 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -1757,6 +1757,111 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> >  	return err;
> >  }
> >  
> > +/**
> > + * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
> > + * @skb: reply buffer
> > + * @info: netlink metadata and command arguments
> > + *
> > + * Return 0 on success or a negative errno.
> > + */
> > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
> > +{
> > +	struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > +	enum vers_op cmd;
> > +	u32 major, minor;
> > +	u8 status;
> > +	int ret;
> > +
> > +	if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
> > +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
> > +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
> > +		return -EINVAL;
> > +
> > +	major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
> > +	minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
> > +
> > +	status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
> > +	cmd = !!status ? NFSD_SET : NFSD_CLEAR;
> > +
> > +	mutex_lock(&nfsd_mutex);
> > +	switch (major) {
> > +	case 4:
> > +		ret = nfsd_minorversion(nn, minor, cmd);
> > +		break;
> > +	case 2:
> > +	case 3:
> > +		if (!minor) {
> > +			ret = nfsd_vers(nn, major, cmd);
> > +			break;
> > +		}
> > +		fallthrough;
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +	mutex_unlock(&nfsd_mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * nfsd_nl_version_get_doit - Handle verion_get dumpit
> > + * @skb: reply buffer
> > + * @cb: netlink metadata and command arguments
> > + *
> > + * Returns the size of the reply or a negative errno.
> > + */
> > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > +			       struct netlink_callback *cb)
> > +{
> > +	struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> > +	int i, ret = -ENOMEM;
> > +
> > +	mutex_lock(&nfsd_mutex);
> > +
> > +	for (i = 2; i <= 4; i++) {
> > +		int j;
> > +
> > +		if (i < cb->args[0]) /* already consumed */
> > +			continue;
> > +
> > +		if (!nfsd_vers(nn, i, NFSD_AVAIL))
> > +			continue;
> > +
> > +		for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
> > +			void *hdr;
> > +
> > +			if (!nfsd_vers(nn, i, NFSD_TEST))
> > +				continue;
> > +
> > +			/* NFSv{2,3} does not support minor numbers */
> > +			if (i < 4 && j)
> > +				continue;
> > +
> > +			if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> > +				continue;
> > +
> > +			hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > +					  cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > +					  0, NFSD_CMD_VERSION_GET);
> > +			if (!hdr)
> > +				goto out;
> > +
> > +			if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> > +			    nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> > +				goto out;
> > +
> > +			genlmsg_end(skb, hdr);
> > +		}
> > +	}
> > +	cb->args[0] = i;
> > +	ret = skb->len;
> > +out:
> > +	mutex_unlock(&nfsd_mutex);
> > +
> > +	return ret;
> > +}
> > +
> >  /**
> >   * nfsd_net_init - Prepare the nfsd_net portion of a new net namespace
> >   * @net: a freshly-created network namespace
> > diff --git a/include/uapi/linux/nfsd_netlink.h b/include/uapi/linux/nfsd_netlink.h
> > index 1b6fe1f9ed0e..1b3340f31baa 100644
> > --- a/include/uapi/linux/nfsd_netlink.h
> > +++ b/include/uapi/linux/nfsd_netlink.h
> > @@ -36,10 +36,21 @@ enum {
> >  	NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
> >  };
> >  
> > +enum {
> > +	NFSD_A_SERVER_VERSION_MAJOR = 1,
> > +	NFSD_A_SERVER_VERSION_MINOR,
> > +	NFSD_A_SERVER_VERSION_STATUS,
> > +
> > +	__NFSD_A_SERVER_VERSION_MAX,
> > +	NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> > +};
> > +
> >  enum {
> >  	NFSD_CMD_RPC_STATUS_GET = 1,
> >  	NFSD_CMD_THREADS_SET,
> >  	NFSD_CMD_THREADS_GET,
> > +	NFSD_CMD_VERSION_SET,
> > +	NFSD_CMD_VERSION_GET,
> >  
> >  	__NFSD_CMD_MAX,
> >  	NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> > index 9768328a7751..4cb71c3cd18d 100644
> > --- a/tools/net/ynl/generated/nfsd-user.c
> > +++ b/tools/net/ynl/generated/nfsd-user.c
> > @@ -17,6 +17,8 @@ static const char * const nfsd_op_strmap[] = {
> >  	[NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
> >  	[NFSD_CMD_THREADS_SET] = "threads-set",
> >  	[NFSD_CMD_THREADS_GET] = "threads-get",
> > +	[NFSD_CMD_VERSION_SET] = "version-set",
> > +	[NFSD_CMD_VERSION_GET] = "version-get",
> >  };
> >  
> >  const char *nfsd_op_str(int op)
> > @@ -58,6 +60,17 @@ struct ynl_policy_nest nfsd_server_worker_nest = {
> >  	.table = nfsd_server_worker_policy,
> >  };
> >  
> > +struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
> > +	[NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
> > +	[NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
> > +	[NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
> > +};
> > +
> > +struct ynl_policy_nest nfsd_server_version_nest = {
> > +	.max_attr = NFSD_A_SERVER_VERSION_MAX,
> > +	.table = nfsd_server_version_policy,
> > +};
> > +
> >  /* Common nested types */
> >  /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> >  /* NFSD_CMD_RPC_STATUS_GET - dump */
> > @@ -290,6 +303,74 @@ struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
> >  	return NULL;
> >  }
> >  
> > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > +/* NFSD_CMD_VERSION_SET - do */
> > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
> > +{
> > +	free(req);
> > +}
> > +
> > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
> > +{
> > +	struct nlmsghdr *nlh;
> > +	int err;
> > +
> > +	nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
> > +	ys->req_policy = &nfsd_server_version_nest;
> > +
> > +	if (req->_present.major)
> > +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
> > +	if (req->_present.minor)
> > +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
> > +	if (req->_present.status)
> > +		mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
> > +
> > +	err = ynl_exec(ys, nlh, NULL);
> > +	if (err < 0)
> > +		return -1;
> > +
> > +	return 0;
> > +}
> > +
> > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > +/* NFSD_CMD_VERSION_GET - dump */
> > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
> > +{
> > +	struct nfsd_version_get_list *next = rsp;
> > +
> > +	while ((void *)next != YNL_LIST_END) {
> > +		rsp = next;
> > +		next = rsp->next;
> > +
> > +		free(rsp);
> > +	}
> > +}
> > +
> > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> > +{
> > +	struct ynl_dump_state yds = {};
> > +	struct nlmsghdr *nlh;
> > +	int err;
> > +
> > +	yds.ys = ys;
> > +	yds.alloc_sz = sizeof(struct nfsd_version_get_list);
> > +	yds.cb = nfsd_version_get_rsp_parse;
> > +	yds.rsp_cmd = NFSD_CMD_VERSION_GET;
> > +	yds.rsp_policy = &nfsd_server_version_nest;
> > +
> > +	nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
> > +
> > +	err = ynl_exec_dump(ys, nlh, &yds);
> > +	if (err < 0)
> > +		goto free_list;
> > +
> > +	return yds.first;
> > +
> > +free_list:
> > +	nfsd_version_get_list_free(yds.first);
> > +	return NULL;
> > +}
> > +
> >  const struct ynl_family ynl_nfsd_family =  {
> >  	.name		= "nfsd",
> >  };
> > diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> > index e162a4f20d91..e61c5a9e46fb 100644
> > --- a/tools/net/ynl/generated/nfsd-user.h
> > +++ b/tools/net/ynl/generated/nfsd-user.h
> > @@ -111,4 +111,59 @@ void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
> >   */
> >  struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
> >  
> > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > +/* NFSD_CMD_VERSION_SET - do */
> > +struct nfsd_version_set_req {
> > +	struct {
> > +		__u32 major:1;
> > +		__u32 minor:1;
> > +		__u32 status:1;
> > +	} _present;
> > +
> > +	__u32 major;
> > +	__u32 minor;
> > +	__u8 status;
> > +};
> > +
> 
> This more or less mirrors how the "versions" file works today, but that
> interface is quite klunky.  We don't have a use case that requires that
> we do this piecemeal like this. I think we'd be better served with a
> more declarative interface that reconfigures the supported versions in
> one shot:
> 
> Instead of having "major,minor,status" and potentially having to call
> this command several times from userland, it seems like it would be
> nicer to just have userland send down a list "major,minor" that should
> be enabled, and then just let the kernel figure out whether to enable or
> disable each. An empty list could mean "disable everything".
> 
> That's simpler to reason out as an interface from userland too. Trying
> to keep track of the enabled and disabled versions and twiddle it is
> really tricky in rpc.nfsd today.

Ack. So far I have just converted the current implementation to netlink
and I have not changed any logic. Anyway I am fine to change the current
logic. Should we have 2 rpc.nfsd version in this case?

Regards,
Lorenzo

>  
> > +static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
> > +{
> > +	return calloc(1, sizeof(struct nfsd_version_set_req));
> > +}
> > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
> > +
> > +static inline void
> > +nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
> > +{
> > +	req->_present.major = 1;
> > +	req->major = major;
> > +}
> > +static inline void
> > +nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
> > +{
> > +	req->_present.minor = 1;
> > +	req->minor = minor;
> > +}
> > +static inline void
> > +nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
> > +{
> > +	req->_present.status = 1;
> > +	req->status = status;
> > +}
> > +
> > +/*
> > + * enable/disable server version
> > + */
> > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
> > +
> > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > +/* NFSD_CMD_VERSION_GET - dump */
> > +struct nfsd_version_get_list {
> > +	struct nfsd_version_get_list *next;
> > +	struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
> > +};
> > +
> > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
> > +
> > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
> > +
> >  #endif /* _LINUX_NFSD_GEN_H */
> 
> -- 
> Jeff Layton <[e-mail]>

Lorenzo Bianconi Nov. 30, 2023, 10:32 a.m. UTC | #4

> On Wed, 29 Nov 2023 18:12:44 +0100 Lorenzo Bianconi wrote:
> > +      -
> > +        name: status
> > +        type: u8
> 
> u8? I guess...

here we need just 0 or 1 I would say. Do you suggest create something like an
enum?

> > +/**
> > + * nfsd_nl_version_get_doit - Handle verion_get dumpit
> 
> doesn't match the function name (do -> dump)

ack, I will fix it.

> 
> > +			/* NFSv{2,3} does not support minor numbers */
> > +			if (i < 4 && j)
> > +				continue;
> > +
> > +			if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> > +				continue;
> > +
> > +			hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > +					  cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > +					  0, NFSD_CMD_VERSION_GET);
> 
> Why not iput()?

ack, I will fix it.

Regards,
Lorenzo

> 
> > +			if (!hdr)
> > +				goto out;
> > +
> > +			if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> > +			    nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> > +				goto out;
> > +
> > +			genlmsg_end(skb, hdr);
> > +		}
> > +	}
>

Chuck Lever Nov. 30, 2023, 4:11 p.m. UTC | #5

On Wed, Nov 29, 2023 at 01:23:37PM -0500, Jeff Layton wrote:
> On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> > Introduce write_version netlink command similar to the ones available
> > through the procfs.
> > 
> > Signed-off-by: Lorenzo Bianconi <[e-mail]>
> > ---
> >  Documentation/netlink/specs/nfsd.yaml |  32 ++++++++
> >  fs/nfsd/netlink.c                     |  19 +++++
> >  fs/nfsd/netlink.h                     |   3 +
> >  fs/nfsd/nfsctl.c                      | 105 ++++++++++++++++++++++++++
> >  include/uapi/linux/nfsd_netlink.h     |  11 +++
> >  tools/net/ynl/generated/nfsd-user.c   |  81 ++++++++++++++++++++
> >  tools/net/ynl/generated/nfsd-user.h   |  55 ++++++++++++++
> >  7 files changed, 306 insertions(+)
> > 
> > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > index c92e1425d316..6c5e42bb20f6 100644
> > --- a/Documentation/netlink/specs/nfsd.yaml
> > +++ b/Documentation/netlink/specs/nfsd.yaml
> > @@ -68,6 +68,18 @@ attribute-sets:
> >        -
> >          name: threads
> >          type: u32
> > +  -
> > +    name: server-version
> > +    attributes:
> > +      -
> > +        name: major
> > +        type: u32
> > +      -
> > +        name: minor
> > +        type: u32
> > +      -
> > +        name: status
> > +        type: u8
> >  
> >  operations:
> >    list:
> > @@ -110,3 +122,23 @@ operations:
> >          reply:
> >            attributes:
> >              - threads
> > +    -
> > +      name: version-set
> > +      doc: enable/disable server version
> > +      attribute-set: server-version
> > +      flags: [ admin-perm ]
> > +      do:
> > +        request:
> > +          attributes:
> > +            - major
> > +            - minor
> > +            - status
> > +    -
> > +      name: version-get
> > +      doc: dump server versions
> > +      attribute-set: server-version
> > +      dump:
> > +        reply:
> > +          attributes:
> > +            - major
> > +            - minor
> > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > index 1a59a8e6c7e2..0608a7bd193b 100644
> > --- a/fs/nfsd/netlink.c
> > +++ b/fs/nfsd/netlink.c
> > @@ -15,6 +15,13 @@ static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
> >  	[NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
> >  };
> >  
> > +/* NFSD_CMD_VERSION_SET - do */
> > +static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
> > +	[NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
> > +	[NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
> > +	[NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> > +};
> > +
> >  /* Ops table for nfsd */
> >  static const struct genl_split_ops nfsd_nl_ops[] = {
> >  	{
> > @@ -36,6 +43,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> >  		.doit	= nfsd_nl_threads_get_doit,
> >  		.flags	= GENL_CMD_CAP_DO,
> >  	},
> > +	{
> > +		.cmd		= NFSD_CMD_VERSION_SET,
> > +		.doit		= nfsd_nl_version_set_doit,
> > +		.policy		= nfsd_version_set_nl_policy,
> > +		.maxattr	= NFSD_A_SERVER_VERSION_STATUS,
> > +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > +	},
> > +	{
> > +		.cmd	= NFSD_CMD_VERSION_GET,
> > +		.dumpit	= nfsd_nl_version_get_dumpit,
> > +		.flags	= GENL_CMD_CAP_DUMP,
> > +	},
> >  };
> >  
> >  struct genl_family nfsd_nl_family __ro_after_init = {
> > diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h
> > index 4137fac477e4..7d203cec08e4 100644
> > --- a/fs/nfsd/netlink.h
> > +++ b/fs/nfsd/netlink.h
> > @@ -18,6 +18,9 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> >  				  struct netlink_callback *cb);
> >  int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> >  int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > +			       struct netlink_callback *cb);
> >  
> >  extern struct genl_family nfsd_nl_family;
> >  
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 130b3d937a79..f04430f79687 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -1757,6 +1757,111 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> >  	return err;
> >  }
> >  
> > +/**
> > + * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
> > + * @skb: reply buffer
> > + * @info: netlink metadata and command arguments
> > + *
> > + * Return 0 on success or a negative errno.
> > + */
> > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
> > +{
> > +	struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > +	enum vers_op cmd;
> > +	u32 major, minor;
> > +	u8 status;
> > +	int ret;
> > +
> > +	if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
> > +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
> > +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
> > +		return -EINVAL;
> > +
> > +	major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
> > +	minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
> > +
> > +	status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
> > +	cmd = !!status ? NFSD_SET : NFSD_CLEAR;
> > +
> > +	mutex_lock(&nfsd_mutex);
> > +	switch (major) {
> > +	case 4:
> > +		ret = nfsd_minorversion(nn, minor, cmd);
> > +		break;
> > +	case 2:
> > +	case 3:
> > +		if (!minor) {
> > +			ret = nfsd_vers(nn, major, cmd);
> > +			break;
> > +		}
> > +		fallthrough;
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +	mutex_unlock(&nfsd_mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * nfsd_nl_version_get_doit - Handle verion_get dumpit
> > + * @skb: reply buffer
> > + * @cb: netlink metadata and command arguments
> > + *
> > + * Returns the size of the reply or a negative errno.
> > + */
> > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > +			       struct netlink_callback *cb)
> > +{
> > +	struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> > +	int i, ret = -ENOMEM;
> > +
> > +	mutex_lock(&nfsd_mutex);
> > +
> > +	for (i = 2; i <= 4; i++) {
> > +		int j;
> > +
> > +		if (i < cb->args[0]) /* already consumed */
> > +			continue;
> > +
> > +		if (!nfsd_vers(nn, i, NFSD_AVAIL))
> > +			continue;
> > +
> > +		for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
> > +			void *hdr;
> > +
> > +			if (!nfsd_vers(nn, i, NFSD_TEST))
> > +				continue;
> > +
> > +			/* NFSv{2,3} does not support minor numbers */
> > +			if (i < 4 && j)
> > +				continue;
> > +
> > +			if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> > +				continue;
> > +
> > +			hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > +					  cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > +					  0, NFSD_CMD_VERSION_GET);
> > +			if (!hdr)
> > +				goto out;
> > +
> > +			if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> > +			    nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> > +				goto out;
> > +
> > +			genlmsg_end(skb, hdr);
> > +		}
> > +	}
> > +	cb->args[0] = i;
> > +	ret = skb->len;
> > +out:
> > +	mutex_unlock(&nfsd_mutex);
> > +
> > +	return ret;
> > +}
> > +
> >  /**
> >   * nfsd_net_init - Prepare the nfsd_net portion of a new net namespace
> >   * @net: a freshly-created network namespace
> > diff --git a/include/uapi/linux/nfsd_netlink.h b/include/uapi/linux/nfsd_netlink.h
> > index 1b6fe1f9ed0e..1b3340f31baa 100644
> > --- a/include/uapi/linux/nfsd_netlink.h
> > +++ b/include/uapi/linux/nfsd_netlink.h
> > @@ -36,10 +36,21 @@ enum {
> >  	NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
> >  };
> >  
> > +enum {
> > +	NFSD_A_SERVER_VERSION_MAJOR = 1,
> > +	NFSD_A_SERVER_VERSION_MINOR,
> > +	NFSD_A_SERVER_VERSION_STATUS,
> > +
> > +	__NFSD_A_SERVER_VERSION_MAX,
> > +	NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> > +};
> > +
> >  enum {
> >  	NFSD_CMD_RPC_STATUS_GET = 1,
> >  	NFSD_CMD_THREADS_SET,
> >  	NFSD_CMD_THREADS_GET,
> > +	NFSD_CMD_VERSION_SET,
> > +	NFSD_CMD_VERSION_GET,
> >  
> >  	__NFSD_CMD_MAX,
> >  	NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> > index 9768328a7751..4cb71c3cd18d 100644
> > --- a/tools/net/ynl/generated/nfsd-user.c
> > +++ b/tools/net/ynl/generated/nfsd-user.c
> > @@ -17,6 +17,8 @@ static const char * const nfsd_op_strmap[] = {
> >  	[NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
> >  	[NFSD_CMD_THREADS_SET] = "threads-set",
> >  	[NFSD_CMD_THREADS_GET] = "threads-get",
> > +	[NFSD_CMD_VERSION_SET] = "version-set",
> > +	[NFSD_CMD_VERSION_GET] = "version-get",
> >  };
> >  
> >  const char *nfsd_op_str(int op)
> > @@ -58,6 +60,17 @@ struct ynl_policy_nest nfsd_server_worker_nest = {
> >  	.table = nfsd_server_worker_policy,
> >  };
> >  
> > +struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
> > +	[NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
> > +	[NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
> > +	[NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
> > +};
> > +
> > +struct ynl_policy_nest nfsd_server_version_nest = {
> > +	.max_attr = NFSD_A_SERVER_VERSION_MAX,
> > +	.table = nfsd_server_version_policy,
> > +};
> > +
> >  /* Common nested types */
> >  /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> >  /* NFSD_CMD_RPC_STATUS_GET - dump */
> > @@ -290,6 +303,74 @@ struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
> >  	return NULL;
> >  }
> >  
> > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > +/* NFSD_CMD_VERSION_SET - do */
> > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
> > +{
> > +	free(req);
> > +}
> > +
> > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
> > +{
> > +	struct nlmsghdr *nlh;
> > +	int err;
> > +
> > +	nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
> > +	ys->req_policy = &nfsd_server_version_nest;
> > +
> > +	if (req->_present.major)
> > +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
> > +	if (req->_present.minor)
> > +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
> > +	if (req->_present.status)
> > +		mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
> > +
> > +	err = ynl_exec(ys, nlh, NULL);
> > +	if (err < 0)
> > +		return -1;
> > +
> > +	return 0;
> > +}
> > +
> > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > +/* NFSD_CMD_VERSION_GET - dump */
> > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
> > +{
> > +	struct nfsd_version_get_list *next = rsp;
> > +
> > +	while ((void *)next != YNL_LIST_END) {
> > +		rsp = next;
> > +		next = rsp->next;
> > +
> > +		free(rsp);
> > +	}
> > +}
> > +
> > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> > +{
> > +	struct ynl_dump_state yds = {};
> > +	struct nlmsghdr *nlh;
> > +	int err;
> > +
> > +	yds.ys = ys;
> > +	yds.alloc_sz = sizeof(struct nfsd_version_get_list);
> > +	yds.cb = nfsd_version_get_rsp_parse;
> > +	yds.rsp_cmd = NFSD_CMD_VERSION_GET;
> > +	yds.rsp_policy = &nfsd_server_version_nest;
> > +
> > +	nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
> > +
> > +	err = ynl_exec_dump(ys, nlh, &yds);
> > +	if (err < 0)
> > +		goto free_list;
> > +
> > +	return yds.first;
> > +
> > +free_list:
> > +	nfsd_version_get_list_free(yds.first);
> > +	return NULL;
> > +}
> > +
> >  const struct ynl_family ynl_nfsd_family =  {
> >  	.name		= "nfsd",
> >  };
> > diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> > index e162a4f20d91..e61c5a9e46fb 100644
> > --- a/tools/net/ynl/generated/nfsd-user.h
> > +++ b/tools/net/ynl/generated/nfsd-user.h
> > @@ -111,4 +111,59 @@ void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
> >   */
> >  struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
> >  
> > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > +/* NFSD_CMD_VERSION_SET - do */
> > +struct nfsd_version_set_req {
> > +	struct {
> > +		__u32 major:1;
> > +		__u32 minor:1;
> > +		__u32 status:1;
> > +	} _present;
> > +
> > +	__u32 major;
> > +	__u32 minor;
> > +	__u8 status;
> > +};
> > +
> 
> This more or less mirrors how the "versions" file works today, but that
> interface is quite klunky.  We don't have a use case that requires that
> we do this piecemeal like this. I think we'd be better served with a
> more declarative interface that reconfigures the supported versions in
> one shot:
> 
> Instead of having "major,minor,status" and potentially having to call
> this command several times from userland, it seems like it would be
> nicer to just have userland send down a list "major,minor" that should
> be enabled, and then just let the kernel figure out whether to enable or
> disable each. An empty list could mean "disable everything".
> 
> That's simpler to reason out as an interface from userland too. Trying
> to keep track of the enabled and disabled versions and twiddle it is
> really tricky in rpc.nfsd today.

Jeff and Lorenzo, this sounds to me like a useful and narrow
improvement to this interface, one that should be implemented as
part of this patch series.

Ditto for Jeff's review comment on 3/3.


> > +static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
> > +{
> > +	return calloc(1, sizeof(struct nfsd_version_set_req));
> > +}
> > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
> > +
> > +static inline void
> > +nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
> > +{
> > +	req->_present.major = 1;
> > +	req->major = major;
> > +}
> > +static inline void
> > +nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
> > +{
> > +	req->_present.minor = 1;
> > +	req->minor = minor;
> > +}
> > +static inline void
> > +nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
> > +{
> > +	req->_present.status = 1;
> > +	req->status = status;
> > +}
> > +
> > +/*
> > + * enable/disable server version
> > + */
> > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
> > +
> > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > +/* NFSD_CMD_VERSION_GET - dump */
> > +struct nfsd_version_get_list {
> > +	struct nfsd_version_get_list *next;
> > +	struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
> > +};
> > +
> > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
> > +
> > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
> > +
> >  #endif /* _LINUX_NFSD_GEN_H */
> 
> -- 
> Jeff Layton <[e-mail]>
>

Jeff Layton Nov. 30, 2023, 4:17 p.m. UTC | #6

On Thu, 2023-11-30 at 10:45 +0100, Lorenzo Bianconi wrote:
> > On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> > > Introduce write_version netlink command similar to the ones available
> > > through the procfs.
> > > 
> > > Signed-off-by: Lorenzo Bianconi <[e-mail]>
> > > ---
> > >  Documentation/netlink/specs/nfsd.yaml |  32 ++++++++
> > >  fs/nfsd/netlink.c                     |  19 +++++
> > >  fs/nfsd/netlink.h                     |   3 +
> > >  fs/nfsd/nfsctl.c                      | 105 ++++++++++++++++++++++++++
> > >  include/uapi/linux/nfsd_netlink.h     |  11 +++
> > >  tools/net/ynl/generated/nfsd-user.c   |  81 ++++++++++++++++++++
> > >  tools/net/ynl/generated/nfsd-user.h   |  55 ++++++++++++++
> > >  7 files changed, 306 insertions(+)
> > > 
> > > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > > index c92e1425d316..6c5e42bb20f6 100644
> > > --- a/Documentation/netlink/specs/nfsd.yaml
> > > +++ b/Documentation/netlink/specs/nfsd.yaml
> > > @@ -68,6 +68,18 @@ attribute-sets:
> > >        -
> > >          name: threads
> > >          type: u32
> > > +  -
> > > +    name: server-version
> > > +    attributes:
> > > +      -
> > > +        name: major
> > > +        type: u32
> > > +      -
> > > +        name: minor
> > > +        type: u32
> > > +      -
> > > +        name: status
> > > +        type: u8
> > >  
> > > 
> > > 
> > > 
> > >  operations:
> > >    list:
> > > @@ -110,3 +122,23 @@ operations:
> > >          reply:
> > >            attributes:
> > >              - threads
> > > +    -
> > > +      name: version-set
> > > +      doc: enable/disable server version
> > > +      attribute-set: server-version
> > > +      flags: [ admin-perm ]
> > > +      do:
> > > +        request:
> > > +          attributes:
> > > +            - major
> > > +            - minor
> > > +            - status
> > > +    -
> > > +      name: version-get
> > > +      doc: dump server versions
> > > +      attribute-set: server-version
> > > +      dump:
> > > +        reply:
> > > +          attributes:
> > > +            - major
> > > +            - minor
> > > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > > index 1a59a8e6c7e2..0608a7bd193b 100644
> > > --- a/fs/nfsd/netlink.c
> > > +++ b/fs/nfsd/netlink.c
> > > @@ -15,6 +15,13 @@ static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
> > >  	[NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
> > >  };
> > >  
> > > 
> > > 
> > > 
> > > +/* NFSD_CMD_VERSION_SET - do */
> > > +static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
> > > +	[NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
> > > +	[NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
> > > +	[NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> > > +};
> > > +
> > >  /* Ops table for nfsd */
> > >  static const struct genl_split_ops nfsd_nl_ops[] = {
> > >  	{
> > > @@ -36,6 +43,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> > >  		.doit	= nfsd_nl_threads_get_doit,
> > >  		.flags	= GENL_CMD_CAP_DO,
> > >  	},
> > > +	{
> > > +		.cmd		= NFSD_CMD_VERSION_SET,
> > > +		.doit		= nfsd_nl_version_set_doit,
> > > +		.policy		= nfsd_version_set_nl_policy,
> > > +		.maxattr	= NFSD_A_SERVER_VERSION_STATUS,
> > > +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > > +	},
> > > +	{
> > > +		.cmd	= NFSD_CMD_VERSION_GET,
> > > +		.dumpit	= nfsd_nl_version_get_dumpit,
> > > +		.flags	= GENL_CMD_CAP_DUMP,
> > > +	},
> > >  };
> > >  
> > > 
> > > 
> > > 
> > >  struct genl_family nfsd_nl_family __ro_after_init = {
> > > diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h
> > > index 4137fac477e4..7d203cec08e4 100644
> > > --- a/fs/nfsd/netlink.h
> > > +++ b/fs/nfsd/netlink.h
> > > @@ -18,6 +18,9 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> > >  				  struct netlink_callback *cb);
> > >  int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> > >  int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> > > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > > +			       struct netlink_callback *cb);
> > >  
> > > 
> > > 
> > > 
> > >  extern struct genl_family nfsd_nl_family;
> > >  
> > > 
> > > 
> > > 
> > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > > index 130b3d937a79..f04430f79687 100644
> > > --- a/fs/nfsd/nfsctl.c
> > > +++ b/fs/nfsd/nfsctl.c
> > > @@ -1757,6 +1757,111 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> > >  	return err;
> > >  }
> > >  
> > > 
> > > 
> > > 
> > > +/**
> > > + * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
> > > + * @skb: reply buffer
> > > + * @info: netlink metadata and command arguments
> > > + *
> > > + * Return 0 on success or a negative errno.
> > > + */
> > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
> > > +{
> > > +	struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > > +	enum vers_op cmd;
> > > +	u32 major, minor;
> > > +	u8 status;
> > > +	int ret;
> > > +
> > > +	if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
> > > +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
> > > +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
> > > +		return -EINVAL;
> > > +
> > > +	major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
> > > +	minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
> > > +
> > > +	status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
> > > +	cmd = !!status ? NFSD_SET : NFSD_CLEAR;
> > > +
> > > +	mutex_lock(&nfsd_mutex);
> > > +	switch (major) {
> > > +	case 4:
> > > +		ret = nfsd_minorversion(nn, minor, cmd);
> > > +		break;
> > > +	case 2:
> > > +	case 3:
> > > +		if (!minor) {
> > > +			ret = nfsd_vers(nn, major, cmd);
> > > +			break;
> > > +		}
> > > +		fallthrough;
> > > +	default:
> > > +		ret = -EINVAL;
> > > +		break;
> > > +	}
> > > +	mutex_unlock(&nfsd_mutex);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/**
> > > + * nfsd_nl_version_get_doit - Handle verion_get dumpit
> > > + * @skb: reply buffer
> > > + * @cb: netlink metadata and command arguments
> > > + *
> > > + * Returns the size of the reply or a negative errno.
> > > + */
> > > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > > +			       struct netlink_callback *cb)
> > > +{
> > > +	struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> > > +	int i, ret = -ENOMEM;
> > > +
> > > +	mutex_lock(&nfsd_mutex);
> > > +
> > > +	for (i = 2; i <= 4; i++) {
> > > +		int j;
> > > +
> > > +		if (i < cb->args[0]) /* already consumed */
> > > +			continue;
> > > +
> > > +		if (!nfsd_vers(nn, i, NFSD_AVAIL))
> > > +			continue;
> > > +
> > > +		for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
> > > +			void *hdr;
> > > +
> > > +			if (!nfsd_vers(nn, i, NFSD_TEST))
> > > +				continue;
> > > +
> > > +			/* NFSv{2,3} does not support minor numbers */
> > > +			if (i < 4 && j)
> > > +				continue;
> > > +
> > > +			if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> > > +				continue;
> > > +
> > > +			hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > > +					  cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > > +					  0, NFSD_CMD_VERSION_GET);
> > > +			if (!hdr)
> > > +				goto out;
> > > +
> > > +			if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> > > +			    nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> > > +				goto out;
> > > +
> > > +			genlmsg_end(skb, hdr);
> > > +		}
> > > +	}
> > > +	cb->args[0] = i;
> > > +	ret = skb->len;
> > > +out:
> > > +	mutex_unlock(&nfsd_mutex);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  /**
> > >   * nfsd_net_init - Prepare the nfsd_net portion of a new net namespace
> > >   * @net: a freshly-created network namespace
> > > diff --git a/include/uapi/linux/nfsd_netlink.h b/include/uapi/linux/nfsd_netlink.h
> > > index 1b6fe1f9ed0e..1b3340f31baa 100644
> > > --- a/include/uapi/linux/nfsd_netlink.h
> > > +++ b/include/uapi/linux/nfsd_netlink.h
> > > @@ -36,10 +36,21 @@ enum {
> > >  	NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
> > >  };
> > >  
> > > 
> > > 
> > > 
> > > +enum {
> > > +	NFSD_A_SERVER_VERSION_MAJOR = 1,
> > > +	NFSD_A_SERVER_VERSION_MINOR,
> > > +	NFSD_A_SERVER_VERSION_STATUS,
> > > +
> > > +	__NFSD_A_SERVER_VERSION_MAX,
> > > +	NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> > > +};
> > > +
> > >  enum {
> > >  	NFSD_CMD_RPC_STATUS_GET = 1,
> > >  	NFSD_CMD_THREADS_SET,
> > >  	NFSD_CMD_THREADS_GET,
> > > +	NFSD_CMD_VERSION_SET,
> > > +	NFSD_CMD_VERSION_GET,
> > >  
> > > 
> > > 
> > > 
> > >  	__NFSD_CMD_MAX,
> > >  	NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > > diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> > > index 9768328a7751..4cb71c3cd18d 100644
> > > --- a/tools/net/ynl/generated/nfsd-user.c
> > > +++ b/tools/net/ynl/generated/nfsd-user.c
> > > @@ -17,6 +17,8 @@ static const char * const nfsd_op_strmap[] = {
> > >  	[NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
> > >  	[NFSD_CMD_THREADS_SET] = "threads-set",
> > >  	[NFSD_CMD_THREADS_GET] = "threads-get",
> > > +	[NFSD_CMD_VERSION_SET] = "version-set",
> > > +	[NFSD_CMD_VERSION_GET] = "version-get",
> > >  };
> > >  
> > > 
> > > 
> > > 
> > >  const char *nfsd_op_str(int op)
> > > @@ -58,6 +60,17 @@ struct ynl_policy_nest nfsd_server_worker_nest = {
> > >  	.table = nfsd_server_worker_policy,
> > >  };
> > >  
> > > 
> > > 
> > > 
> > > +struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
> > > +	[NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
> > > +	[NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
> > > +	[NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
> > > +};
> > > +
> > > +struct ynl_policy_nest nfsd_server_version_nest = {
> > > +	.max_attr = NFSD_A_SERVER_VERSION_MAX,
> > > +	.table = nfsd_server_version_policy,
> > > +};
> > > +
> > >  /* Common nested types */
> > >  /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> > >  /* NFSD_CMD_RPC_STATUS_GET - dump */
> > > @@ -290,6 +303,74 @@ struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
> > >  	return NULL;
> > >  }
> > >  
> > > 
> > > 
> > > 
> > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > +/* NFSD_CMD_VERSION_SET - do */
> > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
> > > +{
> > > +	free(req);
> > > +}
> > > +
> > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
> > > +{
> > > +	struct nlmsghdr *nlh;
> > > +	int err;
> > > +
> > > +	nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
> > > +	ys->req_policy = &nfsd_server_version_nest;
> > > +
> > > +	if (req->_present.major)
> > > +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
> > > +	if (req->_present.minor)
> > > +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
> > > +	if (req->_present.status)
> > > +		mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
> > > +
> > > +	err = ynl_exec(ys, nlh, NULL);
> > > +	if (err < 0)
> > > +		return -1;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > +/* NFSD_CMD_VERSION_GET - dump */
> > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
> > > +{
> > > +	struct nfsd_version_get_list *next = rsp;
> > > +
> > > +	while ((void *)next != YNL_LIST_END) {
> > > +		rsp = next;
> > > +		next = rsp->next;
> > > +
> > > +		free(rsp);
> > > +	}
> > > +}
> > > +
> > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> > > +{
> > > +	struct ynl_dump_state yds = {};
> > > +	struct nlmsghdr *nlh;
> > > +	int err;
> > > +
> > > +	yds.ys = ys;
> > > +	yds.alloc_sz = sizeof(struct nfsd_version_get_list);
> > > +	yds.cb = nfsd_version_get_rsp_parse;
> > > +	yds.rsp_cmd = NFSD_CMD_VERSION_GET;
> > > +	yds.rsp_policy = &nfsd_server_version_nest;
> > > +
> > > +	nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
> > > +
> > > +	err = ynl_exec_dump(ys, nlh, &yds);
> > > +	if (err < 0)
> > > +		goto free_list;
> > > +
> > > +	return yds.first;
> > > +
> > > +free_list:
> > > +	nfsd_version_get_list_free(yds.first);
> > > +	return NULL;
> > > +}
> > > +
> > >  const struct ynl_family ynl_nfsd_family =  {
> > >  	.name		= "nfsd",
> > >  };
> > > diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> > > index e162a4f20d91..e61c5a9e46fb 100644
> > > --- a/tools/net/ynl/generated/nfsd-user.h
> > > +++ b/tools/net/ynl/generated/nfsd-user.h
> > > @@ -111,4 +111,59 @@ void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
> > >   */
> > >  struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
> > >  
> > > 
> > > 
> > > 
> > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > +/* NFSD_CMD_VERSION_SET - do */
> > > +struct nfsd_version_set_req {
> > > +	struct {
> > > +		__u32 major:1;
> > > +		__u32 minor:1;
> > > +		__u32 status:1;
> > > +	} _present;
> > > +
> > > +	__u32 major;
> > > +	__u32 minor;
> > > +	__u8 status;
> > > +};
> > > +
> > 
> > This more or less mirrors how the "versions" file works today, but that
> > interface is quite klunky.  We don't have a use case that requires that
> > we do this piecemeal like this. I think we'd be better served with a
> > more declarative interface that reconfigures the supported versions in
> > one shot:
> > 
> > Instead of having "major,minor,status" and potentially having to call
> > this command several times from userland, it seems like it would be
> > nicer to just have userland send down a list "major,minor" that should
> > be enabled, and then just let the kernel figure out whether to enable or
> > disable each. An empty list could mean "disable everything".
> > 
> > That's simpler to reason out as an interface from userland too. Trying
> > to keep track of the enabled and disabled versions and twiddle it is
> > really tricky in rpc.nfsd today.
> 
> Ack. So far I have just converted the current implementation to netlink
> and I have not changed any logic. Anyway I am fine to change the current
> logic. Should we have 2 rpc.nfsd version in this case?
> 

No. The goal is to make this a seamless change for systems
administrators who are already familiar with the userland tools.

I think what we want to aim for is to teach the existing rpc.nfsd to
speak netlink if it's available. If it's not, then it can fall back to
using the nfsdfs interfaces instead. Eventually (years from now) we can
remove the old interface support, once most everything in the field has
netlink support.


> 
> >  
> > 
> > > +static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
> > > +{
> > > +	return calloc(1, sizeof(struct nfsd_version_set_req));
> > > +}
> > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
> > > +
> > > +static inline void
> > > +nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
> > > +{
> > > +	req->_present.major = 1;
> > > +	req->major = major;
> > > +}
> > > +static inline void
> > > +nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
> > > +{
> > > +	req->_present.minor = 1;
> > > +	req->minor = minor;
> > > +}
> > > +static inline void
> > > +nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
> > > +{
> > > +	req->_present.status = 1;
> > > +	req->status = status;
> > > +}
> > > +
> > > +/*
> > > + * enable/disable server version
> > > + */
> > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
> > > +
> > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > +/* NFSD_CMD_VERSION_GET - dump */
> > > +struct nfsd_version_get_list {
> > > +	struct nfsd_version_get_list *next;
> > > +	struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
> > > +};
> > > +
> > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
> > > +
> > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
> > > +
> > >  #endif /* _LINUX_NFSD_GEN_H */
> > 
> > -- 
> > Jeff Layton <[e-mail]>

Lorenzo Bianconi Nov. 30, 2023, 4:25 p.m. UTC | #7

> On Wed, Nov 29, 2023 at 01:23:37PM -0500, Jeff Layton wrote:
> > On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> > > Introduce write_version netlink command similar to the ones available
> > > through the procfs.
> > > 
> > > Signed-off-by: Lorenzo Bianconi <[e-mail]>
> > > ---
> > >  Documentation/netlink/specs/nfsd.yaml |  32 ++++++++
> > >  fs/nfsd/netlink.c                     |  19 +++++
> > >  fs/nfsd/netlink.h                     |   3 +
> > >  fs/nfsd/nfsctl.c                      | 105 ++++++++++++++++++++++++++
> > >  include/uapi/linux/nfsd_netlink.h     |  11 +++
> > >  tools/net/ynl/generated/nfsd-user.c   |  81 ++++++++++++++++++++
> > >  tools/net/ynl/generated/nfsd-user.h   |  55 ++++++++++++++
> > >  7 files changed, 306 insertions(+)
> > > 
> > > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > > index c92e1425d316..6c5e42bb20f6 100644
> > > --- a/Documentation/netlink/specs/nfsd.yaml
> > > +++ b/Documentation/netlink/specs/nfsd.yaml
> > > @@ -68,6 +68,18 @@ attribute-sets:
> > >        -
> > >          name: threads
> > >          type: u32
> > > +  -
> > > +    name: server-version
> > > +    attributes:
> > > +      -
> > > +        name: major
> > > +        type: u32
> > > +      -
> > > +        name: minor
> > > +        type: u32
> > > +      -
> > > +        name: status
> > > +        type: u8
> > >  
> > >  operations:
> > >    list:
> > > @@ -110,3 +122,23 @@ operations:
> > >          reply:
> > >            attributes:
> > >              - threads
> > > +    -
> > > +      name: version-set
> > > +      doc: enable/disable server version
> > > +      attribute-set: server-version
> > > +      flags: [ admin-perm ]
> > > +      do:
> > > +        request:
> > > +          attributes:
> > > +            - major
> > > +            - minor
> > > +            - status
> > > +    -
> > > +      name: version-get
> > > +      doc: dump server versions
> > > +      attribute-set: server-version
> > > +      dump:
> > > +        reply:
> > > +          attributes:
> > > +            - major
> > > +            - minor
> > > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > > index 1a59a8e6c7e2..0608a7bd193b 100644
> > > --- a/fs/nfsd/netlink.c
> > > +++ b/fs/nfsd/netlink.c
> > > @@ -15,6 +15,13 @@ static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
> > >  	[NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
> > >  };
> > >  
> > > +/* NFSD_CMD_VERSION_SET - do */
> > > +static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
> > > +	[NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
> > > +	[NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
> > > +	[NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> > > +};
> > > +
> > >  /* Ops table for nfsd */
> > >  static const struct genl_split_ops nfsd_nl_ops[] = {
> > >  	{
> > > @@ -36,6 +43,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> > >  		.doit	= nfsd_nl_threads_get_doit,
> > >  		.flags	= GENL_CMD_CAP_DO,
> > >  	},
> > > +	{
> > > +		.cmd		= NFSD_CMD_VERSION_SET,
> > > +		.doit		= nfsd_nl_version_set_doit,
> > > +		.policy		= nfsd_version_set_nl_policy,
> > > +		.maxattr	= NFSD_A_SERVER_VERSION_STATUS,
> > > +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > > +	},
> > > +	{
> > > +		.cmd	= NFSD_CMD_VERSION_GET,
> > > +		.dumpit	= nfsd_nl_version_get_dumpit,
> > > +		.flags	= GENL_CMD_CAP_DUMP,
> > > +	},
> > >  };
> > >  
> > >  struct genl_family nfsd_nl_family __ro_after_init = {
> > > diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h
> > > index 4137fac477e4..7d203cec08e4 100644
> > > --- a/fs/nfsd/netlink.h
> > > +++ b/fs/nfsd/netlink.h
> > > @@ -18,6 +18,9 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> > >  				  struct netlink_callback *cb);
> > >  int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> > >  int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> > > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > > +			       struct netlink_callback *cb);
> > >  
> > >  extern struct genl_family nfsd_nl_family;
> > >  
> > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > > index 130b3d937a79..f04430f79687 100644
> > > --- a/fs/nfsd/nfsctl.c
> > > +++ b/fs/nfsd/nfsctl.c
> > > @@ -1757,6 +1757,111 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> > >  	return err;
> > >  }
> > >  
> > > +/**
> > > + * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
> > > + * @skb: reply buffer
> > > + * @info: netlink metadata and command arguments
> > > + *
> > > + * Return 0 on success or a negative errno.
> > > + */
> > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
> > > +{
> > > +	struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > > +	enum vers_op cmd;
> > > +	u32 major, minor;
> > > +	u8 status;
> > > +	int ret;
> > > +
> > > +	if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
> > > +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
> > > +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
> > > +		return -EINVAL;
> > > +
> > > +	major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
> > > +	minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
> > > +
> > > +	status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
> > > +	cmd = !!status ? NFSD_SET : NFSD_CLEAR;
> > > +
> > > +	mutex_lock(&nfsd_mutex);
> > > +	switch (major) {
> > > +	case 4:
> > > +		ret = nfsd_minorversion(nn, minor, cmd);
> > > +		break;
> > > +	case 2:
> > > +	case 3:
> > > +		if (!minor) {
> > > +			ret = nfsd_vers(nn, major, cmd);
> > > +			break;
> > > +		}
> > > +		fallthrough;
> > > +	default:
> > > +		ret = -EINVAL;
> > > +		break;
> > > +	}
> > > +	mutex_unlock(&nfsd_mutex);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/**
> > > + * nfsd_nl_version_get_doit - Handle verion_get dumpit
> > > + * @skb: reply buffer
> > > + * @cb: netlink metadata and command arguments
> > > + *
> > > + * Returns the size of the reply or a negative errno.
> > > + */
> > > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > > +			       struct netlink_callback *cb)
> > > +{
> > > +	struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> > > +	int i, ret = -ENOMEM;
> > > +
> > > +	mutex_lock(&nfsd_mutex);
> > > +
> > > +	for (i = 2; i <= 4; i++) {
> > > +		int j;
> > > +
> > > +		if (i < cb->args[0]) /* already consumed */
> > > +			continue;
> > > +
> > > +		if (!nfsd_vers(nn, i, NFSD_AVAIL))
> > > +			continue;
> > > +
> > > +		for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
> > > +			void *hdr;
> > > +
> > > +			if (!nfsd_vers(nn, i, NFSD_TEST))
> > > +				continue;
> > > +
> > > +			/* NFSv{2,3} does not support minor numbers */
> > > +			if (i < 4 && j)
> > > +				continue;
> > > +
> > > +			if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> > > +				continue;
> > > +
> > > +			hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > > +					  cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > > +					  0, NFSD_CMD_VERSION_GET);
> > > +			if (!hdr)
> > > +				goto out;
> > > +
> > > +			if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> > > +			    nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> > > +				goto out;
> > > +
> > > +			genlmsg_end(skb, hdr);
> > > +		}
> > > +	}
> > > +	cb->args[0] = i;
> > > +	ret = skb->len;
> > > +out:
> > > +	mutex_unlock(&nfsd_mutex);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  /**
> > >   * nfsd_net_init - Prepare the nfsd_net portion of a new net namespace
> > >   * @net: a freshly-created network namespace
> > > diff --git a/include/uapi/linux/nfsd_netlink.h b/include/uapi/linux/nfsd_netlink.h
> > > index 1b6fe1f9ed0e..1b3340f31baa 100644
> > > --- a/include/uapi/linux/nfsd_netlink.h
> > > +++ b/include/uapi/linux/nfsd_netlink.h
> > > @@ -36,10 +36,21 @@ enum {
> > >  	NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
> > >  };
> > >  
> > > +enum {
> > > +	NFSD_A_SERVER_VERSION_MAJOR = 1,
> > > +	NFSD_A_SERVER_VERSION_MINOR,
> > > +	NFSD_A_SERVER_VERSION_STATUS,
> > > +
> > > +	__NFSD_A_SERVER_VERSION_MAX,
> > > +	NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> > > +};
> > > +
> > >  enum {
> > >  	NFSD_CMD_RPC_STATUS_GET = 1,
> > >  	NFSD_CMD_THREADS_SET,
> > >  	NFSD_CMD_THREADS_GET,
> > > +	NFSD_CMD_VERSION_SET,
> > > +	NFSD_CMD_VERSION_GET,
> > >  
> > >  	__NFSD_CMD_MAX,
> > >  	NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > > diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> > > index 9768328a7751..4cb71c3cd18d 100644
> > > --- a/tools/net/ynl/generated/nfsd-user.c
> > > +++ b/tools/net/ynl/generated/nfsd-user.c
> > > @@ -17,6 +17,8 @@ static const char * const nfsd_op_strmap[] = {
> > >  	[NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
> > >  	[NFSD_CMD_THREADS_SET] = "threads-set",
> > >  	[NFSD_CMD_THREADS_GET] = "threads-get",
> > > +	[NFSD_CMD_VERSION_SET] = "version-set",
> > > +	[NFSD_CMD_VERSION_GET] = "version-get",
> > >  };
> > >  
> > >  const char *nfsd_op_str(int op)
> > > @@ -58,6 +60,17 @@ struct ynl_policy_nest nfsd_server_worker_nest = {
> > >  	.table = nfsd_server_worker_policy,
> > >  };
> > >  
> > > +struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
> > > +	[NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
> > > +	[NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
> > > +	[NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
> > > +};
> > > +
> > > +struct ynl_policy_nest nfsd_server_version_nest = {
> > > +	.max_attr = NFSD_A_SERVER_VERSION_MAX,
> > > +	.table = nfsd_server_version_policy,
> > > +};
> > > +
> > >  /* Common nested types */
> > >  /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> > >  /* NFSD_CMD_RPC_STATUS_GET - dump */
> > > @@ -290,6 +303,74 @@ struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
> > >  	return NULL;
> > >  }
> > >  
> > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > +/* NFSD_CMD_VERSION_SET - do */
> > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
> > > +{
> > > +	free(req);
> > > +}
> > > +
> > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
> > > +{
> > > +	struct nlmsghdr *nlh;
> > > +	int err;
> > > +
> > > +	nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
> > > +	ys->req_policy = &nfsd_server_version_nest;
> > > +
> > > +	if (req->_present.major)
> > > +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
> > > +	if (req->_present.minor)
> > > +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
> > > +	if (req->_present.status)
> > > +		mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
> > > +
> > > +	err = ynl_exec(ys, nlh, NULL);
> > > +	if (err < 0)
> > > +		return -1;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > +/* NFSD_CMD_VERSION_GET - dump */
> > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
> > > +{
> > > +	struct nfsd_version_get_list *next = rsp;
> > > +
> > > +	while ((void *)next != YNL_LIST_END) {
> > > +		rsp = next;
> > > +		next = rsp->next;
> > > +
> > > +		free(rsp);
> > > +	}
> > > +}
> > > +
> > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> > > +{
> > > +	struct ynl_dump_state yds = {};
> > > +	struct nlmsghdr *nlh;
> > > +	int err;
> > > +
> > > +	yds.ys = ys;
> > > +	yds.alloc_sz = sizeof(struct nfsd_version_get_list);
> > > +	yds.cb = nfsd_version_get_rsp_parse;
> > > +	yds.rsp_cmd = NFSD_CMD_VERSION_GET;
> > > +	yds.rsp_policy = &nfsd_server_version_nest;
> > > +
> > > +	nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
> > > +
> > > +	err = ynl_exec_dump(ys, nlh, &yds);
> > > +	if (err < 0)
> > > +		goto free_list;
> > > +
> > > +	return yds.first;
> > > +
> > > +free_list:
> > > +	nfsd_version_get_list_free(yds.first);
> > > +	return NULL;
> > > +}
> > > +
> > >  const struct ynl_family ynl_nfsd_family =  {
> > >  	.name		= "nfsd",
> > >  };
> > > diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> > > index e162a4f20d91..e61c5a9e46fb 100644
> > > --- a/tools/net/ynl/generated/nfsd-user.h
> > > +++ b/tools/net/ynl/generated/nfsd-user.h
> > > @@ -111,4 +111,59 @@ void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
> > >   */
> > >  struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
> > >  
> > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > +/* NFSD_CMD_VERSION_SET - do */
> > > +struct nfsd_version_set_req {
> > > +	struct {
> > > +		__u32 major:1;
> > > +		__u32 minor:1;
> > > +		__u32 status:1;
> > > +	} _present;
> > > +
> > > +	__u32 major;
> > > +	__u32 minor;
> > > +	__u8 status;
> > > +};
> > > +
> > 
> > This more or less mirrors how the "versions" file works today, but that
> > interface is quite klunky.  We don't have a use case that requires that
> > we do this piecemeal like this. I think we'd be better served with a
> > more declarative interface that reconfigures the supported versions in
> > one shot:
> > 
> > Instead of having "major,minor,status" and potentially having to call
> > this command several times from userland, it seems like it would be
> > nicer to just have userland send down a list "major,minor" that should
> > be enabled, and then just let the kernel figure out whether to enable or
> > disable each. An empty list could mean "disable everything".
> > 
> > That's simpler to reason out as an interface from userland too. Trying
> > to keep track of the enabled and disabled versions and twiddle it is
> > really tricky in rpc.nfsd today.
> 
> Jeff and Lorenzo, this sounds to me like a useful and narrow
> improvement to this interface, one that should be implemented as
> part of this patch series.

ack, I am fine with it, I will work on patch 2/3 and 3/3.
@Chuck: am I suppose to respin patch 1/3 too?

Regards,
Lorenzo

> 
> Ditto for Jeff's review comment on 3/3.
> 
> 
> > > +static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
> > > +{
> > > +	return calloc(1, sizeof(struct nfsd_version_set_req));
> > > +}
> > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
> > > +
> > > +static inline void
> > > +nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
> > > +{
> > > +	req->_present.major = 1;
> > > +	req->major = major;
> > > +}
> > > +static inline void
> > > +nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
> > > +{
> > > +	req->_present.minor = 1;
> > > +	req->minor = minor;
> > > +}
> > > +static inline void
> > > +nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
> > > +{
> > > +	req->_present.status = 1;
> > > +	req->status = status;
> > > +}
> > > +
> > > +/*
> > > + * enable/disable server version
> > > + */
> > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
> > > +
> > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > +/* NFSD_CMD_VERSION_GET - dump */
> > > +struct nfsd_version_get_list {
> > > +	struct nfsd_version_get_list *next;
> > > +	struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
> > > +};
> > > +
> > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
> > > +
> > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
> > > +
> > >  #endif /* _LINUX_NFSD_GEN_H */
> > 
> > -- 
> > Jeff Layton <[e-mail]>
> > 
> 
> -- 
> Chuck Lever

Chuck Lever Nov. 30, 2023, 4:29 p.m. UTC | #8

On Thu, Nov 30, 2023 at 05:25:52PM +0100, Lorenzo Bianconi wrote:
> > On Wed, Nov 29, 2023 at 01:23:37PM -0500, Jeff Layton wrote:
> > > On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> > > > Introduce write_version netlink command similar to the ones available
> > > > through the procfs.
> > > > 
> > > > Signed-off-by: Lorenzo Bianconi <[e-mail]>
> > > > ---
> > > >  Documentation/netlink/specs/nfsd.yaml |  32 ++++++++
> > > >  fs/nfsd/netlink.c                     |  19 +++++
> > > >  fs/nfsd/netlink.h                     |   3 +
> > > >  fs/nfsd/nfsctl.c                      | 105 ++++++++++++++++++++++++++
> > > >  include/uapi/linux/nfsd_netlink.h     |  11 +++
> > > >  tools/net/ynl/generated/nfsd-user.c   |  81 ++++++++++++++++++++
> > > >  tools/net/ynl/generated/nfsd-user.h   |  55 ++++++++++++++
> > > >  7 files changed, 306 insertions(+)
> > > > 
> > > > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > > > index c92e1425d316..6c5e42bb20f6 100644
> > > > --- a/Documentation/netlink/specs/nfsd.yaml
> > > > +++ b/Documentation/netlink/specs/nfsd.yaml
> > > > @@ -68,6 +68,18 @@ attribute-sets:
> > > >        -
> > > >          name: threads
> > > >          type: u32
> > > > +  -
> > > > +    name: server-version
> > > > +    attributes:
> > > > +      -
> > > > +        name: major
> > > > +        type: u32
> > > > +      -
> > > > +        name: minor
> > > > +        type: u32
> > > > +      -
> > > > +        name: status
> > > > +        type: u8
> > > >  
> > > >  operations:
> > > >    list:
> > > > @@ -110,3 +122,23 @@ operations:
> > > >          reply:
> > > >            attributes:
> > > >              - threads
> > > > +    -
> > > > +      name: version-set
> > > > +      doc: enable/disable server version
> > > > +      attribute-set: server-version
> > > > +      flags: [ admin-perm ]
> > > > +      do:
> > > > +        request:
> > > > +          attributes:
> > > > +            - major
> > > > +            - minor
> > > > +            - status
> > > > +    -
> > > > +      name: version-get
> > > > +      doc: dump server versions
> > > > +      attribute-set: server-version
> > > > +      dump:
> > > > +        reply:
> > > > +          attributes:
> > > > +            - major
> > > > +            - minor
> > > > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > > > index 1a59a8e6c7e2..0608a7bd193b 100644
> > > > --- a/fs/nfsd/netlink.c
> > > > +++ b/fs/nfsd/netlink.c
> > > > @@ -15,6 +15,13 @@ static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
> > > >  	[NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
> > > >  };
> > > >  
> > > > +/* NFSD_CMD_VERSION_SET - do */
> > > > +static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
> > > > +	[NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
> > > > +	[NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
> > > > +	[NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> > > > +};
> > > > +
> > > >  /* Ops table for nfsd */
> > > >  static const struct genl_split_ops nfsd_nl_ops[] = {
> > > >  	{
> > > > @@ -36,6 +43,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> > > >  		.doit	= nfsd_nl_threads_get_doit,
> > > >  		.flags	= GENL_CMD_CAP_DO,
> > > >  	},
> > > > +	{
> > > > +		.cmd		= NFSD_CMD_VERSION_SET,
> > > > +		.doit		= nfsd_nl_version_set_doit,
> > > > +		.policy		= nfsd_version_set_nl_policy,
> > > > +		.maxattr	= NFSD_A_SERVER_VERSION_STATUS,
> > > > +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > > > +	},
> > > > +	{
> > > > +		.cmd	= NFSD_CMD_VERSION_GET,
> > > > +		.dumpit	= nfsd_nl_version_get_dumpit,
> > > > +		.flags	= GENL_CMD_CAP_DUMP,
> > > > +	},
> > > >  };
> > > >  
> > > >  struct genl_family nfsd_nl_family __ro_after_init = {
> > > > diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h
> > > > index 4137fac477e4..7d203cec08e4 100644
> > > > --- a/fs/nfsd/netlink.h
> > > > +++ b/fs/nfsd/netlink.h
> > > > @@ -18,6 +18,9 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> > > >  				  struct netlink_callback *cb);
> > > >  int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> > > >  int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> > > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> > > > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > > > +			       struct netlink_callback *cb);
> > > >  
> > > >  extern struct genl_family nfsd_nl_family;
> > > >  
> > > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > > > index 130b3d937a79..f04430f79687 100644
> > > > --- a/fs/nfsd/nfsctl.c
> > > > +++ b/fs/nfsd/nfsctl.c
> > > > @@ -1757,6 +1757,111 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> > > >  	return err;
> > > >  }
> > > >  
> > > > +/**
> > > > + * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
> > > > + * @skb: reply buffer
> > > > + * @info: netlink metadata and command arguments
> > > > + *
> > > > + * Return 0 on success or a negative errno.
> > > > + */
> > > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
> > > > +{
> > > > +	struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > > > +	enum vers_op cmd;
> > > > +	u32 major, minor;
> > > > +	u8 status;
> > > > +	int ret;
> > > > +
> > > > +	if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
> > > > +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
> > > > +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
> > > > +		return -EINVAL;
> > > > +
> > > > +	major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
> > > > +	minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
> > > > +
> > > > +	status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
> > > > +	cmd = !!status ? NFSD_SET : NFSD_CLEAR;
> > > > +
> > > > +	mutex_lock(&nfsd_mutex);
> > > > +	switch (major) {
> > > > +	case 4:
> > > > +		ret = nfsd_minorversion(nn, minor, cmd);
> > > > +		break;
> > > > +	case 2:
> > > > +	case 3:
> > > > +		if (!minor) {
> > > > +			ret = nfsd_vers(nn, major, cmd);
> > > > +			break;
> > > > +		}
> > > > +		fallthrough;
> > > > +	default:
> > > > +		ret = -EINVAL;
> > > > +		break;
> > > > +	}
> > > > +	mutex_unlock(&nfsd_mutex);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +/**
> > > > + * nfsd_nl_version_get_doit - Handle verion_get dumpit
> > > > + * @skb: reply buffer
> > > > + * @cb: netlink metadata and command arguments
> > > > + *
> > > > + * Returns the size of the reply or a negative errno.
> > > > + */
> > > > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > > > +			       struct netlink_callback *cb)
> > > > +{
> > > > +	struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> > > > +	int i, ret = -ENOMEM;
> > > > +
> > > > +	mutex_lock(&nfsd_mutex);
> > > > +
> > > > +	for (i = 2; i <= 4; i++) {
> > > > +		int j;
> > > > +
> > > > +		if (i < cb->args[0]) /* already consumed */
> > > > +			continue;
> > > > +
> > > > +		if (!nfsd_vers(nn, i, NFSD_AVAIL))
> > > > +			continue;
> > > > +
> > > > +		for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
> > > > +			void *hdr;
> > > > +
> > > > +			if (!nfsd_vers(nn, i, NFSD_TEST))
> > > > +				continue;
> > > > +
> > > > +			/* NFSv{2,3} does not support minor numbers */
> > > > +			if (i < 4 && j)
> > > > +				continue;
> > > > +
> > > > +			if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> > > > +				continue;
> > > > +
> > > > +			hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > > > +					  cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > > > +					  0, NFSD_CMD_VERSION_GET);
> > > > +			if (!hdr)
> > > > +				goto out;
> > > > +
> > > > +			if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> > > > +			    nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> > > > +				goto out;
> > > > +
> > > > +			genlmsg_end(skb, hdr);
> > > > +		}
> > > > +	}
> > > > +	cb->args[0] = i;
> > > > +	ret = skb->len;
> > > > +out:
> > > > +	mutex_unlock(&nfsd_mutex);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > >  /**
> > > >   * nfsd_net_init - Prepare the nfsd_net portion of a new net namespace
> > > >   * @net: a freshly-created network namespace
> > > > diff --git a/include/uapi/linux/nfsd_netlink.h b/include/uapi/linux/nfsd_netlink.h
> > > > index 1b6fe1f9ed0e..1b3340f31baa 100644
> > > > --- a/include/uapi/linux/nfsd_netlink.h
> > > > +++ b/include/uapi/linux/nfsd_netlink.h
> > > > @@ -36,10 +36,21 @@ enum {
> > > >  	NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
> > > >  };
> > > >  
> > > > +enum {
> > > > +	NFSD_A_SERVER_VERSION_MAJOR = 1,
> > > > +	NFSD_A_SERVER_VERSION_MINOR,
> > > > +	NFSD_A_SERVER_VERSION_STATUS,
> > > > +
> > > > +	__NFSD_A_SERVER_VERSION_MAX,
> > > > +	NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> > > > +};
> > > > +
> > > >  enum {
> > > >  	NFSD_CMD_RPC_STATUS_GET = 1,
> > > >  	NFSD_CMD_THREADS_SET,
> > > >  	NFSD_CMD_THREADS_GET,
> > > > +	NFSD_CMD_VERSION_SET,
> > > > +	NFSD_CMD_VERSION_GET,
> > > >  
> > > >  	__NFSD_CMD_MAX,
> > > >  	NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > > > diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> > > > index 9768328a7751..4cb71c3cd18d 100644
> > > > --- a/tools/net/ynl/generated/nfsd-user.c
> > > > +++ b/tools/net/ynl/generated/nfsd-user.c
> > > > @@ -17,6 +17,8 @@ static const char * const nfsd_op_strmap[] = {
> > > >  	[NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
> > > >  	[NFSD_CMD_THREADS_SET] = "threads-set",
> > > >  	[NFSD_CMD_THREADS_GET] = "threads-get",
> > > > +	[NFSD_CMD_VERSION_SET] = "version-set",
> > > > +	[NFSD_CMD_VERSION_GET] = "version-get",
> > > >  };
> > > >  
> > > >  const char *nfsd_op_str(int op)
> > > > @@ -58,6 +60,17 @@ struct ynl_policy_nest nfsd_server_worker_nest = {
> > > >  	.table = nfsd_server_worker_policy,
> > > >  };
> > > >  
> > > > +struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
> > > > +	[NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
> > > > +	[NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
> > > > +	[NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
> > > > +};
> > > > +
> > > > +struct ynl_policy_nest nfsd_server_version_nest = {
> > > > +	.max_attr = NFSD_A_SERVER_VERSION_MAX,
> > > > +	.table = nfsd_server_version_policy,
> > > > +};
> > > > +
> > > >  /* Common nested types */
> > > >  /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> > > >  /* NFSD_CMD_RPC_STATUS_GET - dump */
> > > > @@ -290,6 +303,74 @@ struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
> > > >  	return NULL;
> > > >  }
> > > >  
> > > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > > +/* NFSD_CMD_VERSION_SET - do */
> > > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
> > > > +{
> > > > +	free(req);
> > > > +}
> > > > +
> > > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
> > > > +{
> > > > +	struct nlmsghdr *nlh;
> > > > +	int err;
> > > > +
> > > > +	nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
> > > > +	ys->req_policy = &nfsd_server_version_nest;
> > > > +
> > > > +	if (req->_present.major)
> > > > +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
> > > > +	if (req->_present.minor)
> > > > +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
> > > > +	if (req->_present.status)
> > > > +		mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
> > > > +
> > > > +	err = ynl_exec(ys, nlh, NULL);
> > > > +	if (err < 0)
> > > > +		return -1;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > > +/* NFSD_CMD_VERSION_GET - dump */
> > > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
> > > > +{
> > > > +	struct nfsd_version_get_list *next = rsp;
> > > > +
> > > > +	while ((void *)next != YNL_LIST_END) {
> > > > +		rsp = next;
> > > > +		next = rsp->next;
> > > > +
> > > > +		free(rsp);
> > > > +	}
> > > > +}
> > > > +
> > > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> > > > +{
> > > > +	struct ynl_dump_state yds = {};
> > > > +	struct nlmsghdr *nlh;
> > > > +	int err;
> > > > +
> > > > +	yds.ys = ys;
> > > > +	yds.alloc_sz = sizeof(struct nfsd_version_get_list);
> > > > +	yds.cb = nfsd_version_get_rsp_parse;
> > > > +	yds.rsp_cmd = NFSD_CMD_VERSION_GET;
> > > > +	yds.rsp_policy = &nfsd_server_version_nest;
> > > > +
> > > > +	nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
> > > > +
> > > > +	err = ynl_exec_dump(ys, nlh, &yds);
> > > > +	if (err < 0)
> > > > +		goto free_list;
> > > > +
> > > > +	return yds.first;
> > > > +
> > > > +free_list:
> > > > +	nfsd_version_get_list_free(yds.first);
> > > > +	return NULL;
> > > > +}
> > > > +
> > > >  const struct ynl_family ynl_nfsd_family =  {
> > > >  	.name		= "nfsd",
> > > >  };
> > > > diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> > > > index e162a4f20d91..e61c5a9e46fb 100644
> > > > --- a/tools/net/ynl/generated/nfsd-user.h
> > > > +++ b/tools/net/ynl/generated/nfsd-user.h
> > > > @@ -111,4 +111,59 @@ void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
> > > >   */
> > > >  struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
> > > >  
> > > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > > +/* NFSD_CMD_VERSION_SET - do */
> > > > +struct nfsd_version_set_req {
> > > > +	struct {
> > > > +		__u32 major:1;
> > > > +		__u32 minor:1;
> > > > +		__u32 status:1;
> > > > +	} _present;
> > > > +
> > > > +	__u32 major;
> > > > +	__u32 minor;
> > > > +	__u8 status;
> > > > +};
> > > > +
> > > 
> > > This more or less mirrors how the "versions" file works today, but that
> > > interface is quite klunky.  We don't have a use case that requires that
> > > we do this piecemeal like this. I think we'd be better served with a
> > > more declarative interface that reconfigures the supported versions in
> > > one shot:
> > > 
> > > Instead of having "major,minor,status" and potentially having to call
> > > this command several times from userland, it seems like it would be
> > > nicer to just have userland send down a list "major,minor" that should
> > > be enabled, and then just let the kernel figure out whether to enable or
> > > disable each. An empty list could mean "disable everything".
> > > 
> > > That's simpler to reason out as an interface from userland too. Trying
> > > to keep track of the enabled and disabled versions and twiddle it is
> > > really tricky in rpc.nfsd today.
> > 
> > Jeff and Lorenzo, this sounds to me like a useful and narrow
> > improvement to this interface, one that should be implemented as
> > part of this patch series.
> 
> ack, I am fine with it, I will work on patch 2/3 and 3/3.
> @Chuck: am I suppose to respin patch 1/3 too?

I assumed there might be minor changes to 1/3 after Jakub's second
review, so I have not applied it yet. You can resend all three to
me.


> Regards,
> Lorenzo
> 
> > 
> > Ditto for Jeff's review comment on 3/3.
> > 
> > 
> > > > +static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
> > > > +{
> > > > +	return calloc(1, sizeof(struct nfsd_version_set_req));
> > > > +}
> > > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
> > > > +
> > > > +static inline void
> > > > +nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
> > > > +{
> > > > +	req->_present.major = 1;
> > > > +	req->major = major;
> > > > +}
> > > > +static inline void
> > > > +nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
> > > > +{
> > > > +	req->_present.minor = 1;
> > > > +	req->minor = minor;
> > > > +}
> > > > +static inline void
> > > > +nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
> > > > +{
> > > > +	req->_present.status = 1;
> > > > +	req->status = status;
> > > > +}
> > > > +
> > > > +/*
> > > > + * enable/disable server version
> > > > + */
> > > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
> > > > +
> > > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > > +/* NFSD_CMD_VERSION_GET - dump */
> > > > +struct nfsd_version_get_list {
> > > > +	struct nfsd_version_get_list *next;
> > > > +	struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
> > > > +};
> > > > +
> > > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
> > > > +
> > > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
> > > > +
> > > >  #endif /* _LINUX_NFSD_GEN_H */
> > > 
> > > -- 
> > > Jeff Layton <[e-mail]>
> > > 
> > 
> > -- 
> > Chuck Lever

Lorenzo Bianconi Nov. 30, 2023, 4:32 p.m. UTC | #9

On Nov 30, Chuck Lever wrote:
> On Thu, Nov 30, 2023 at 05:25:52PM +0100, Lorenzo Bianconi wrote:
> > > On Wed, Nov 29, 2023 at 01:23:37PM -0500, Jeff Layton wrote:
> > > > On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> > > > > Introduce write_version netlink command similar to the ones available
> > > > > through the procfs.
> > > > > 
> > > > > Signed-off-by: Lorenzo Bianconi <[e-mail]>
> > > > > ---
> > > > >  Documentation/netlink/specs/nfsd.yaml |  32 ++++++++
> > > > >  fs/nfsd/netlink.c                     |  19 +++++
> > > > >  fs/nfsd/netlink.h                     |   3 +
> > > > >  fs/nfsd/nfsctl.c                      | 105 ++++++++++++++++++++++++++
> > > > >  include/uapi/linux/nfsd_netlink.h     |  11 +++
> > > > >  tools/net/ynl/generated/nfsd-user.c   |  81 ++++++++++++++++++++
> > > > >  tools/net/ynl/generated/nfsd-user.h   |  55 ++++++++++++++
> > > > >  7 files changed, 306 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > > > > index c92e1425d316..6c5e42bb20f6 100644
> > > > > --- a/Documentation/netlink/specs/nfsd.yaml
> > > > > +++ b/Documentation/netlink/specs/nfsd.yaml
> > > > > @@ -68,6 +68,18 @@ attribute-sets:
> > > > >        -
> > > > >          name: threads
> > > > >          type: u32
> > > > > +  -
> > > > > +    name: server-version
> > > > > +    attributes:
> > > > > +      -
> > > > > +        name: major
> > > > > +        type: u32
> > > > > +      -
> > > > > +        name: minor
> > > > > +        type: u32
> > > > > +      -
> > > > > +        name: status
> > > > > +        type: u8
> > > > >  
> > > > >  operations:
> > > > >    list:
> > > > > @@ -110,3 +122,23 @@ operations:
> > > > >          reply:
> > > > >            attributes:
> > > > >              - threads
> > > > > +    -
> > > > > +      name: version-set
> > > > > +      doc: enable/disable server version
> > > > > +      attribute-set: server-version
> > > > > +      flags: [ admin-perm ]
> > > > > +      do:
> > > > > +        request:
> > > > > +          attributes:
> > > > > +            - major
> > > > > +            - minor
> > > > > +            - status
> > > > > +    -
> > > > > +      name: version-get
> > > > > +      doc: dump server versions
> > > > > +      attribute-set: server-version
> > > > > +      dump:
> > > > > +        reply:
> > > > > +          attributes:
> > > > > +            - major
> > > > > +            - minor
> > > > > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > > > > index 1a59a8e6c7e2..0608a7bd193b 100644
> > > > > --- a/fs/nfsd/netlink.c
> > > > > +++ b/fs/nfsd/netlink.c
> > > > > @@ -15,6 +15,13 @@ static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
> > > > >  	[NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
> > > > >  };
> > > > >  
> > > > > +/* NFSD_CMD_VERSION_SET - do */
> > > > > +static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
> > > > > +	[NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
> > > > > +	[NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
> > > > > +	[NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> > > > > +};
> > > > > +
> > > > >  /* Ops table for nfsd */
> > > > >  static const struct genl_split_ops nfsd_nl_ops[] = {
> > > > >  	{
> > > > > @@ -36,6 +43,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> > > > >  		.doit	= nfsd_nl_threads_get_doit,
> > > > >  		.flags	= GENL_CMD_CAP_DO,
> > > > >  	},
> > > > > +	{
> > > > > +		.cmd		= NFSD_CMD_VERSION_SET,
> > > > > +		.doit		= nfsd_nl_version_set_doit,
> > > > > +		.policy		= nfsd_version_set_nl_policy,
> > > > > +		.maxattr	= NFSD_A_SERVER_VERSION_STATUS,
> > > > > +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > > > > +	},
> > > > > +	{
> > > > > +		.cmd	= NFSD_CMD_VERSION_GET,
> > > > > +		.dumpit	= nfsd_nl_version_get_dumpit,
> > > > > +		.flags	= GENL_CMD_CAP_DUMP,
> > > > > +	},
> > > > >  };
> > > > >  
> > > > >  struct genl_family nfsd_nl_family __ro_after_init = {
> > > > > diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h
> > > > > index 4137fac477e4..7d203cec08e4 100644
> > > > > --- a/fs/nfsd/netlink.h
> > > > > +++ b/fs/nfsd/netlink.h
> > > > > @@ -18,6 +18,9 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> > > > >  				  struct netlink_callback *cb);
> > > > >  int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> > > > >  int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> > > > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> > > > > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > > > > +			       struct netlink_callback *cb);
> > > > >  
> > > > >  extern struct genl_family nfsd_nl_family;
> > > > >  
> > > > > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > > > > index 130b3d937a79..f04430f79687 100644
> > > > > --- a/fs/nfsd/nfsctl.c
> > > > > +++ b/fs/nfsd/nfsctl.c
> > > > > @@ -1757,6 +1757,111 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> > > > >  	return err;
> > > > >  }
> > > > >  
> > > > > +/**
> > > > > + * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
> > > > > + * @skb: reply buffer
> > > > > + * @info: netlink metadata and command arguments
> > > > > + *
> > > > > + * Return 0 on success or a negative errno.
> > > > > + */
> > > > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
> > > > > +{
> > > > > +	struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > > > > +	enum vers_op cmd;
> > > > > +	u32 major, minor;
> > > > > +	u8 status;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
> > > > > +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
> > > > > +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
> > > > > +	minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
> > > > > +
> > > > > +	status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
> > > > > +	cmd = !!status ? NFSD_SET : NFSD_CLEAR;
> > > > > +
> > > > > +	mutex_lock(&nfsd_mutex);
> > > > > +	switch (major) {
> > > > > +	case 4:
> > > > > +		ret = nfsd_minorversion(nn, minor, cmd);
> > > > > +		break;
> > > > > +	case 2:
> > > > > +	case 3:
> > > > > +		if (!minor) {
> > > > > +			ret = nfsd_vers(nn, major, cmd);
> > > > > +			break;
> > > > > +		}
> > > > > +		fallthrough;
> > > > > +	default:
> > > > > +		ret = -EINVAL;
> > > > > +		break;
> > > > > +	}
> > > > > +	mutex_unlock(&nfsd_mutex);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * nfsd_nl_version_get_doit - Handle verion_get dumpit
> > > > > + * @skb: reply buffer
> > > > > + * @cb: netlink metadata and command arguments
> > > > > + *
> > > > > + * Returns the size of the reply or a negative errno.
> > > > > + */
> > > > > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > > > > +			       struct netlink_callback *cb)
> > > > > +{
> > > > > +	struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> > > > > +	int i, ret = -ENOMEM;
> > > > > +
> > > > > +	mutex_lock(&nfsd_mutex);
> > > > > +
> > > > > +	for (i = 2; i <= 4; i++) {
> > > > > +		int j;
> > > > > +
> > > > > +		if (i < cb->args[0]) /* already consumed */
> > > > > +			continue;
> > > > > +
> > > > > +		if (!nfsd_vers(nn, i, NFSD_AVAIL))
> > > > > +			continue;
> > > > > +
> > > > > +		for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
> > > > > +			void *hdr;
> > > > > +
> > > > > +			if (!nfsd_vers(nn, i, NFSD_TEST))
> > > > > +				continue;
> > > > > +
> > > > > +			/* NFSv{2,3} does not support minor numbers */
> > > > > +			if (i < 4 && j)
> > > > > +				continue;
> > > > > +
> > > > > +			if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> > > > > +				continue;
> > > > > +
> > > > > +			hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > > > > +					  cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > > > > +					  0, NFSD_CMD_VERSION_GET);
> > > > > +			if (!hdr)
> > > > > +				goto out;
> > > > > +
> > > > > +			if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> > > > > +			    nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> > > > > +				goto out;
> > > > > +
> > > > > +			genlmsg_end(skb, hdr);
> > > > > +		}
> > > > > +	}
> > > > > +	cb->args[0] = i;
> > > > > +	ret = skb->len;
> > > > > +out:
> > > > > +	mutex_unlock(&nfsd_mutex);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * nfsd_net_init - Prepare the nfsd_net portion of a new net namespace
> > > > >   * @net: a freshly-created network namespace
> > > > > diff --git a/include/uapi/linux/nfsd_netlink.h b/include/uapi/linux/nfsd_netlink.h
> > > > > index 1b6fe1f9ed0e..1b3340f31baa 100644
> > > > > --- a/include/uapi/linux/nfsd_netlink.h
> > > > > +++ b/include/uapi/linux/nfsd_netlink.h
> > > > > @@ -36,10 +36,21 @@ enum {
> > > > >  	NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
> > > > >  };
> > > > >  
> > > > > +enum {
> > > > > +	NFSD_A_SERVER_VERSION_MAJOR = 1,
> > > > > +	NFSD_A_SERVER_VERSION_MINOR,
> > > > > +	NFSD_A_SERVER_VERSION_STATUS,
> > > > > +
> > > > > +	__NFSD_A_SERVER_VERSION_MAX,
> > > > > +	NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> > > > > +};
> > > > > +
> > > > >  enum {
> > > > >  	NFSD_CMD_RPC_STATUS_GET = 1,
> > > > >  	NFSD_CMD_THREADS_SET,
> > > > >  	NFSD_CMD_THREADS_GET,
> > > > > +	NFSD_CMD_VERSION_SET,
> > > > > +	NFSD_CMD_VERSION_GET,
> > > > >  
> > > > >  	__NFSD_CMD_MAX,
> > > > >  	NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > > > > diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> > > > > index 9768328a7751..4cb71c3cd18d 100644
> > > > > --- a/tools/net/ynl/generated/nfsd-user.c
> > > > > +++ b/tools/net/ynl/generated/nfsd-user.c
> > > > > @@ -17,6 +17,8 @@ static const char * const nfsd_op_strmap[] = {
> > > > >  	[NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
> > > > >  	[NFSD_CMD_THREADS_SET] = "threads-set",
> > > > >  	[NFSD_CMD_THREADS_GET] = "threads-get",
> > > > > +	[NFSD_CMD_VERSION_SET] = "version-set",
> > > > > +	[NFSD_CMD_VERSION_GET] = "version-get",
> > > > >  };
> > > > >  
> > > > >  const char *nfsd_op_str(int op)
> > > > > @@ -58,6 +60,17 @@ struct ynl_policy_nest nfsd_server_worker_nest = {
> > > > >  	.table = nfsd_server_worker_policy,
> > > > >  };
> > > > >  
> > > > > +struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
> > > > > +	[NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
> > > > > +	[NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
> > > > > +	[NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
> > > > > +};
> > > > > +
> > > > > +struct ynl_policy_nest nfsd_server_version_nest = {
> > > > > +	.max_attr = NFSD_A_SERVER_VERSION_MAX,
> > > > > +	.table = nfsd_server_version_policy,
> > > > > +};
> > > > > +
> > > > >  /* Common nested types */
> > > > >  /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> > > > >  /* NFSD_CMD_RPC_STATUS_GET - dump */
> > > > > @@ -290,6 +303,74 @@ struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
> > > > >  	return NULL;
> > > > >  }
> > > > >  
> > > > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > > > +/* NFSD_CMD_VERSION_SET - do */
> > > > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
> > > > > +{
> > > > > +	free(req);
> > > > > +}
> > > > > +
> > > > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
> > > > > +{
> > > > > +	struct nlmsghdr *nlh;
> > > > > +	int err;
> > > > > +
> > > > > +	nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
> > > > > +	ys->req_policy = &nfsd_server_version_nest;
> > > > > +
> > > > > +	if (req->_present.major)
> > > > > +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
> > > > > +	if (req->_present.minor)
> > > > > +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
> > > > > +	if (req->_present.status)
> > > > > +		mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
> > > > > +
> > > > > +	err = ynl_exec(ys, nlh, NULL);
> > > > > +	if (err < 0)
> > > > > +		return -1;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > > > +/* NFSD_CMD_VERSION_GET - dump */
> > > > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
> > > > > +{
> > > > > +	struct nfsd_version_get_list *next = rsp;
> > > > > +
> > > > > +	while ((void *)next != YNL_LIST_END) {
> > > > > +		rsp = next;
> > > > > +		next = rsp->next;
> > > > > +
> > > > > +		free(rsp);
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> > > > > +{
> > > > > +	struct ynl_dump_state yds = {};
> > > > > +	struct nlmsghdr *nlh;
> > > > > +	int err;
> > > > > +
> > > > > +	yds.ys = ys;
> > > > > +	yds.alloc_sz = sizeof(struct nfsd_version_get_list);
> > > > > +	yds.cb = nfsd_version_get_rsp_parse;
> > > > > +	yds.rsp_cmd = NFSD_CMD_VERSION_GET;
> > > > > +	yds.rsp_policy = &nfsd_server_version_nest;
> > > > > +
> > > > > +	nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
> > > > > +
> > > > > +	err = ynl_exec_dump(ys, nlh, &yds);
> > > > > +	if (err < 0)
> > > > > +		goto free_list;
> > > > > +
> > > > > +	return yds.first;
> > > > > +
> > > > > +free_list:
> > > > > +	nfsd_version_get_list_free(yds.first);
> > > > > +	return NULL;
> > > > > +}
> > > > > +
> > > > >  const struct ynl_family ynl_nfsd_family =  {
> > > > >  	.name		= "nfsd",
> > > > >  };
> > > > > diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> > > > > index e162a4f20d91..e61c5a9e46fb 100644
> > > > > --- a/tools/net/ynl/generated/nfsd-user.h
> > > > > +++ b/tools/net/ynl/generated/nfsd-user.h
> > > > > @@ -111,4 +111,59 @@ void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
> > > > >   */
> > > > >  struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
> > > > >  
> > > > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > > > +/* NFSD_CMD_VERSION_SET - do */
> > > > > +struct nfsd_version_set_req {
> > > > > +	struct {
> > > > > +		__u32 major:1;
> > > > > +		__u32 minor:1;
> > > > > +		__u32 status:1;
> > > > > +	} _present;
> > > > > +
> > > > > +	__u32 major;
> > > > > +	__u32 minor;
> > > > > +	__u8 status;
> > > > > +};
> > > > > +
> > > > 
> > > > This more or less mirrors how the "versions" file works today, but that
> > > > interface is quite klunky.  We don't have a use case that requires that
> > > > we do this piecemeal like this. I think we'd be better served with a
> > > > more declarative interface that reconfigures the supported versions in
> > > > one shot:
> > > > 
> > > > Instead of having "major,minor,status" and potentially having to call
> > > > this command several times from userland, it seems like it would be
> > > > nicer to just have userland send down a list "major,minor" that should
> > > > be enabled, and then just let the kernel figure out whether to enable or
> > > > disable each. An empty list could mean "disable everything".
> > > > 
> > > > That's simpler to reason out as an interface from userland too. Trying
> > > > to keep track of the enabled and disabled versions and twiddle it is
> > > > really tricky in rpc.nfsd today.
> > > 
> > > Jeff and Lorenzo, this sounds to me like a useful and narrow
> > > improvement to this interface, one that should be implemented as
> > > part of this patch series.
> > 
> > ack, I am fine with it, I will work on patch 2/3 and 3/3.
> > @Chuck: am I suppose to respin patch 1/3 too?
> 
> I assumed there might be minor changes to 1/3 after Jakub's second
> review, so I have not applied it yet. You can resend all three to
> me.

iirc jakub's review was just for patch 2/3, anyway up to you.

Regards,
Lorenzo

> 
> 
> > Regards,
> > Lorenzo
> > 
> > > 
> > > Ditto for Jeff's review comment on 3/3.
> > > 
> > > 
> > > > > +static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
> > > > > +{
> > > > > +	return calloc(1, sizeof(struct nfsd_version_set_req));
> > > > > +}
> > > > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
> > > > > +
> > > > > +static inline void
> > > > > +nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
> > > > > +{
> > > > > +	req->_present.major = 1;
> > > > > +	req->major = major;
> > > > > +}
> > > > > +static inline void
> > > > > +nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
> > > > > +{
> > > > > +	req->_present.minor = 1;
> > > > > +	req->minor = minor;
> > > > > +}
> > > > > +static inline void
> > > > > +nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
> > > > > +{
> > > > > +	req->_present.status = 1;
> > > > > +	req->status = status;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * enable/disable server version
> > > > > + */
> > > > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
> > > > > +
> > > > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > > > +/* NFSD_CMD_VERSION_GET - dump */
> > > > > +struct nfsd_version_get_list {
> > > > > +	struct nfsd_version_get_list *next;
> > > > > +	struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
> > > > > +};
> > > > > +
> > > > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
> > > > > +
> > > > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
> > > > > +
> > > > >  #endif /* _LINUX_NFSD_GEN_H */
> > > > 
> > > > -- 
> > > > Jeff Layton <[e-mail]>
> > > > 
> > > 
> > > -- 
> > > Chuck Lever
> 
> 
> 
> -- 
> Chuck Lever

Jakub Kicinski Dec. 1, 2023, 7:28 a.m. UTC | #10

On Thu, 30 Nov 2023 11:32:17 +0100 Lorenzo Bianconi wrote:
> > u8? I guess...  
> 
> here we need just 0 or 1 I would say. Do you suggest create something like an
> enum?

Ah, sorry, thought I must have already complained to you about this 
in the past - netlink aligns everything to 4B. So u8 or u16 or u32
it's all the same size at the message level. For the <u32 types some
bytes are just treated as padding instead of being useful.

But if you explicitly need only 0/1 then it doesn't matter.

Simon Horman Dec. 5, 2023, 8:33 p.m. UTC | #11

On Wed, Nov 29, 2023 at 06:12:44PM +0100, Lorenzo Bianconi wrote:
> Introduce write_version netlink command similar to the ones available
> through the procfs.
> 
> Signed-off-by: Lorenzo Bianconi <[e-mail]>

...

> +/**
> + * nfsd_nl_version_get_doit - Handle verion_get dumpit

Hi Lorenzo,

a minor nit: this function is nfsd_nl_version_get_dumpit

> + * @skb: reply buffer
> + * @cb: netlink metadata and command arguments
> + *
> + * Returns the size of the reply or a negative errno.
> + */
> +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,

...

Patch

diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
index c92e1425d316..6c5e42bb20f6 100644
--- a/Documentation/netlink/specs/nfsd.yaml
+++ b/Documentation/netlink/specs/nfsd.yaml
@@ -68,6 +68,18 @@  attribute-sets:
       -
         name: threads
         type: u32
+  -
+    name: server-version
+    attributes:
+      -
+        name: major
+        type: u32
+      -
+        name: minor
+        type: u32
+      -
+        name: status
+        type: u8
 
 operations:
   list:
@@ -110,3 +122,23 @@  operations:
         reply:
           attributes:
             - threads
+    -
+      name: version-set
+      doc: enable/disable server version
+      attribute-set: server-version
+      flags: [ admin-perm ]
+      do:
+        request:
+          attributes:
+            - major
+            - minor
+            - status
+    -
+      name: version-get
+      doc: dump server versions
+      attribute-set: server-version
+      dump:
+        reply:
+          attributes:
+            - major
+            - minor
diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
index 1a59a8e6c7e2..0608a7bd193b 100644
--- a/fs/nfsd/netlink.c
+++ b/fs/nfsd/netlink.c
@@ -15,6 +15,13 @@  static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
 	[NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
 };
 
+/* NFSD_CMD_VERSION_SET - do */
+static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
+	[NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
+	[NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
+	[NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
+};
+
 /* Ops table for nfsd */
 static const struct genl_split_ops nfsd_nl_ops[] = {
 	{
@@ -36,6 +43,18 @@  static const struct genl_split_ops nfsd_nl_ops[] = {
 		.doit	= nfsd_nl_threads_get_doit,
 		.flags	= GENL_CMD_CAP_DO,
 	},
+	{
+		.cmd		= NFSD_CMD_VERSION_SET,
+		.doit		= nfsd_nl_version_set_doit,
+		.policy		= nfsd_version_set_nl_policy,
+		.maxattr	= NFSD_A_SERVER_VERSION_STATUS,
+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
+	},
+	{
+		.cmd	= NFSD_CMD_VERSION_GET,
+		.dumpit	= nfsd_nl_version_get_dumpit,
+		.flags	= GENL_CMD_CAP_DUMP,
+	},
 };
 
 struct genl_family nfsd_nl_family __ro_after_init = {
diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h
index 4137fac477e4..7d203cec08e4 100644
--- a/fs/nfsd/netlink.h
+++ b/fs/nfsd/netlink.h
@@ -18,6 +18,9 @@  int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
 				  struct netlink_callback *cb);
 int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
 int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
+int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
+int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
+			       struct netlink_callback *cb);
 
 extern struct genl_family nfsd_nl_family;
 
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 130b3d937a79..f04430f79687 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1757,6 +1757,111 @@  int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
+/**
+ * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
+ * @skb: reply buffer
+ * @info: netlink metadata and command arguments
+ *
+ * Return 0 on success or a negative errno.
+ */
+int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
+{
+	struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
+	enum vers_op cmd;
+	u32 major, minor;
+	u8 status;
+	int ret;
+
+	if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
+	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
+	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
+		return -EINVAL;
+
+	major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
+	minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
+
+	status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
+	cmd = !!status ? NFSD_SET : NFSD_CLEAR;
+
+	mutex_lock(&nfsd_mutex);
+	switch (major) {
+	case 4:
+		ret = nfsd_minorversion(nn, minor, cmd);
+		break;
+	case 2:
+	case 3:
+		if (!minor) {
+			ret = nfsd_vers(nn, major, cmd);
+			break;
+		}
+		fallthrough;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	mutex_unlock(&nfsd_mutex);
+
+	return ret;
+}
+
+/**
+ * nfsd_nl_version_get_doit - Handle verion_get dumpit
+ * @skb: reply buffer
+ * @cb: netlink metadata and command arguments
+ *
+ * Returns the size of the reply or a negative errno.
+ */
+int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
+			       struct netlink_callback *cb)
+{
+	struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
+	int i, ret = -ENOMEM;
+
+	mutex_lock(&nfsd_mutex);
+
+	for (i = 2; i <= 4; i++) {
+		int j;
+
+		if (i < cb->args[0]) /* already consumed */
+			continue;
+
+		if (!nfsd_vers(nn, i, NFSD_AVAIL))
+			continue;
+
+		for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
+			void *hdr;
+
+			if (!nfsd_vers(nn, i, NFSD_TEST))
+				continue;
+
+			/* NFSv{2,3} does not support minor numbers */
+			if (i < 4 && j)
+				continue;
+
+			if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
+				continue;
+
+			hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
+					  cb->nlh->nlmsg_seq, &nfsd_nl_family,
+					  0, NFSD_CMD_VERSION_GET);
+			if (!hdr)
+				goto out;
+
+			if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
+			    nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
+				goto out;
+
+			genlmsg_end(skb, hdr);
+		}
+	}
+	cb->args[0] = i;
+	ret = skb->len;
+out:
+	mutex_unlock(&nfsd_mutex);
+
+	return ret;
+}
+
 /**
  * nfsd_net_init - Prepare the nfsd_net portion of a new net namespace
  * @net: a freshly-created network namespace
diff --git a/include/uapi/linux/nfsd_netlink.h b/include/uapi/linux/nfsd_netlink.h
index 1b6fe1f9ed0e..1b3340f31baa 100644
--- a/include/uapi/linux/nfsd_netlink.h
+++ b/include/uapi/linux/nfsd_netlink.h
@@ -36,10 +36,21 @@  enum {
 	NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
 };
 
+enum {
+	NFSD_A_SERVER_VERSION_MAJOR = 1,
+	NFSD_A_SERVER_VERSION_MINOR,
+	NFSD_A_SERVER_VERSION_STATUS,
+
+	__NFSD_A_SERVER_VERSION_MAX,
+	NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
+};
+
 enum {
 	NFSD_CMD_RPC_STATUS_GET = 1,
 	NFSD_CMD_THREADS_SET,
 	NFSD_CMD_THREADS_GET,
+	NFSD_CMD_VERSION_SET,
+	NFSD_CMD_VERSION_GET,
 
 	__NFSD_CMD_MAX,
 	NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
index 9768328a7751..4cb71c3cd18d 100644
--- a/tools/net/ynl/generated/nfsd-user.c
+++ b/tools/net/ynl/generated/nfsd-user.c
@@ -17,6 +17,8 @@  static const char * const nfsd_op_strmap[] = {
 	[NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
 	[NFSD_CMD_THREADS_SET] = "threads-set",
 	[NFSD_CMD_THREADS_GET] = "threads-get",
+	[NFSD_CMD_VERSION_SET] = "version-set",
+	[NFSD_CMD_VERSION_GET] = "version-get",
 };
 
 const char *nfsd_op_str(int op)
@@ -58,6 +60,17 @@  struct ynl_policy_nest nfsd_server_worker_nest = {
 	.table = nfsd_server_worker_policy,
 };
 
+struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
+	[NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
+	[NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
+	[NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
+};
+
+struct ynl_policy_nest nfsd_server_version_nest = {
+	.max_attr = NFSD_A_SERVER_VERSION_MAX,
+	.table = nfsd_server_version_policy,
+};
+
 /* Common nested types */
 /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
 /* NFSD_CMD_RPC_STATUS_GET - dump */
@@ -290,6 +303,74 @@  struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
 	return NULL;
 }
 
+/* ============== NFSD_CMD_VERSION_SET ============== */
+/* NFSD_CMD_VERSION_SET - do */
+void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
+{
+	free(req);
+}
+
+int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
+{
+	struct nlmsghdr *nlh;
+	int err;
+
+	nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
+	ys->req_policy = &nfsd_server_version_nest;
+
+	if (req->_present.major)
+		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
+	if (req->_present.minor)
+		mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
+	if (req->_present.status)
+		mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
+
+	err = ynl_exec(ys, nlh, NULL);
+	if (err < 0)
+		return -1;
+
+	return 0;
+}
+
+/* ============== NFSD_CMD_VERSION_GET ============== */
+/* NFSD_CMD_VERSION_GET - dump */
+void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
+{
+	struct nfsd_version_get_list *next = rsp;
+
+	while ((void *)next != YNL_LIST_END) {
+		rsp = next;
+		next = rsp->next;
+
+		free(rsp);
+	}
+}
+
+struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
+{
+	struct ynl_dump_state yds = {};
+	struct nlmsghdr *nlh;
+	int err;
+
+	yds.ys = ys;
+	yds.alloc_sz = sizeof(struct nfsd_version_get_list);
+	yds.cb = nfsd_version_get_rsp_parse;
+	yds.rsp_cmd = NFSD_CMD_VERSION_GET;
+	yds.rsp_policy = &nfsd_server_version_nest;
+
+	nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
+
+	err = ynl_exec_dump(ys, nlh, &yds);
+	if (err < 0)
+		goto free_list;
+
+	return yds.first;
+
+free_list:
+	nfsd_version_get_list_free(yds.first);
+	return NULL;
+}
+
 const struct ynl_family ynl_nfsd_family =  {
 	.name		= "nfsd",
 };
diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
index e162a4f20d91..e61c5a9e46fb 100644
--- a/tools/net/ynl/generated/nfsd-user.h
+++ b/tools/net/ynl/generated/nfsd-user.h
@@ -111,4 +111,59 @@  void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
  */
 struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
 
+/* ============== NFSD_CMD_VERSION_SET ============== */
+/* NFSD_CMD_VERSION_SET - do */
+struct nfsd_version_set_req {
+	struct {
+		__u32 major:1;
+		__u32 minor:1;
+		__u32 status:1;
+	} _present;
+
+	__u32 major;
+	__u32 minor;
+	__u8 status;
+};
+
+static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
+{
+	return calloc(1, sizeof(struct nfsd_version_set_req));
+}
+void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
+
+static inline void
+nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
+{
+	req->_present.major = 1;
+	req->major = major;
+}
+static inline void
+nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
+{
+	req->_present.minor = 1;
+	req->minor = minor;
+}
+static inline void
+nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
+{
+	req->_present.status = 1;
+	req->status = status;
+}
+
+/*
+ * enable/disable server version
+ */
+int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
+
+/* ============== NFSD_CMD_VERSION_GET ============== */
+/* NFSD_CMD_VERSION_GET - dump */
+struct nfsd_version_get_list {
+	struct nfsd_version_get_list *next;
+	struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
+};
+
+void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
+
+struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
+
 #endif /* _LINUX_NFSD_GEN_H */