| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZWi4am03iXb6LY01@tissot.1015granger.net>
Date: Thu, 30 Nov 2023 11:29:30 -0500
From: Chuck Lever <chuck.lever@...cle.com>
To: Lorenzo Bianconi <lorenzo@...nel.org>
Cc: Jeff Layton <jlayton@...nel.org>, linux-nfs@...r.kernel.org,
lorenzo.bianconi@...hat.com, neilb@...e.de, netdev@...r.kernel.org,
kuba@...nel.org
Subject: Re: [PATCH v5 2/3] NFSD: convert write_version to netlink command
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 <lorenzo@...nel.org>
> > > > ---
> > > > 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 <jlayton@...nel.org>
> > >
> >
> > --
> > Chuck Lever
--
Chuck Lever
Powered by blists - more mailing lists