diff mbox series

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

Message ID 67251eabfbbccb806991e6437ebcf1cf00166017.1701277475.git.lorenzo@kernel.org (mailing list archive)
State Not Applicable
Headers show
Series convert write_threads, write_version and write_ports to netlink commands | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Lorenzo Bianconi Nov. 29, 2023, 5:12 p.m. UTC
Introduce write_ports netlink command similar to the ones available
through the procfs.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 Documentation/netlink/specs/nfsd.yaml |  28 +++++++
 fs/nfsd/netlink.c                     |  18 +++++
 fs/nfsd/netlink.h                     |   3 +
 fs/nfsd/nfsctl.c                      | 104 ++++++++++++++++++++++++--
 include/uapi/linux/nfsd_netlink.h     |  10 +++
 tools/net/ynl/generated/nfsd-user.c   |  81 ++++++++++++++++++++
 tools/net/ynl/generated/nfsd-user.h   |  54 +++++++++++++
 7 files changed, 291 insertions(+), 7 deletions(-)

Comments

Jeff Layton Nov. 29, 2023, 6:28 p.m. UTC | #1
On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> Introduce write_ports netlink command similar to the ones available
> through the procfs.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  Documentation/netlink/specs/nfsd.yaml |  28 +++++++
>  fs/nfsd/netlink.c                     |  18 +++++
>  fs/nfsd/netlink.h                     |   3 +
>  fs/nfsd/nfsctl.c                      | 104 ++++++++++++++++++++++++--
>  include/uapi/linux/nfsd_netlink.h     |  10 +++
>  tools/net/ynl/generated/nfsd-user.c   |  81 ++++++++++++++++++++
>  tools/net/ynl/generated/nfsd-user.h   |  54 +++++++++++++
>  7 files changed, 291 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> index 6c5e42bb20f6..1c342ad3c5fa 100644
> --- a/Documentation/netlink/specs/nfsd.yaml
> +++ b/Documentation/netlink/specs/nfsd.yaml
> @@ -80,6 +80,15 @@ attribute-sets:
>        -
>          name: status
>          type: u8
> +  -
> +    name: server-listener
> +    attributes:
> +      -
> +        name: transport-name
> +        type: string
> +      -
> +        name: port
> +        type: u32
>  
>  operations:
>    list:
> @@ -142,3 +151,22 @@ operations:
>            attributes:
>              - major
>              - minor
> +    -
> +      name: listener-start
> +      doc: start server listener
> +      attribute-set: server-listener
> +      flags: [ admin-perm ]
> +      do:
> +        request:
> +          attributes:
> +            - transport-name
> +            - port
> +    -
> +      name: listener-get
> +      doc: dump server listeners
> +      attribute-set: server-listener
> +      dump:
> +        reply:
> +          attributes:
> +            - transport-name
> +            - port
> diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> index 0608a7bd193b..cd51393ede72 100644
> --- a/fs/nfsd/netlink.c
> +++ b/fs/nfsd/netlink.c
> @@ -22,6 +22,12 @@ static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_
>  	[NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
>  };
>  
> +/* NFSD_CMD_LISTENER_START - do */
> +static const struct nla_policy nfsd_listener_start_nl_policy[NFSD_A_SERVER_LISTENER_PORT + 1] = {
> +	[NFSD_A_SERVER_LISTENER_TRANSPORT_NAME] = { .type = NLA_NUL_STRING, },
> +	[NFSD_A_SERVER_LISTENER_PORT] = { .type = NLA_U32, },
> +};
> +
>  /* Ops table for nfsd */
>  static const struct genl_split_ops nfsd_nl_ops[] = {
>  	{
> @@ -55,6 +61,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
>  		.dumpit	= nfsd_nl_version_get_dumpit,
>  		.flags	= GENL_CMD_CAP_DUMP,
>  	},
> +	{
> +		.cmd		= NFSD_CMD_LISTENER_START,
> +		.doit		= nfsd_nl_listener_start_doit,
> +		.policy		= nfsd_listener_start_nl_policy,
> +		.maxattr	= NFSD_A_SERVER_LISTENER_PORT,
> +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> +	},
> +	{
> +		.cmd	= NFSD_CMD_LISTENER_GET,
> +		.dumpit	= nfsd_nl_listener_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 7d203cec08e4..9a51cb83f343 100644
> --- a/fs/nfsd/netlink.h
> +++ b/fs/nfsd/netlink.h
> @@ -21,6 +21,9 @@ 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);
> +int nfsd_nl_listener_start_doit(struct sk_buff *skb, struct genl_info *info);
> +int nfsd_nl_listener_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 f04430f79687..53129b5b7d3c 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -721,18 +721,16 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
>   * A transport listener is added by writing its transport name and
>   * a port number.
>   */
> -static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cred *cred)
> +static ssize_t ___write_ports_addxprt(struct net *net, const struct cred *cred,
> +				      const char *transport, const int port)
>  {
> -	char transport[16];
> -	struct svc_xprt *xprt;
> -	int port, err;
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> -
> -	if (sscanf(buf, "%15s %5u", transport, &port) != 2)
> -		return -EINVAL;
> +	struct svc_xprt *xprt;
> +	int err;
>  
>  	if (port < 1 || port > USHRT_MAX)
>  		return -EINVAL;
> +
>  	trace_nfsd_ctl_ports_addxprt(net, transport, port);
>  
>  	err = nfsd_create_serv(net);
> @@ -765,6 +763,17 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
>  	return err;
>  }
>  
> +static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cred *cred)
> +{
> +	char transport[16];
> +	int port;
> +
> +	if (sscanf(buf, "%15s %5u", transport, &port) != 2)
> +		return -EINVAL;
> +
> +	return ___write_ports_addxprt(net, cred, transport, port);
> +}
> +
>  static ssize_t __write_ports(struct file *file, char *buf, size_t size,
>  			     struct net *net)
>  {
> @@ -1862,6 +1871,87 @@ int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
>  	return ret;
>  }
>  
> +/**
> + * nfsd_nl_listener_start_doit - start the provided nfs server listener
> + * @skb: reply buffer
> + * @info: netlink metadata and command arguments
> + *
> + * Return 0 on success or a negative errno.
> + */
> +int nfsd_nl_listener_start_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int ret;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME) ||
> +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_LISTENER_PORT))
> +		return -EINVAL;
> +
> +	mutex_lock(&nfsd_mutex);
> +	ret = ___write_ports_addxprt(genl_info_net(info), get_current_cred(),
> +			nla_data(info->attrs[NFSD_A_SERVER_LISTENER_TRANSPORT_NAME]),
> +			nla_get_u32(info->attrs[NFSD_A_SERVER_LISTENER_PORT]));
> +	mutex_unlock(&nfsd_mutex);
> +
> +	return 0;
> +}
> +
> +/**
> + * nfsd_nl_version_get_dumpit - Handle listener_get dumpit
> + * @skb: reply buffer
> + * @cb: netlink metadata and command arguments
> + *
> + * Returns the size of the reply or a negative errno.
> + */
> +int nfsd_nl_listener_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 = 0, ret = -ENOMEM;
> +	struct svc_xprt *xprt;
> +	struct svc_serv *serv;
> +
> +	mutex_lock(&nfsd_mutex);
> +
> +	serv = nn->nfsd_serv;
> +	if (!serv) {
> +		mutex_unlock(&nfsd_mutex);
> +		return 0;
> +	}
> +
> +	spin_lock_bh(&serv->sv_lock);
> +	list_for_each_entry(xprt, &serv->sv_permsocks, xpt_list) {
> +		void *hdr;
> +
> +		if (i < cb->args[0]) /* already consumed */
> +			continue;
> +
> +		hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> +				  cb->nlh->nlmsg_seq, &nfsd_nl_family,
> +				  0, NFSD_CMD_LISTENER_GET);
> +		if (!hdr)
> +			goto out;
> +
> +		if (nla_put_string(skb, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME,
> +				   xprt->xpt_class->xcl_name))
> +			goto out;
> +
> +		if (nla_put_u32(skb, NFSD_A_SERVER_LISTENER_PORT,
> +				svc_xprt_local_port(xprt)))
> +			goto out;
> +
> +		genlmsg_end(skb, hdr);
> +		i++;
> +	}
> +	cb->args[0] = i;
> +	ret = skb->len;
> +out:
> +	spin_unlock_bh(&serv->sv_lock);
> +
> +	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 1b3340f31baa..61f4c5b50ecb 100644
> --- a/include/uapi/linux/nfsd_netlink.h
> +++ b/include/uapi/linux/nfsd_netlink.h
> @@ -45,12 +45,22 @@ enum {
>  	NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
>  };
>  
> +enum {
> +	NFSD_A_SERVER_LISTENER_TRANSPORT_NAME = 1,
> +	NFSD_A_SERVER_LISTENER_PORT,
> +
> +	__NFSD_A_SERVER_LISTENER_MAX,
> +	NFSD_A_SERVER_LISTENER_MAX = (__NFSD_A_SERVER_LISTENER_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_LISTENER_START,
> +	NFSD_CMD_LISTENER_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 4cb71c3cd18d..167e404c9e20 100644
> --- a/tools/net/ynl/generated/nfsd-user.c
> +++ b/tools/net/ynl/generated/nfsd-user.c
> @@ -19,6 +19,8 @@ static const char * const nfsd_op_strmap[] = {
>  	[NFSD_CMD_THREADS_GET] = "threads-get",
>  	[NFSD_CMD_VERSION_SET] = "version-set",
>  	[NFSD_CMD_VERSION_GET] = "version-get",
> +	[NFSD_CMD_LISTENER_START] = "listener-start",
> +	[NFSD_CMD_LISTENER_GET] = "listener-get",
>  };
>  
>  const char *nfsd_op_str(int op)
> @@ -71,6 +73,16 @@ struct ynl_policy_nest nfsd_server_version_nest = {
>  	.table = nfsd_server_version_policy,
>  };
>  
> +struct ynl_policy_attr nfsd_server_listener_policy[NFSD_A_SERVER_LISTENER_MAX + 1] = {
> +	[NFSD_A_SERVER_LISTENER_TRANSPORT_NAME] = { .name = "transport-name", .type = YNL_PT_NUL_STR, },
> +	[NFSD_A_SERVER_LISTENER_PORT] = { .name = "port", .type = YNL_PT_U32, },
> +};
> +
> +struct ynl_policy_nest nfsd_server_listener_nest = {
> +	.max_attr = NFSD_A_SERVER_LISTENER_MAX,
> +	.table = nfsd_server_listener_policy,
> +};
> +
>  /* Common nested types */
>  /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
>  /* NFSD_CMD_RPC_STATUS_GET - dump */
> @@ -371,6 +383,75 @@ struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
>  	return NULL;
>  }
>  
> +/* ============== NFSD_CMD_LISTENER_START ============== */
> +/* NFSD_CMD_LISTENER_START - do */
> +void nfsd_listener_start_req_free(struct nfsd_listener_start_req *req)
> +{
> +	free(req->transport_name);
> +	free(req);
> +}
> +
> +int nfsd_listener_start(struct ynl_sock *ys,
> +			struct nfsd_listener_start_req *req)
> +{
> +	struct nlmsghdr *nlh;
> +	int err;
> +
> +	nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_LISTENER_START, 1);
> +	ys->req_policy = &nfsd_server_listener_nest;
> +
> +	if (req->_present.transport_name_len)
> +		mnl_attr_put_strz(nlh, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME, req->transport_name);
> +	if (req->_present.port)
> +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_LISTENER_PORT, req->port);
> +
> +	err = ynl_exec(ys, nlh, NULL);
> +	if (err < 0)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +/* ============== NFSD_CMD_LISTENER_GET ============== */
> +/* NFSD_CMD_LISTENER_GET - dump */
> +void nfsd_listener_get_list_free(struct nfsd_listener_get_list *rsp)
> +{
> +	struct nfsd_listener_get_list *next = rsp;
> +
> +	while ((void *)next != YNL_LIST_END) {
> +		rsp = next;
> +		next = rsp->next;
> +
> +		free(rsp->obj.transport_name);
> +		free(rsp);
> +	}
> +}
> +
> +struct nfsd_listener_get_list *nfsd_listener_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_listener_get_list);
> +	yds.cb = nfsd_listener_get_rsp_parse;
> +	yds.rsp_cmd = NFSD_CMD_LISTENER_GET;
> +	yds.rsp_policy = &nfsd_server_listener_nest;
> +
> +	nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_LISTENER_GET, 1);
> +
> +	err = ynl_exec_dump(ys, nlh, &yds);
> +	if (err < 0)
> +		goto free_list;
> +
> +	return yds.first;
> +
> +free_list:
> +	nfsd_listener_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 e61c5a9e46fb..da3aaaf3f6c0 100644
> --- a/tools/net/ynl/generated/nfsd-user.h
> +++ b/tools/net/ynl/generated/nfsd-user.h
> @@ -166,4 +166,58 @@ 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);
>  
> +/* ============== NFSD_CMD_LISTENER_START ============== */
> +/* NFSD_CMD_LISTENER_START - do */
> +struct nfsd_listener_start_req {
> +	struct {
> +		__u32 transport_name_len;
> +		__u32 port:1;
> +	} _present;
> +
> +	char *transport_name;
> +	__u32 port;
> +};

