Message-ID: <ZWhcc8384pf11sAu@lore-desk>
Date: Thu, 30 Nov 2023 10:57:07 +0100
From: Lorenzo Bianconi <lorenzo@...nel.org>
To: Jeff Layton <jlayton@...nel.org>
Cc: linux-nfs@...r.kernel.org, lorenzo.bianconi@...hat.com, neilb@...e.de,
	netdev@...r.kernel.org, kuba@...nel.org
Subject: Re: [PATCH v5 3/3] NFSD: convert write_ports to netlink command

> 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@...nel.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@...nel.org>
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists