netdev - Re: [PATCH] NFSD: convert write_threads and write_v4_end

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 22 Sep 2023 20:49:27 +0000
From: Chuck Lever III <chuck.lever@...cle.com>
To: Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
CC: Jeff Layton <jlayton@...nel.org>, Lorenzo Bianconi <lorenzo@...nel.org>,
        Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        Neil Brown
	<neilb@...e.de>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH] NFSD: convert write_threads and write_v4_end_grace to
 netlink commands



> On Sep 22, 2023, at 3:25 PM, Lorenzo Bianconi <lorenzo.bianconi@...hat.com> wrote:
> 
>> 
>> 
>>> On Sep 22, 2023, at 12:20 PM, Lorenzo Bianconi <lorenzo.bianconi@...hat.com> wrote:
>>> 
>>>> On Fri, 2023-09-22 at 14:44 +0200, Lorenzo Bianconi wrote:
>>>>> Introduce write_threads and write_v4_end_grace netlink commands similar
>>>>> to the ones available through the procfs.
>>>>> Introduce nfsd_nl_server_status_get_dumpit netlink command in order to
>>>>> report global server metadata.
>>>>> 
>>>>> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
>>>>> ---
>>>>> This patch can be tested with user-space tool reported below:
>>>>> https://github.com/LorenzoBianconi/nfsd-netlink.git
>>>>> ---
>>>>> Documentation/netlink/specs/nfsd.yaml | 33 +++++++++
>>>>> fs/nfsd/netlink.c                     | 30 ++++++++
>>>>> fs/nfsd/netlink.h                     |  5 ++
>>>>> fs/nfsd/nfsctl.c                      | 98 +++++++++++++++++++++++++++
>>>>> include/uapi/linux/nfsd_netlink.h     | 11 +++
>>>>> 5 files changed, 177 insertions(+)
>>>>> 
>>>>> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
>>>>> index 403d3e3a04f3..fa1204892703 100644
>>>>> --- a/Documentation/netlink/specs/nfsd.yaml
>>>>> +++ b/Documentation/netlink/specs/nfsd.yaml
>>>>> @@ -62,6 +62,15 @@ attribute-sets:
>>>>>        name: compound-ops
>>>>>        type: u32
>>>>>        multi-attr: true
>>>>> +  -
>>>>> +    name: server-attr
>>>>> +    attributes:
>>>>> +      -
>>>>> +        name: threads
>>>>> +        type: u16
>>>> 
>>>> 65k threads ought to be enough for anybody!
>>> 
>>> maybe u8 is fine here :)
>> 
>> 32-bit is the usual for this kind of interface. I don't think we need to go with 16-bit.
> 
> ack, fine
> 
>> 
>> 
>>>>> +      -
>>>>> +        name: v4-grace
>>>>> +        type: u8
>>>>> 
>>>>> operations:
>>>>>  list:
>>>>> @@ -72,3 +81,27 @@ operations:
>>>>>      dump:
>>>>>        pre: nfsd-nl-rpc-status-get-start
>>>>>        post: nfsd-nl-rpc-status-get-done
>>>>> +    -
>>>>> +      name: threads-set
>>>>> +      doc: set the number of running threads
>>>>> +      attribute-set: server-attr
>>>>> +      flags: [ admin-perm ]
> 
> [...]
> 
>>> 
>>> I am not sure if ynl supports a doit operation with a request with no parameters.
>>> @Chuck, Jakub: any input here?
>> 
>> I think it does, I might have done something like that for one of the
>> handshake protocol commands.
> 
> please correct me if I am wrong but in Documentation/netlink/specs/handshake.yaml
> we have accept and done operations and both of them have some parameters in the
> request field, right?

I was probably thinking of a command that did not make it into the
final upstream version of handshake.


>> But I think Jeff's right, end_grace might be better postponed. Pick any of
>> the others that you think might be easy to implement instead.
> 
> ack, fine. Do you agree to have a global container (server-status-get) for all
> the server metadata instead of adding dedicated get APIs?