How do you deconfigure a listener with this interface? i.e. suppose I
want to stop nfsd from listening on a particular port? I think this too
is a place where a declarative interface would be better:

Have userland send down a list of the ports that we should currently be
listening on, and let the kernel do the work to match the request. Again
too, an empty list could mean "close everything".

> +
> +static inline struct nfsd_listener_start_req *
> +nfsd_listener_start_req_alloc(void)
> +{
> +	return calloc(1, sizeof(struct nfsd_listener_start_req));
> +}
> +void nfsd_listener_start_req_free(struct nfsd_listener_start_req *req);
> +
> +static inline void
> +nfsd_listener_start_req_set_transport_name(struct nfsd_listener_start_req *req,
> +					   const char *transport_name)
> +{
> +	free(req->transport_name);
> +	req->_present.transport_name_len = strlen(transport_name);
> +	req->transport_name = malloc(req->_present.transport_name_len + 1);
> +	memcpy(req->transport_name, transport_name, req->_present.transport_name_len);
> +	req->transport_name[req->_present.transport_name_len] = 0;
> +}
> +static inline void
> +nfsd_listener_start_req_set_port(struct nfsd_listener_start_req *req,
> +				 __u32 port)
> +{
> +	req->_present.port = 1;
> +	req->port = port;
> +}
> +
> +/*
> + * start server listener
> + */
> +int nfsd_listener_start(struct ynl_sock *ys,
> +			struct nfsd_listener_start_req *req);
> +
> +/* ============== NFSD_CMD_LISTENER_GET ============== */
> +/* NFSD_CMD_LISTENER_GET - dump */
> +struct nfsd_listener_get_list {
> +	struct nfsd_listener_get_list *next;
> +	struct nfsd_listener_get_rsp obj __attribute__ ((aligned (8)));
> +};
> +
> +void nfsd_listener_get_list_free(struct nfsd_listener_get_list *rsp);
> +
> +struct nfsd_listener_get_list *nfsd_listener_get_dump(struct ynl_sock *ys);
> +
>  #endif /* _LINUX_NFSD_GEN_H */
Lorenzo Bianconi Nov. 30, 2023, 9:57 a.m. UTC | #2
> On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> > Introduce write_ports netlink command similar to the ones available
> > through the procfs.
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  Documentation/netlink/specs/nfsd.yaml |  28 +++++++
> >  fs/nfsd/netlink.c                     |  18 +++++
> >  fs/nfsd/netlink.h                     |   3 +
> >  fs/nfsd/nfsctl.c                      | 104 ++++++++++++++++++++++++--
> >  include/uapi/linux/nfsd_netlink.h     |  10 +++
> >  tools/net/ynl/generated/nfsd-user.c   |  81 ++++++++++++++++++++
> >  tools/net/ynl/generated/nfsd-user.h   |  54 +++++++++++++
> >  7 files changed, 291 insertions(+), 7 deletions(-)
> > 
> > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > index 6c5e42bb20f6..1c342ad3c5fa 100644
> > --- a/Documentation/netlink/specs/nfsd.yaml
> > +++ b/Documentation/netlink/specs/nfsd.yaml
> > @@ -80,6 +80,15 @@ attribute-sets:
> >        -
> >          name: status
> >          type: u8
> > +  -
> > +    name: server-listener
> > +    attributes:
> > +      -
> > +        name: transport-name
> > +        type: string
> > +      -
> > +        name: port
> > +        type: u32
> >  
> >  operations:
> >    list:
> > @@ -142,3 +151,22 @@ operations:
> >            attributes:
> >              - major
> >              - minor
> > +    -
> > +      name: listener-start
> > +      doc: start server listener
> > +      attribute-set: server-listener
> > +      flags: [ admin-perm ]
> > +      do:
> > +        request:
> > +          attributes:
> > +            - transport-name
> > +            - port
> > +    -
> > +      name: listener-get
> > +      doc: dump server listeners
> > +      attribute-set: server-listener
> > +      dump:
> > +        reply:
> > +          attributes:
> > +            - transport-name
> > +            - port
> > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > index 0608a7bd193b..cd51393ede72 100644
> > --- a/fs/nfsd/netlink.c
> > +++ b/fs/nfsd/netlink.c
> > @@ -22,6 +22,12 @@ static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_
> >  	[NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> >  };
> >  
> > +/* NFSD_CMD_LISTENER_START - do */
> > +static const struct nla_policy nfsd_listener_start_nl_policy[NFSD_A_SERVER_LISTENER_PORT + 1] = {
> > +	[NFSD_A_SERVER_LISTENER_TRANSPORT_NAME] = { .type = NLA_NUL_STRING, },
> > +	[NFSD_A_SERVER_LISTENER_PORT] = { .type = NLA_U32, },
> > +};
> > +
> >  /* Ops table for nfsd */
> >  static const struct genl_split_ops nfsd_nl_ops[] = {
> >  	{
> > @@ -55,6 +61,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> >  		.dumpit	= nfsd_nl_version_get_dumpit,
> >  		.flags	= GENL_CMD_CAP_DUMP,
> >  	},
> > +	{
> > +		.cmd		= NFSD_CMD_LISTENER_START,
> > +		.doit		= nfsd_nl_listener_start_doit,
> > +		.policy		= nfsd_listener_start_nl_policy,
> > +		.maxattr	= NFSD_A_SERVER_LISTENER_PORT,
> > +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > +	},
> > +	{
> > +		.cmd	= NFSD_CMD_LISTENER_GET,
> > +		.dumpit	= nfsd_nl_listener_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 7d203cec08e4..9a51cb83f343 100644
> > --- a/fs/nfsd/netlink.h
> > +++ b/fs/nfsd/netlink.h
> > @@ -21,6 +21,9 @@ 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);
> > +int nfsd_nl_listener_start_doit(struct sk_buff *skb, struct genl_info *info);
> > +int nfsd_nl_listener_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 f04430f79687..53129b5b7d3c 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -721,18 +721,16 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
> >   * A transport listener is added by writing its transport name and
> >   * a port number.
> >   */
> > -static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cred *cred)
> > +static ssize_t ___write_ports_addxprt(struct net *net, const struct cred *cred,
> > +				      const char *transport, const int port)
> >  {
> > -	char transport[16];
> > -	struct svc_xprt *xprt;
> > -	int port, err;
> >  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > -
> > -	if (sscanf(buf, "%15s %5u", transport, &port) != 2)
> > -		return -EINVAL;
> > +	struct svc_xprt *xprt;
> > +	int err;
> >  
> >  	if (port < 1 || port > USHRT_MAX)
> >  		return -EINVAL;
> > +
> >  	trace_nfsd_ctl_ports_addxprt(net, transport, port);
> >  
> >  	err = nfsd_create_serv(net);
> > @@ -765,6 +763,17 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
> >  	return err;
> >  }
> >  
> > +static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cred *cred)
> > +{
> > +	char transport[16];
> > +	int port;
> > +
> > +	if (sscanf(buf, "%15s %5u", transport, &port) != 2)
> > +		return -EINVAL;
> > +
> > +	return ___write_ports_addxprt(net, cred, transport, port);
> > +}
> > +
> >  static ssize_t __write_ports(struct file *file, char *buf, size_t size,
> >  			     struct net *net)
> >  {
> > @@ -1862,6 +1871,87 @@ int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> >  	return ret;
> >  }
> >  
> > +/**
> > + * nfsd_nl_listener_start_doit - start the provided nfs server listener
> > + * @skb: reply buffer
> > + * @info: netlink metadata and command arguments
> > + *
> > + * Return 0 on success or a negative errno.
> > + */
> > +int nfsd_nl_listener_start_doit(struct sk_buff *skb, struct genl_info *info)
> > +{
> > +	int ret;
> > +
> > +	if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME) ||
> > +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_LISTENER_PORT))
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&nfsd_mutex);
> > +	ret = ___write_ports_addxprt(genl_info_net(info), get_current_cred(),
> > +			nla_data(info->attrs[NFSD_A_SERVER_LISTENER_TRANSPORT_NAME]),
> > +			nla_get_u32(info->attrs[NFSD_A_SERVER_LISTENER_PORT]));
> > +	mutex_unlock(&nfsd_mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * nfsd_nl_version_get_dumpit - Handle listener_get dumpit
> > + * @skb: reply buffer
> > + * @cb: netlink metadata and command arguments
> > + *
> > + * Returns the size of the reply or a negative errno.
> > + */
> > +int nfsd_nl_listener_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 = 0, ret = -ENOMEM;
> > +	struct svc_xprt *xprt;
> > +	struct svc_serv *serv;
> > +
> > +	mutex_lock(&nfsd_mutex);
> > +
> > +	serv = nn->nfsd_serv;
> > +	if (!serv) {
> > +		mutex_unlock(&nfsd_mutex);
> > +		return 0;
> > +	}
> > +
> > +	spin_lock_bh(&serv->sv_lock);
> > +	list_for_each_entry(xprt, &serv->sv_permsocks, xpt_list) {
> > +		void *hdr;
> > +
> > +		if (i < cb->args[0]) /* already consumed */
> > +			continue;
> > +
> > +		hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > +				  cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > +				  0, NFSD_CMD_LISTENER_GET);
> > +		if (!hdr)
> > +			goto out;
> > +
> > +		if (nla_put_string(skb, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME,
> > +				   xprt->xpt_class->xcl_name))
> > +			goto out;
> > +
> > +		if (nla_put_u32(skb, NFSD_A_SERVER_LISTENER_PORT,
> > +				svc_xprt_local_port(xprt)))
> > +			goto out;
> > +
> > +		genlmsg_end(skb, hdr);
> > +		i++;
> > +	}
> > +	cb->args[0] = i;
> > +	ret = skb->len;
> > +out:
> > +	spin_unlock_bh(&serv->sv_lock);
> > +
> > +	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 1b3340f31baa..61f4c5b50ecb 100644
> > --- a/include/uapi/linux/nfsd_netlink.h
> > +++ b/include/uapi/linux/nfsd_netlink.h
> > @@ -45,12 +45,22 @@ enum {
> >  	NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> >  };
> >  
> > +enum {
> > +	NFSD_A_SERVER_LISTENER_TRANSPORT_NAME = 1,
> > +	NFSD_A_SERVER_LISTENER_PORT,
> > +
> > +	__NFSD_A_SERVER_LISTENER_MAX,
> > +	NFSD_A_SERVER_LISTENER_MAX = (__NFSD_A_SERVER_LISTENER_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_LISTENER_START,
> > +	NFSD_CMD_LISTENER_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 4cb71c3cd18d..167e404c9e20 100644
> > --- a/tools/net/ynl/generated/nfsd-user.c
> > +++ b/tools/net/ynl/generated/nfsd-user.c
> > @@ -19,6 +19,8 @@ static const char * const nfsd_op_strmap[] = {
> >  	[NFSD_CMD_THREADS_GET] = "threads-get",
> >  	[NFSD_CMD_VERSION_SET] = "version-set",
> >  	[NFSD_CMD_VERSION_GET] = "version-get",
> > +	[NFSD_CMD_LISTENER_START] = "listener-start",
> > +	[NFSD_CMD_LISTENER_GET] = "listener-get",
> >  };
> >  
> >  const char *nfsd_op_str(int op)
> > @@ -71,6 +73,16 @@ struct ynl_policy_nest nfsd_server_version_nest = {
> >  	.table = nfsd_server_version_policy,
> >  };
> >  
> > +struct ynl_policy_attr nfsd_server_listener_policy[NFSD_A_SERVER_LISTENER_MAX + 1] = {
> > +	[NFSD_A_SERVER_LISTENER_TRANSPORT_NAME] = { .name = "transport-name", .type = YNL_PT_NUL_STR, },
> > +	[NFSD_A_SERVER_LISTENER_PORT] = { .name = "port", .type = YNL_PT_U32, },
> > +};
> > +
> > +struct ynl_policy_nest nfsd_server_listener_nest = {
> > +	.max_attr = NFSD_A_SERVER_LISTENER_MAX,
> > +	.table = nfsd_server_listener_policy,
> > +};
> > +
> >  /* Common nested types */
> >  /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> >  /* NFSD_CMD_RPC_STATUS_GET - dump */
> > @@ -371,6 +383,75 @@ struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> >  	return NULL;
> >  }
> >  
> > +/* ============== NFSD_CMD_LISTENER_START ============== */
> > +/* NFSD_CMD_LISTENER_START - do */
> > +void nfsd_listener_start_req_free(struct nfsd_listener_start_req *req)
> > +{
> > +	free(req->transport_name);
> > +	free(req);
> > +}
> > +
> > +int nfsd_listener_start(struct ynl_sock *ys,
> > +			struct nfsd_listener_start_req *req)
> > +{
> > +	struct nlmsghdr *nlh;
> > +	int err;
> > +
> > +	nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_LISTENER_START, 1);
> > +	ys->req_policy = &nfsd_server_listener_nest;
> > +
> > +	if (req->_present.transport_name_len)
> > +		mnl_attr_put_strz(nlh, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME, req->transport_name);
> > +	if (req->_present.port)
> > +		mnl_attr_put_u32(nlh, NFSD_A_SERVER_LISTENER_PORT, req->port);
> > +
> > +	err = ynl_exec(ys, nlh, NULL);
> > +	if (err < 0)
> > +		return -1;
> > +
> > +	return 0;
> > +}
> > +
> > +/* ============== NFSD_CMD_LISTENER_GET ============== */
> > +/* NFSD_CMD_LISTENER_GET - dump */
> > +void nfsd_listener_get_list_free(struct nfsd_listener_get_list *rsp)
> > +{
> > +	struct nfsd_listener_get_list *next = rsp;
> > +
> > +	while ((void *)next != YNL_LIST_END) {
> > +		rsp = next;
> > +		next = rsp->next;
> > +
> > +		free(rsp->obj.transport_name);
> > +		free(rsp);
> > +	}
> > +}
> > +
> > +struct nfsd_listener_get_list *nfsd_listener_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_listener_get_list);
> > +	yds.cb = nfsd_listener_get_rsp_parse;
> > +	yds.rsp_cmd = NFSD_CMD_LISTENER_GET;
> > +	yds.rsp_policy = &nfsd_server_listener_nest;
> > +
> > +	nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_LISTENER_GET, 1);
> > +
> > +	err = ynl_exec_dump(ys, nlh, &yds);
> > +	if (err < 0)
> > +		goto free_list;
> > +
> > +	return yds.first;
> > +
> > +free_list:
> > +	nfsd_listener_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 e61c5a9e46fb..da3aaaf3f6c0 100644
> > --- a/tools/net/ynl/generated/nfsd-user.h
> > +++ b/tools/net/ynl/generated/nfsd-user.h
> > @@ -166,4 +166,58 @@ 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);
> >  
> > +/* ============== NFSD_CMD_LISTENER_START ============== */
> > +/* NFSD_CMD_LISTENER_START - do */
> > +struct nfsd_listener_start_req {
> > +	struct {
> > +		__u32 transport_name_len;
> > +		__u32 port:1;
> > +	} _present;
> > +
> > +	char *transport_name;
> > +	__u32 port;
> > +};
> 
> How do you deconfigure a listener with this interface? i.e. suppose I
> want to stop nfsd from listening on a particular port? I think this too
> is a place where a declarative interface would be better:

