netdev - Re: [RFC] netlink: add variable-length / auto integers

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]

Message-ID: <9c70c71cecf19a50c56ed57c0f99660a3176d11d.camel@sipsolutions.net>
Date: Wed, 11 Oct 2023 18:16:42 +0200
From: Johannes Berg <johannes@...solutions.net>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, nicolas.dichtel@...nd.com, fw@...len.de, 
	pablo@...filter.org, jiri@...nulli.us, mkubecek@...e.cz, 
	aleksander.lobakin@...el.com, Thomas Haller <thaller@...hat.com>
Subject: Re: [RFC] netlink: add variable-length / auto integers

On Wed, 2023-10-11 at 09:08 -0700, Jakub Kicinski wrote:
> On Wed, 11 Oct 2023 15:11:15 +0200 Johannes Berg wrote:
> > > +++ b/include/uapi/linux/netlink.h
> > > @@ -298,6 +298,8 @@ struct nla_bitfield32 {
> > >   *	entry has attributes again, the policy for those inner ones
> > >   *	and the corresponding maxtype may be specified.
> > >   * @NL_ATTR_TYPE_BITFIELD32: &struct nla_bitfield32 attribute
> > > + * @NL_ATTR_TYPE_SINT: 32-bit or 64-bit signed attribute, aligned to 4B
> > > + * @NL_ATTR_TYPE_UINT: 32-bit or 64-bit unsigned attribute, aligned to 4B  
> > 
> > This is only for exposing the policy (policy description), not sure the
> > alignment thing matters here?
> > 
> > OTOH, there's nothing in this file that ever describes *any* of the
> > attributes, yet in pracice all the uapi headers do refer to NLA_U8 and
> > similar - so we should probably have a new comment section here in the
> > UAPI that describes the various types as used by the documentation of
> > other families?
> > 
> > Anyway, I think some kind of bigger "careful with alignment" here would
> > be good, so people do the correct thing and not just "if (big)
> > nla_get_u64()" which would get the alignment thing problematic again.
> 
> I was planning to add the docs to Documentation/userspace-api/netlink/
> Is that too YNL-specific?

Oh. I guess I keep expecting that header files at least have some hints,
but whatever ... experience tells me anyway that nobody bothers reading
the comments and people just copy stuff from elsewhere, so we just have
to get this right first in one place ;-)

> diff --git a/Documentation/userspace-api/netlink/specs.rst b/Documentation/userspace-api/netlink/specs.rst
> index cc4e2430997e..a8218284e67a 100644
> --- a/Documentation/userspace-api/netlink/specs.rst
> +++ b/Documentation/userspace-api/netlink/specs.rst
> @@ -408,10 +408,21 @@ This section describes the attribute types supported by the ``genetlink``
>  compatibility level. Refer to documentation of different levels for additional
>  attribute types.
>  
> -Scalar integer types
> +Common integer types
>  --------------------
>  
> -Fixed-width integer types:
> +``sint`` and ``uint`` represent signed and unsigned 64 bit integers.
> +If the value can fit on 32 bits only 32 bits are carried in netlink
> +messages, otherwise full 64 bits are carried. Note that the payload
> +is only aligned to 4B, so the full 64 bit value may be unaligned!
> +
> +Common integer types should be preferred over fix-width types in majority
> +of cases.
> +
> +Fix-width integer types
> +-----------------------
> +
> +Fixed-width integer types include:
>  ``u8``, ``u16``, ``u32``, ``u64``, ``s8``, ``s16``, ``s32``, ``s64``.
>  
>  Note that types smaller than 32 bit should be avoided as using them
> @@ -421,6 +432,9 @@ See :ref:`pad_type` for padding of 64 bit attributes.
>  The payload of the attribute is the integer in host order unless ``byte-order``
>  specifies otherwise.
>  
> +64 bit values are usually aligned by the kernel but it is recommended
> +that the user space is able to deal with unaligned values.
> +
>  .. _pad_type:
> 

That does look good though :)

johannes