Message-ID: <3577c959.1049c7.189e032afdc.Coremail.linma@zju.edu.cn>
Date:   Fri, 11 Aug 2023 00:06:05 +0800 (GMT+08:00)
From:   "Lin Ma" <linma@....edu.cn>
To:     "Randy Dunlap" <rdunlap@...radead.org>
Cc:     "Simon Horman" <horms@...nel.org>, corbet@....net,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, void@...ifault.com, jani.nikula@...el.com,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH v2] docs: staging: add netlink attrs best practices

Hello Randy,

> > Perhaps this was discussed earlier.
> > But I'm curious to know if there is a preference for putting
> > this into staging rather than networking or elsewhere.
> 
> I don't know of any reason for it to be in staging. If there is one,
> I would like to hear about it.
> 

Sorry, no special reason for that. :D
I currently think maybe the better choice is Documentation/networking/
Thanks for pointing this out.

> >>  3 files changed, 546 insertions(+)
> >>  create mode 100644 Documentation/staging/netlink-attrs-best-practices.rst
> >>
> 
> >> diff --git a/Documentation/staging/netlink-attrs-best-practices.rst b/Documentation/staging/netlink-attrs-best-practices.rst
> >> new file mode 100644
> >> index 000000000000..7dac562bee47
> >> --- /dev/null
> >> +++ b/Documentation/staging/netlink-attrs-best-practices.rst
> >> @@ -0,0 +1,544 @@
> >> +.. SPDX-License-Identifier: BSD-3-Clause
> >> +
> >> +=================================
> >> +Netlink Attributes Best Practices
> >> +=================================
> >> +
> >> +Introduction
> >> +============
> >> +
> >> +This document serves as a guide to the best practices, or cautions, for parsing user-space-provided Netlink attributes in kernel space. The intended audience is those who want to leverage the powerful Netlink interface (mainly for classic or legacy Netlink and the general Netlink users basically don't worry about these, see penultimate section) in their code. Additionally, for those who are curious about the parsing of Netlink attributes but may feel apprehensive about delving into the code itself, this document can serve as an excellent starting point.
> > 
> > I think it is normal to linewrap rst documentation.
> > At <80 columns would be best IMHO.
> 
> absolutely.
> 
> > 
> >> +
> >> +However, if you are concerned about how to prepare Netlink messages from a user space socket instead of writing kernel code, it is recommended to read the `Netlink Handbook <https://docs.kernel.org/userspace-api/netlink/intro.html>`_ first.
> >> +
> >> +Background
> >> +==========
> >> +
> >> +Data Structures
> >> +---------------
> >> +
> >> +So what is a Netlink attribute? In simple terms, **the Netlink attribute is the structural payload carried by the Netlink message in TLV (Type-Length-Value) format** (At least it is suggested to do so). One can straightly read the comment and the code in ``include/net/netlink.h`` (most of the below content is derived from there). The following graph demonstrates the structure for the majority of messages.
> > 
> > Suggestion:
> > 
> > * straightly -> directly; or, perhaps better
> >   one can straightly -> one can
> > 
> >> +
> >> +.. code-block::
> >> +
> >> +   +-----------------+------------+--------------------------
> >> +   | Netlink Msg Hdr | Family Hdr |  Netlink Attributes  ...
> >> +   +-----------------+------------+--------------------------
> >> +                     ^            ^
> >> +
> >> +The ``^`` part will be padded to align to ``NLMSG_ALIGNTO`` (4 bytes for the Linux kernel).
> >> +
> >> +The term **majority** is used here because the structure is dependent on the specific Netlink family you are dealing with. For example, the general Netlink family (NETLINK_GENERIC) puts ``struct genlmsghdr`` in the Family Hdr location and is strictly followed by specified Netlink attributes TLV. Differently, the connector Netlink family (NETLINK_CONNECTOR) does not use Netlink attributes for transiting the payload, but straight places naked data structure after its family header ``struct cn_msg``. In general, when working with Netlink-powered code, most developers opt for Netlink attributes due to their convenience and ease of maintenance. This means is definitely okay to overlook the corner cases which may eventually be incorporated into Netlink attributes in the future.
> > 
> > Suggestions:
> > 
> > * "Differently, the connector"
> >   -> As a counter example, the connector"
> 
> s/counter example/counterexample/
> 
> > 
> > * "but straight places naked data structure after its family header"
> >   -> "but, rather, places a naked data structure immediately after its
> >       family header"
> > 
> > 
> >> +
> >> +The Netlink attribute in the Linux kernel conforms to the following structure.
> >> +
> >> +.. code-block:: c
> >> +
> >> +   // >------- nla header --------<
> >> +   // +-------------+-------------+----------- - - - ----------+
> >> +   // |  Attr Len   |  Attr Type  |          Attr Data         |
> >> +   // +-------------+-------------+----------- - - - ----------+
> >> +   // >-- 2 bytes --|-- 2 bytes --|------ Attr Len bytes ------<
> >> +
> >> +   struct nlattr {
> >> +       __u16           nla_len;
> >> +       __u16           nla_type;
> >> +   };
> >> +
> >> +The 2 bytes attr len (\ ``nla_len``\ ) indicates the entire attribute length (includes the nla header) and the other 2 bytes attr type (\ ``nla_type``\ ) is used by the kernel code to identify the expected attribute. To process these attributes, the kernel code needs to locate the specific attribute and extract the payload from it, a process known as attribute parsing. This procedure can be tedious and error-prone when done manually, so the interface provides parsers to simplify the process.
> >> +
> >> +*It is worth mentioning that if you choose to use general Netlink without a nested data structure, you don't even need to call any parsers as the interface already does this and your task will be simplified to handling the parsed result.*
> >> +
> >> +Parsers
> >> +-------
> >> +
> >> +There are several parsers available, each designed to handle a specific type of object being parsed. If you have a pointer to a Netlink message, specifically a (\ ``struct nlmsghdr *``\ ), it's likely that you'll want to call the ``nlmsg_parse`` function. The prototype for this function is as follows:
> >> +
> >> +.. code-block:: c
> >> +
> >> +   /**
> >> +    * nlmsg_parse - parse attributes of a netlink message
> >> +    * @nlh: netlink message header
> >> +    * @hdrlen: length of family specific header
> >> +    * @tb: destination array with maxtype+1 elements
> >> +    * @maxtype: maximum attribute type to be expected
> >> +    * @policy: validation policy
> >> +    * @extack: extended ACK report struct
> >> +    *
> >> +    * See nla_parse()
> >> +    */
> >> +   static inline int nlmsg_parse(const struct nlmsghdr *nlh, int hdrlen,
> >> +                     struct nlattr *tb[], int maxtype,
> >> +                     const struct nla_policy *policy,
> >> +                     struct netlink_ext_ack *extack);
> >> +
> 
> Instead of duplicating kernel-doc function comments here, it is preferable to do
> what Jakub has started to do lately:
> 
> .. kernel-doc:: include/linux/netlink.h
>    :identifiers: nlmsg_parse
> 
> Same for the functions below.
> 
> >> +Else if you have a pointer to Netlink attribute (\ ``struct nlattr *``\ ) already, the ``nla_parse``\ , or  ``nla_parse_nested`` could be your choices.
> > 
> > Suggestions:
> > 
> >   * Else -> Otherwise,
> >   * "could be your choices" -> "may  be used"
> > 
> >> +
> >> +.. code-block:: c
> >> +
> >> +   /**
> >> +    * nla_parse - Parse a stream of attributes into a tb buffer
> >> +    * @tb: destination array with maxtype+1 elements
> >> +    * @maxtype: maximum attribute type to be expected
> >> +    * @head: head of attribute stream
> >> +    * @len: length of attribute stream
> >> +    * @policy: validation policy
> >> +    * @extack: extended ACK pointer
> >> +    *
> >> +    * Parses a stream of attributes and stores a pointer to each attribute in
> >> +    * the tb array accessible via the attribute type. Attributes with a type
> >> +    * exceeding maxtype will be rejected, policy must be specified, attributes
> >> +    * will be validated in the strictest way possible.
> >> +    *
> >> +    * Returns 0 on success or a negative error code.
> >> +    */
> >> +   static inline int nla_parse(struct nlattr **tb, int maxtype,
> >> +                   const struct nlattr *head, int len,
> >> +                   const struct nla_policy *policy,
> >> +                   struct netlink_ext_ack *extack);
> >> +   /**
> >> +    * nla_parse_nested - parse nested attributes
> >> +    * @tb: destination array with maxtype+1 elements
> >> +    * @maxtype: maximum attribute type to be expected
> >> +    * @nla: attribute containing the nested attributes
> >> +    * @policy: validation policy
> >> +    * @extack: extended ACK report struct
> >> +    *
> >> +    * See nla_parse()
> >> +    */
> >> +   static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
> >> +                      const struct nlattr *nla,
> >> +                      const struct nla_policy *policy,
> >> +                      struct netlink_ext_ack *extack);
> >> +
> >> +Upon closer inspection, one will notice that the parameters for the various parsers bear a striking resemblance to one another. In fact, they share a commonality that goes beyond mere coincidence, as they all ultimately call upon the same internal parsing helper function, namely ``__nla_parse``.
> >> +
> >> +.. code-block:: c
> >> +
> >> +   int __nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
> >> +           int len, const struct nla_policy *policy, unsigned int validate,
> >> +           struct netlink_ext_ack *extack);
> >> +
> >> +The idea is straightforward since we know that adding an offset to either the Netlink message header or the nested attribute will yield the TLV format of the attributes. On this basis, we will learn how to utilize those parsers.
> 
> [snip]
> 
> -- 
> ~Randy

Cool, thanks for these awesome suggestions. I will incorporate them and 
prepare the next version tomorrow, Thanks!

Regards
Lin

Powered by blists - more mailing lists