Is it possible with current APIs? as for 2/3 so far I have just added netlink
counter for current implementation but I am fine to change the logic here to
better APIs.

Regards,
Lorenzo

> 
> Have userland send down a list of the ports that we should currently be
> listening on, and let the kernel do the work to match the request. Again
> too, an empty list could mean "close everything".
> 
> > +
> > +static inline struct nfsd_listener_start_req *
> > +nfsd_listener_start_req_alloc(void)
> > +{
> > +	return calloc(1, sizeof(struct nfsd_listener_start_req));
> > +}
> > +void nfsd_listener_start_req_free(struct nfsd_listener_start_req *req);
> > +
> > +static inline void
> > +nfsd_listener_start_req_set_transport_name(struct nfsd_listener_start_req *req,
> > +					   const char *transport_name)
> > +{
> > +	free(req->transport_name);
> > +	req->_present.transport_name_len = strlen(transport_name);
> > +	req->transport_name = malloc(req->_present.transport_name_len + 1);
> > +	memcpy(req->transport_name, transport_name, req->_present.transport_name_len);
> > +	req->transport_name[req->_present.transport_name_len] = 0;
> > +}
> > +static inline void
> > +nfsd_listener_start_req_set_port(struct nfsd_listener_start_req *req,
> > +				 __u32 port)
> > +{
> > +	req->_present.port = 1;
> > +	req->port = port;
> > +}
> > +
> > +/*
> > + * start server listener
> > + */
> > +int nfsd_listener_start(struct ynl_sock *ys,
> > +			struct nfsd_listener_start_req *req);
> > +
> > +/* ============== NFSD_CMD_LISTENER_GET ============== */
> > +/* NFSD_CMD_LISTENER_GET - dump */
> > +struct nfsd_listener_get_list {
> > +	struct nfsd_listener_get_list *next;
> > +	struct nfsd_listener_get_rsp obj __attribute__ ((aligned (8)));
> > +};
> > +
> > +void nfsd_listener_get_list_free(struct nfsd_listener_get_list *rsp);
> > +
> > +struct nfsd_listener_get_list *nfsd_listener_get_dump(struct ynl_sock *ys);
> > +
> >  #endif /* _LINUX_NFSD_GEN_H */
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
>
Jeff Layton Nov. 30, 2023, 4:22 p.m. UTC | #3
On Thu, 2023-11-30 at 10:57 +0100, Lorenzo Bianconi wrote:
> > > 
> > > +/* ============== NFSD_CMD_LISTENER_START ============== */
> > > +/* NFSD_CMD_LISTENER_START - do */
> > > +struct nfsd_listener_start_req {
> > > +	struct {
> > > +		__u32 transport_name_len;
> > > +		__u32 port:1;
> > > +	} _present;
> > > +
> > > +	char *transport_name;
> > > +	__u32 port;
> > > +};
> > 
> > How do you deconfigure a listener with this interface? i.e. suppose I
> > want to stop nfsd from listening on a particular port? I think this too
> > is a place where a declarative interface would be better:
> 
> Is it possible with current APIs? as for 2/3 so far I have just added netlink
> counter for current implementation but I am fine to change the logic here to
> better APIs.
> 
> > 

