| 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: <ZRbUp0gsLv9PqriL@tissot.1015granger.net>
Date: Fri, 29 Sep 2023 09:44:07 -0400
From: Chuck Lever <chuck.lever@...cle.com>
To: NeilBrown <neilb@...e.de>
Cc: Lorenzo Bianconi <lorenzo@...nel.org>, linux-nfs@...r.kernel.org,
lorenzo.bianconi@...hat.com, jlayton@...nel.org,
netdev@...r.kernel.org
Subject: Re: [PATCH v3] NFSD: convert write_threads, write_maxblksize and
write_maxconn to netlink commands
On Wed, Sep 27, 2023 at 09:05:10AM +1000, NeilBrown wrote:
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index b71744e355a8..07e7a09e28e3 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -1694,6 +1694,147 @@ int nfsd_nl_rpc_status_get_done(struct netlink_callback *cb)
> > return 0;
> > }
> >
> > +/**
> > + * nfsd_nl_threads_set_doit - set the number of running threads
> > + * @skb: reply buffer
> > + * @info: netlink metadata and command arguments
> > + *
> > + * Return 0 on success or a negative errno.
> > + */
> > +int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
> > +{
> > + u32 nthreads;
> > + int ret;
> > +
> > + if (!info->attrs[NFSD_A_CONTROL_PLANE_THREADS])
> > + return -EINVAL;
> > +
> > + nthreads = nla_get_u32(info->attrs[NFSD_A_CONTROL_PLANE_THREADS]);
> > +
> > + ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred());
> > + return ret == nthreads ? 0 : ret;
> > +}
> > +
> > +static int nfsd_nl_get_dump(struct sk_buff *skb, struct netlink_callback *cb,
> > + int cmd, int attr, u32 val)
> > +{
> > + void *hdr;
> > +
> > + if (cb->args[0]) /* already consumed */
> > + return 0;
> > +
> > + hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
> > + &nfsd_nl_family, NLM_F_MULTI, cmd);
> > + if (!hdr)
> > + return -ENOBUFS;
> > +
> > + if (nla_put_u32(skb, attr, val))
> > + return -ENOBUFS;
> > +
> > + genlmsg_end(skb, hdr);
> > + cb->args[0] = 1;
> > +
> > + return skb->len;
> > +}
> > +
> > +/**
> > + * nfsd_nl_threads_get_dumpit - dump the number of running threads
> > + * @skb: reply buffer
> > + * @cb: netlink metadata and command arguments
> > + *
> > + * Returns the size of the reply or a negative errno.
> > + */
> > +int nfsd_nl_threads_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> > +{
> > + return nfsd_nl_get_dump(skb, cb, NFSD_CMD_THREADS_GET,
> > + NFSD_A_CONTROL_PLANE_THREADS,
> > + nfsd_nrthreads(sock_net(skb->sk)));
> > +}
> > +
> > +/**
> > + * nfsd_nl_max_blksize_set_doit - set the nfs block size
> > + * @skb: reply buffer
> > + * @info: netlink metadata and command arguments
> > + *
> > + * Return 0 on success or a negative errno.
> > + */
> > +int nfsd_nl_max_blksize_set_doit(struct sk_buff *skb, struct genl_info *info)
> > +{
> > + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > + struct nlattr *attr = info->attrs[NFSD_A_CONTROL_PLANE_MAX_BLKSIZE];
> > + int ret = 0;
> > +
> > + if (!attr)
> > + return -EINVAL;
> > +
> > + mutex_lock(&nfsd_mutex);
> > + if (nn->nfsd_serv) {
> > + ret = -EBUSY;
> > + goto out;
> > + }
>
> This code is wrong... but then the original in write_maxblksize is wrong
> to, so you can't be blamed.
> nfsd_max_blksize applies to nfsd in ALL network namespaces. So if we
> need to check there are no active services in one namespace, we need to
> check the same for *all* namespaces.
Yes, the original code does look strange and is probably incorrect
with regard to its handling of the mutex. Shall we explore and fix
that issue in the nfsctl code first so that it can be backported to
stable kernels?
> I think we should make nfsd_max_blksize a per-namespace value.
That is a different conversation.
First, the current name of this tunable is incongruent with its
actual function, which is to specify the maximum network buffer size
that is allocated when the NFSD service pool is created. We should
find a more descriptive and specific name for this element in the
netlink protocol.
Second, it does seem like a candidate for becoming namespace-
specific, but TBH I'm not familiar enough with its current user
space consumers to know if that change would be welcome or fraught.
Since more discussion, research, and possibly a fix are needed, we
might drop max_blksize from this round and look for one or two
other tunables to convert for the first round.
> > +
> > + nfsd_max_blksize = nla_get_u32(attr);
> > + nfsd_max_blksize = max_t(int, nfsd_max_blksize, 1024);
> > + nfsd_max_blksize = min_t(int, nfsd_max_blksize, NFSSVC_MAXBLKSIZE);
> > + nfsd_max_blksize &= ~1023;
> > +out:
> > + mutex_unlock(&nfsd_mutex);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * nfsd_nl_max_blksize_get_dumpit - dump the nfs block size
> > + * @skb: reply buffer
> > + * @cb: netlink metadata and command arguments
> > + *
> > + * Returns the size of the reply or a negative errno.
> > + */
> > +int nfsd_nl_max_blksize_get_dumpit(struct sk_buff *skb,
> > + struct netlink_callback *cb)
> > +{
> > + return nfsd_nl_get_dump(skb, cb, NFSD_CMD_MAX_BLKSIZE_GET,
> > + NFSD_A_CONTROL_PLANE_MAX_BLKSIZE,
> > + nfsd_max_blksize);
> > +}
> > +
> > +/**
> > + * nfsd_nl_max_conn_set_doit - set the max number of connections
> > + * @skb: reply buffer
> > + * @info: netlink metadata and command arguments
> > + *
> > + * Return 0 on success or a negative errno.
> > + */
> > +int nfsd_nl_max_conn_set_doit(struct sk_buff *skb, struct genl_info *info)
> > +{
> > + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > + struct nlattr *attr = info->attrs[NFSD_A_CONTROL_PLANE_MAX_CONN];
> > +
> > + if (!attr)
> > + return -EINVAL;
> > +
> > + nn->max_connections = nla_get_u32(attr);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * nfsd_nl_max_conn_get_dumpit - dump the max number of connections
> > + * @skb: reply buffer
> > + * @cb: netlink metadata and command arguments
> > + *
> > + * Returns the size of the reply or a negative errno.
> > + */
> > +int nfsd_nl_max_conn_get_dumpit(struct sk_buff *skb,
> > + struct netlink_callback *cb)
> > +{
> > + struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id);
> > +
> > + return nfsd_nl_get_dump(skb, cb, NFSD_CMD_MAX_CONN_GET,
> > + NFSD_A_CONTROL_PLANE_MAX_CONN,
> > + nn->max_connections);
> > +}
> > +
> > /**
> > * nfsd_net_init - Prepare the nfsd_net portion of a new net namespace
> > * @net: a freshly-created network namespace
Powered by blists - more mailing lists