At this point I would like to have a distinct command for each data
item.


> Regards,
> Lorenzo
> 
>> 
>> 
>>> Regards,
>>> Lorenzo
>>> 
>>>> 
>>>>> + return 0;
>>>>> +#else
>>>>> + return -EOPNOTSUPP;
>>>>> +#endif /* CONFIG_NFSD_V4 */
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * nfsd_nl_server_status_get_start - Prepare server_status_get dumpit
>>>>> + * @cb: netlink metadata and command arguments
>>>>> + *
>>>>> + * Return values:
>>>>> + *   %0: The server_status_get command may proceed
>>>>> + *   %-ENODEV: There is no NFSD running in this namespace
>>>>> + */
>>>>> +int nfsd_nl_server_status_get_start(struct netlink_callback *cb)
>>>>> +{
>>>>> + struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id);
>>>>> +
>>>>> + return nn->nfsd_serv ? 0 : -ENODEV;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * nfsd_nl_server_status_get_dumpit - dump server status info
>>>>> + * @skb: reply buffer
>>>>> + * @cb: netlink metadata and command arguments
>>>>> + *
>>>>> + * Returns the size of the reply or a negative errno.
>>>>> + */
>>>>> +int nfsd_nl_server_status_get_dumpit(struct sk_buff *skb,
>>>>> +     struct netlink_callback *cb)
>>>>> +{
>>>>> + struct net *net = sock_net(skb->sk);
>>>>> +#ifdef CONFIG_NFSD_V4
>>>>> + struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>>>>> +#endif /* CONFIG_NFSD_V4 */
>>>>> + 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,
>>>>> +  NFSD_CMD_SERVER_STATUS_GET);
>>>>> + if (!hdr)
>>>>> + return -ENOBUFS;
>>>>> +
>>>>> + if (nla_put_u16(skb, NFSD_A_SERVER_ATTR_THREADS, nfsd_nrthreads(net)))
>>>>> + return -ENOBUFS;
>>>>> +#ifdef CONFIG_NFSD_V4
>>>>> + if (nla_put_u8(skb, NFSD_A_SERVER_ATTR_V4_GRACE, !nn->grace_ended))
>>>>> + return -ENOBUFS;
>>>>> +#endif /* CONFIG_NFSD_V4 */
>>>>> +
>>>>> + genlmsg_end(skb, hdr);
>>>>> + cb->args[0] = 1;
>>>>> +
>>>>> + return skb->len;
>>>>> +}
>>>>> +
>>>>> /**
>>>>> * 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 c8ae72466ee6..b82fbc53d336 100644
>>>>> --- a/include/uapi/linux/nfsd_netlink.h
>>>>> +++ b/include/uapi/linux/nfsd_netlink.h
>>>>> @@ -29,8 +29,19 @@ enum {
>>>>> NFSD_A_RPC_STATUS_MAX = (__NFSD_A_RPC_STATUS_MAX - 1)
>>>>> };
>>>>> 
>>>>> +enum {
>>>>> + NFSD_A_SERVER_ATTR_THREADS = 1,
>>>>> + NFSD_A_SERVER_ATTR_V4_GRACE,
>>>>> +
>>>>> + __NFSD_A_SERVER_ATTR_MAX,
>>>>> + NFSD_A_SERVER_ATTR_MAX = (__NFSD_A_SERVER_ATTR_MAX - 1)
>>>>> +};
>>>>> +
>>>>> enum {
>>>>> NFSD_CMD_RPC_STATUS_GET = 1,
>>>>> + NFSD_CMD_THREADS_SET,
>>>>> + NFSD_CMD_V4_GRACE_RELEASE,
>>>>> + NFSD_CMD_SERVER_STATUS_GET,
>>>>> 
>>>>> __NFSD_CMD_MAX,
>>>>> NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
>>>> 
>>>> -- 
>>>> Jeff Layton <jlayton@...nel.org>
>>>> 
>> 
>> --
>> Chuck Lever


--
Chuck Lever