No, I don't think you can do this with the current API at all. I
consider it a major deficiency. I don't think we want to repeat that
mistake in the new interface.

> > Have userland send down a list of the ports that we should currently be
> > listening on, and let the kernel do the work to match the request. Again
> > too, an empty list could mean "close everything".
> > 
> >
Jeff Layton Nov. 30, 2023, 4:55 p.m. UTC | #4
On Thu, 2023-11-30 at 11:22 -0500, Jeff Layton wrote:
> On Thu, 2023-11-30 at 10:57 +0100, Lorenzo Bianconi wrote:
> > > > 
> > > > +/* ============== NFSD_CMD_LISTENER_START ============== */
> > > > +/* NFSD_CMD_LISTENER_START - do */
> > > > +struct nfsd_listener_start_req {
> > > > +	struct {
> > > > +		__u32 transport_name_len;
> > > > +		__u32 port:1;
> > > > +	} _present;
> > > > +
> > > > +	char *transport_name;
> > > > +	__u32 port;
> > > > +};
> > > 
> > > How do you deconfigure a listener with this interface? i.e. suppose I
> > > want to stop nfsd from listening on a particular port? I think this too
> > > is a place where a declarative interface would be better:
> > 
> > Is it possible with current APIs? as for 2/3 so far I have just added netlink
> > counter for current implementation but I am fine to change the logic here to
> > better APIs.
> > 
> > > 
> 
> No, I don't think you can do this with the current API at all. I
> consider it a major deficiency. I don't think we want to repeat that
> mistake in the new interface.
> 
> > > Have userland send down a list of the ports that we should currently be
> > > listening on, and let the kernel do the work to match the request. Again
> > > too, an empty list could mean "close everything".
> > > 
> > > 
> 

Another thought: should this interface also report and allow you to
specify the address to listen on?

When the write_ports interface was first created, it lacked a field for
the address to listen on. Later we added a way to just hand off a socket
to the kernel to pass that info.

I think it's possible today to send down a socket that only listens on a
particular address, and you have no real way to tell that with the
current "ports" file.

Should we instead plumb a complete struct sockaddr_storage (or some
other suitable address structure) into this interface?
Chuck Lever III Nov. 30, 2023, 5:39 p.m. UTC | #5
On Thu, Nov 30, 2023 at 11:55:29AM -0500, Jeff Layton wrote:
> On Thu, 2023-11-30 at 11:22 -0500, Jeff Layton wrote:
> > On Thu, 2023-11-30 at 10:57 +0100, Lorenzo Bianconi wrote:
> > > > > 
> > > > > +/* ============== NFSD_CMD_LISTENER_START ============== */
> > > > > +/* NFSD_CMD_LISTENER_START - do */
> > > > > +struct nfsd_listener_start_req {
> > > > > +	struct {
> > > > > +		__u32 transport_name_len;
> > > > > +		__u32 port:1;
> > > > > +	} _present;
> > > > > +
> > > > > +	char *transport_name;
> > > > > +	__u32 port;
> > > > > +};
> > > > 
> > > > How do you deconfigure a listener with this interface? i.e. suppose I
> > > > want to stop nfsd from listening on a particular port? I think this too
> > > > is a place where a declarative interface would be better:
> > > 
> > > Is it possible with current APIs? as for 2/3 so far I have just added netlink
> > > counter for current implementation but I am fine to change the logic here to
> > > better APIs.
> > > 
> > > > 
> > 
> > No, I don't think you can do this with the current API at all. I
> > consider it a major deficiency. I don't think we want to repeat that
> > mistake in the new interface.
> > 
> > > > Have userland send down a list of the ports that we should currently be
> > > > listening on, and let the kernel do the work to match the request. Again
> > > > too, an empty list could mean "close everything".
> 
> Another thought: should this interface also report and allow you to
> specify the address to listen on?
> 
> When the write_ports interface was first created, it lacked a field for
> the address to listen on. Later we added a way to just hand off a socket
> to the kernel to pass that info.
> 
> I think it's possible today to send down a socket that only listens on a
> particular address, and you have no real way to tell that with the
> current "ports" file.

All agreed, but listening on a particular address isn't something we
need today. (Or is it?)

Does the socket-passing thing work for non socket-based transports
like RDMA? I would think that mechanism is legacy.


> Should we instead plumb a complete struct sockaddr_storage (or some
> other suitable address structure) into this interface?

How difficult would it be to add this later, when we actually have a
specific use case?
Jeff Layton Nov. 30, 2023, 6:01 p.m. UTC | #6
On Thu, 2023-11-30 at 12:39 -0500, Chuck Lever wrote:
> On Thu, Nov 30, 2023 at 11:55:29AM -0500, Jeff Layton wrote:
> > On Thu, 2023-11-30 at 11:22 -0500, Jeff Layton wrote:
> > > On Thu, 2023-11-30 at 10:57 +0100, Lorenzo Bianconi wrote:
> > > > > > 
> > > > > > +/* ============== NFSD_CMD_LISTENER_START ============== */
> > > > > > +/* NFSD_CMD_LISTENER_START - do */
> > > > > > +struct nfsd_listener_start_req {
> > > > > > +	struct {
> > > > > > +		__u32 transport_name_len;
> > > > > > +		__u32 port:1;
> > > > > > +	} _present;
> > > > > > +
> > > > > > +	char *transport_name;
> > > > > > +	__u32 port;
> > > > > > +};
> > > > > 
> > > > > How do you deconfigure a listener with this interface? i.e. suppose I
> > > > > want to stop nfsd from listening on a particular port? I think this too
> > > > > is a place where a declarative interface would be better:
> > > > 
> > > > Is it possible with current APIs? as for 2/3 so far I have just added netlink
> > > > counter for current implementation but I am fine to change the logic here to
> > > > better APIs.
> > > > 
> > > > > 
> > > 
> > > No, I don't think you can do this with the current API at all. I
> > > consider it a major deficiency. I don't think we want to repeat that
> > > mistake in the new interface.
> > > 
> > > > > Have userland send down a list of the ports that we should currently be
> > > > > listening on, and let the kernel do the work to match the request. Again
> > > > > too, an empty list could mean "close everything".
> > 
> > Another thought: should this interface also report and allow you to
> > specify the address to listen on?
> > 
> > When the write_ports interface was first created, it lacked a field for
> > the address to listen on. Later we added a way to just hand off a socket
> > to the kernel to pass that info.
> > 
> > I think it's possible today to send down a socket that only listens on a
> > particular address, and you have no real way to tell that with the
> > current "ports" file.
> 
> All agreed, but listening on a particular address isn't something we
> need today. (Or is it?)
> 

It is for TCP/UDP -- see the -H option to rpc.nfsd.

> Does the socket-passing thing work for non socket-based transports
> like RDMA? I would think that mechanism is legacy.
> 

To the contrary actually. rpc.nfsd almost always does this today unless
you're configuring rdma. But, that was just a convenient way to do this
with a fd based interface.

I think with netlink, we just want to send down a list of
(transport_name, sockaddr) pairs and let the kernel open and close the
appropriate sockets. For RDMA, we can just fill out the sa_port field
and ignore the rest.

> 
> > Should we instead plumb a complete struct sockaddr_storage (or some
> > other suitable address structure) into this interface?
> 
> How difficult would it be to add this later, when we actually have a
> specific use case?


I think we have one now. rpc.nfsd passes down sockets to the kernel for
TCP and UDP listeners, so we need a way to send down a complete sockaddr
for the listeners.
Chuck Lever III Nov. 30, 2023, 6:34 p.m. UTC | #7
On Thu, Nov 30, 2023 at 01:01:14PM -0500, Jeff Layton wrote:
> On Thu, 2023-11-30 at 12:39 -0500, Chuck Lever wrote:
> > On Thu, Nov 30, 2023 at 11:55:29AM -0500, Jeff Layton wrote:
> > > On Thu, 2023-11-30 at 11:22 -0500, Jeff Layton wrote:
> > > > On Thu, 2023-11-30 at 10:57 +0100, Lorenzo Bianconi wrote:
> > > > > > > 
> > > > > > > +/* ============== NFSD_CMD_LISTENER_START ============== */
> > > > > > > +/* NFSD_CMD_LISTENER_START - do */
> > > > > > > +struct nfsd_listener_start_req {
> > > > > > > +	struct {
> > > > > > > +		__u32 transport_name_len;
> > > > > > > +		__u32 port:1;
> > > > > > > +	} _present;
> > > > > > > +
> > > > > > > +	char *transport_name;
> > > > > > > +	__u32 port;
> > > > > > > +};
> > > > > > 
> > > > > > How do you deconfigure a listener with this interface? i.e. suppose I
> > > > > > want to stop nfsd from listening on a particular port? I think this too
> > > > > > is a place where a declarative interface would be better:
> > > > > 
> > > > > Is it possible with current APIs? as for 2/3 so far I have just added netlink
> > > > > counter for current implementation but I am fine to change the logic here to
> > > > > better APIs.
> > > > > 
> > > > > > 
> > > > 
> > > > No, I don't think you can do this with the current API at all. I
> > > > consider it a major deficiency. I don't think we want to repeat that
> > > > mistake in the new interface.
> > > > 
> > > > > > Have userland send down a list of the ports that we should currently be
> > > > > > listening on, and let the kernel do the work to match the request. Again
> > > > > > too, an empty list could mean "close everything".
> > > 
> > > Another thought: should this interface also report and allow you to
> > > specify the address to listen on?
> > > 
> > > When the write_ports interface was first created, it lacked a field for
> > > the address to listen on. Later we added a way to just hand off a socket
> > > to the kernel to pass that info.
> > > 
> > > I think it's possible today to send down a socket that only listens on a
> > > particular address, and you have no real way to tell that with the
> > > current "ports" file.
> > 
> > All agreed, but listening on a particular address isn't something we
> > need today. (Or is it?)
> 
> It is for TCP/UDP -- see the -H option to rpc.nfsd.
> 
> > Does the socket-passing thing work for non socket-based transports
> > like RDMA? I would think that mechanism is legacy.
> 
> To the contrary actually. rpc.nfsd almost always does this today unless
> you're configuring rdma. But, that was just a convenient way to do this
> with a fd based interface.
> 
> I think with netlink, we just want to send down a list of
> (transport_name, sockaddr) pairs and let the kernel open and close the
> appropriate sockets. For RDMA, we can just fill out the sa_port field
> and ignore the rest.

Agreed, it would be cleaner to handle all transport classes the same
way and eventually deprecate the fd-passing mechanism.


> > > Should we instead plumb a complete struct sockaddr_storage (or some
> > > other suitable address structure) into this interface?
> > 
> > How difficult would it be to add this later, when we actually have a
> > specific use case?
> 
> I think we have one now. rpc.nfsd passes down sockets to the kernel for
> TCP and UDP listeners, so we need a way to send down a complete sockaddr
> for the listeners.

If we have a usage scenario, then it makes sense to try it now.
Simon Horman Dec. 5, 2023, 8:37 p.m. UTC | #8
On Wed, Nov 29, 2023 at 06:12:45PM +0100, Lorenzo Bianconi wrote:
> Introduce write_ports netlink command similar to the ones available
> through the procfs.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Hi Lorenzo,

some minor feedback from my side.

...

> @@ -1862,6 +1871,87 @@ int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
>  	return ret;
>  }
>  
> +/**
> + * nfsd_nl_listener_start_doit - start the provided nfs server listener
> + * @skb: reply buffer
> + * @info: netlink metadata and command arguments
> + *
> + * Return 0 on success or a negative errno.
> + */
> +int nfsd_nl_listener_start_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int ret;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME) ||
> +	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_LISTENER_PORT))
> +		return -EINVAL;
> +
> +	mutex_lock(&nfsd_mutex);
> +	ret = ___write_ports_addxprt(genl_info_net(info), get_current_cred(),
> +			nla_data(info->attrs[NFSD_A_SERVER_LISTENER_TRANSPORT_NAME]),
> +			nla_get_u32(info->attrs[NFSD_A_SERVER_LISTENER_PORT]));

gcc-13 and clang-17 W=1 builds warn that ret is set but otherwise unused in
this function.

> +	mutex_unlock(&nfsd_mutex);
> +
> +	return 0;
> +}
> +
> +/**
> + * nfsd_nl_version_get_dumpit - Handle listener_get dumpit

nit: nfsd_nl_listener_get_dumpit

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

...
diff mbox series

Patch

diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
index 6c5e42bb20f6..1c342ad3c5fa 100644
--- a/Documentation/netlink/specs/nfsd.yaml
+++ b/Documentation/netlink/specs/nfsd.yaml
@@ -80,6 +80,15 @@  attribute-sets:
       -
         name: status
         type: u8
+  -
+    name: server-listener
+    attributes:
+      -
+        name: transport-name
+        type: string
+      -
+        name: port
+        type: u32
 
 operations:
   list:
@@ -142,3 +151,22 @@  operations:
           attributes:
             - major
             - minor
+    -
+      name: listener-start
+      doc: start server listener
+      attribute-set: server-listener
+      flags: [ admin-perm ]
+      do:
+        request:
+          attributes:
+            - transport-name
+            - port
+    -
+      name: listener-get
+      doc: dump server listeners
+      attribute-set: server-listener
+      dump:
+        reply:
+          attributes:
+            - transport-name
+            - port
diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
index 0608a7bd193b..cd51393ede72 100644
--- a/fs/nfsd/netlink.c
+++ b/fs/nfsd/netlink.c
@@ -22,6 +22,12 @@  static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_
 	[NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
 };
 
+/* NFSD_CMD_LISTENER_START - do */
+static const struct nla_policy nfsd_listener_start_nl_policy[NFSD_A_SERVER_LISTENER_PORT + 1] = {
+	[NFSD_A_SERVER_LISTENER_TRANSPORT_NAME] = { .type = NLA_NUL_STRING, },
+	[NFSD_A_SERVER_LISTENER_PORT] = { .type = NLA_U32, },
+};
+
 /* Ops table for nfsd */
 static const struct genl_split_ops nfsd_nl_ops[] = {
 	{
@@ -55,6 +61,18 @@  static const struct genl_split_ops nfsd_nl_ops[] = {
 		.dumpit	= nfsd_nl_version_get_dumpit,
 		.flags	= GENL_CMD_CAP_DUMP,
 	},
+	{
+		.cmd		= NFSD_CMD_LISTENER_START,
+		.doit		= nfsd_nl_listener_start_doit,
+		.policy		= nfsd_listener_start_nl_policy,
+		.maxattr	= NFSD_A_SERVER_LISTENER_PORT,
+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
+	},
+	{
+		.cmd	= NFSD_CMD_LISTENER_GET,
+		.dumpit	= nfsd_nl_listener_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 7d203cec08e4..9a51cb83f343 100644
--- a/fs/nfsd/netlink.h
+++ b/fs/nfsd/netlink.h
@@ -21,6 +21,9 @@  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);
+int nfsd_nl_listener_start_doit(struct sk_buff *skb, struct genl_info *info);
+int nfsd_nl_listener_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 f04430f79687..53129b5b7d3c 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -721,18 +721,16 @@  static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
  * A transport listener is added by writing its transport name and
  * a port number.
  */
-static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cred *cred)
+static ssize_t ___write_ports_addxprt(struct net *net, const struct cred *cred,
+				      const char *transport, const int port)
 {
-	char transport[16];
-	struct svc_xprt *xprt;
-	int port, err;
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
-
-	if (sscanf(buf, "%15s %5u", transport, &port) != 2)
-		return -EINVAL;
+	struct svc_xprt *xprt;
+	int err;
 
 	if (port < 1 || port > USHRT_MAX)
 		return -EINVAL;
+
 	trace_nfsd_ctl_ports_addxprt(net, transport, port);
 
 	err = nfsd_create_serv(net);
@@ -765,6 +763,17 @@  static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
 	return err;
 }
 
+static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cred *cred)
+{
+	char transport[16];
+	int port;
+
+	if (sscanf(buf, "%15s %5u", transport, &port) != 2)
+		return -EINVAL;
+
+	return ___write_ports_addxprt(net, cred, transport, port);
+}
+
 static ssize_t __write_ports(struct file *file, char *buf, size_t size,
 			     struct net *net)
 {
@@ -1862,6 +1871,87 @@  int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
 	return ret;
 }
 
+/**
+ * nfsd_nl_listener_start_doit - start the provided nfs server listener
+ * @skb: reply buffer
+ * @info: netlink metadata and command arguments
+ *
+ * Return 0 on success or a negative errno.
+ */
+int nfsd_nl_listener_start_doit(struct sk_buff *skb, struct genl_info *info)
+{
+	int ret;
+
+	if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME) ||
+	    GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_LISTENER_PORT))
+		return -EINVAL;
+
+	mutex_lock(&nfsd_mutex);
+	ret = ___write_ports_addxprt(genl_info_net(info), get_current_cred(),
+			nla_data(info->attrs[NFSD_A_SERVER_LISTENER_TRANSPORT_NAME]),
+			nla_get_u32(info->attrs[NFSD_A_SERVER_LISTENER_PORT]));
+	mutex_unlock(&nfsd_mutex);
+
+	return 0;
+}
+
+/**
+ * nfsd_nl_version_get_dumpit - Handle listener_get dumpit
+ * @skb: reply buffer
+ * @cb: netlink metadata and command arguments
+ *
+ * Returns the size of the reply or a negative errno.
+ */
+int nfsd_nl_listener_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 = 0, ret = -ENOMEM;
+	struct svc_xprt *xprt;
+	struct svc_serv *serv;
+
+	mutex_lock(&nfsd_mutex);
+
+	serv = nn->nfsd_serv;
+	if (!serv) {
+		mutex_unlock(&nfsd_mutex);
+		return 0;
+	}
+
+	spin_lock_bh(&serv->sv_lock);
+	list_for_each_entry(xprt, &serv->sv_permsocks, xpt_list) {
+		void *hdr;
+
+		if (i < cb->args[0]) /* already consumed */
+			continue;
+
+		hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
+				  cb->nlh->nlmsg_seq, &nfsd_nl_family,
+				  0, NFSD_CMD_LISTENER_GET);
+		if (!hdr)
+			goto out;
+
+		if (nla_put_string(skb, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME,
+				   xprt->xpt_class->xcl_name))
+			goto out;
+
+		if (nla_put_u32(skb, NFSD_A_SERVER_LISTENER_PORT,
+				svc_xprt_local_port(xprt)))
+			goto out;
+
+		genlmsg_end(skb, hdr);
+		i++;
+	}
+	cb->args[0] = i;
+	ret = skb->len;
+out:
+	spin_unlock_bh(&serv->sv_lock);
+
+	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 1b3340f31baa..61f4c5b50ecb 100644
--- a/include/uapi/linux/nfsd_netlink.h
+++ b/include/uapi/linux/nfsd_netlink.h
@@ -45,12 +45,22 @@  enum {
 	NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
 };
 
+enum {
+	NFSD_A_SERVER_LISTENER_TRANSPORT_NAME = 1,
+	NFSD_A_SERVER_LISTENER_PORT,
+
+	__NFSD_A_SERVER_LISTENER_MAX,
+	NFSD_A_SERVER_LISTENER_MAX = (__NFSD_A_SERVER_LISTENER_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_LISTENER_START,
+	NFSD_CMD_LISTENER_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 4cb71c3cd18d..167e404c9e20 100644
--- a/tools/net/ynl/generated/nfsd-user.c
+++ b/tools/net/ynl/generated/nfsd-user.c
@@ -19,6 +19,8 @@  static const char * const nfsd_op_strmap[] = {
 	[NFSD_CMD_THREADS_GET] = "threads-get",
 	[NFSD_CMD_VERSION_SET] = "version-set",
 	[NFSD_CMD_VERSION_GET] = "version-get",
+	[NFSD_CMD_LISTENER_START] = "listener-start",
+	[NFSD_CMD_LISTENER_GET] = "listener-get",
 };
 
 const char *nfsd_op_str(int op)
@@ -71,6 +73,16 @@  struct ynl_policy_nest nfsd_server_version_nest = {
 	.table = nfsd_server_version_policy,
 };
 
+struct ynl_policy_attr nfsd_server_listener_policy[NFSD_A_SERVER_LISTENER_MAX + 1] = {
+	[NFSD_A_SERVER_LISTENER_TRANSPORT_NAME] = { .name = "transport-name", .type = YNL_PT_NUL_STR, },
+	[NFSD_A_SERVER_LISTENER_PORT] = { .name = "port", .type = YNL_PT_U32, },
+};
+
+struct ynl_policy_nest nfsd_server_listener_nest = {
+	.max_attr = NFSD_A_SERVER_LISTENER_MAX,
+	.table = nfsd_server_listener_policy,
+};
+
 /* Common nested types */
 /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
 /* NFSD_CMD_RPC_STATUS_GET - dump */
@@ -371,6 +383,75 @@  struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
 	return NULL;
 }
 
+/* ============== NFSD_CMD_LISTENER_START ============== */
+/* NFSD_CMD_LISTENER_START - do */
+void nfsd_listener_start_req_free(struct nfsd_listener_start_req *req)
+{
+	free(req->transport_name);
+	free(req);
+}
+
+int nfsd_listener_start(struct ynl_sock *ys,
+			struct nfsd_listener_start_req *req)
+{
+	struct nlmsghdr *nlh;
+	int err;
+
+	nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_LISTENER_START, 1);
+	ys->req_policy = &nfsd_server_listener_nest;
+
+	if (req->_present.transport_name_len)
+		mnl_attr_put_strz(nlh, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME, req->transport_name);
+	if (req->_present.port)
+		mnl_attr_put_u32(nlh, NFSD_A_SERVER_LISTENER_PORT, req->port);
+
+	err = ynl_exec(ys, nlh, NULL);
+	if (err < 0)
+		return -1;
+
+	return 0;
+}
+
+/* ============== NFSD_CMD_LISTENER_GET ============== */
+/* NFSD_CMD_LISTENER_GET - dump */
+void nfsd_listener_get_list_free(struct nfsd_listener_get_list *rsp)
+{
+	struct nfsd_listener_get_list *next = rsp;
+
+	while ((void *)next != YNL_LIST_END) {
+		rsp = next;
+		next = rsp->next;
+
+		free(rsp->obj.transport_name);
+		free(rsp);
+	}
+}
+
+struct nfsd_listener_get_list *nfsd_listener_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_listener_get_list);
+	yds.cb = nfsd_listener_get_rsp_parse;
+	yds.rsp_cmd = NFSD_CMD_LISTENER_GET;
+	yds.rsp_policy = &nfsd_server_listener_nest;
+
+	nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_LISTENER_GET, 1);
+
+	err = ynl_exec_dump(ys, nlh, &yds);
+	if (err < 0)
+		goto free_list;
+
+	return yds.first;
+
+free_list:
+	nfsd_listener_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 e61c5a9e46fb..da3aaaf3f6c0 100644
--- a/tools/net/ynl/generated/nfsd-user.h
+++ b/tools/net/ynl/generated/nfsd-user.h
@@ -166,4 +166,58 @@  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);
 
+/* ============== NFSD_CMD_LISTENER_START ============== */
+/* NFSD_CMD_LISTENER_START - do */
+struct nfsd_listener_start_req {
+	struct {
+		__u32 transport_name_len;
+		__u32 port:1;
+	} _present;
+
+	char *transport_name;
+	__u32 port;
+};
+
+static inline struct nfsd_listener_start_req *
+nfsd_listener_start_req_alloc(void)
+{
+	return calloc(1, sizeof(struct nfsd_listener_start_req));
+}
+void nfsd_listener_start_req_free(struct nfsd_listener_start_req *req);
+
+static inline void
+nfsd_listener_start_req_set_transport_name(struct nfsd_listener_start_req *req,
+					   const char *transport_name)
+{
+	free(req->transport_name);
+	req->_present.transport_name_len = strlen(transport_name);
+	req->transport_name = malloc(req->_present.transport_name_len + 1);
+	memcpy(req->transport_name, transport_name, req->_present.transport_name_len);
+	req->transport_name[req->_present.transport_name_len] = 0;
+}
+static inline void
+nfsd_listener_start_req_set_port(struct nfsd_listener_start_req *req,
+				 __u32 port)
+{
+	req->_present.port = 1;
+	req->port = port;
+}
+
+/*
+ * start server listener
+ */
+int nfsd_listener_start(struct ynl_sock *ys,
+			struct nfsd_listener_start_req *req);
+
+/* ============== NFSD_CMD_LISTENER_GET ============== */
+/* NFSD_CMD_LISTENER_GET - dump */
+struct nfsd_listener_get_list {
+	struct nfsd_listener_get_list *next;
+	struct nfsd_listener_get_rsp obj __attribute__ ((aligned (8)));
+};
+
+void nfsd_listener_get_list_free(struct nfsd_listener_get_list *rsp);
+
+struct nfsd_listener_get_list *nfsd_listener_get_dump(struct ynl_sock *ys);
+
 #endif /* _LINUX_NFSD_GEN_H */