netdev - Re: [PATCH net-next 0/4] netlink: add 'bitmap' attribute type and API

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: <20220725115324.7a1ad2d6@kernel.org>
Date:   Mon, 25 Jul 2022 11:53:24 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Yury Norov <yury.norov@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Nikolay Aleksandrov <razor@...ckwall.org>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 0/4] netlink: add 'bitmap' attribute type and
 API

On Mon, 25 Jul 2022 15:02:55 +0200 Alexander Lobakin wrote:
> > Actually once a field crosses the biggest natural int size I was
> > thinking that the user would go to a bitmap.
> > 
> > So at the netlink level the field is bigint (LE, don't care about BE).
> > Kernel side API is:
> > 
> > 	nla_get_b8, nla_get_b16, nla_get_b32, nla_get_b64,
> > 	nla_get_bitmap
> > 	nla_put_b8, nla_put_b16 etc.
> > 
> > 	u16 my_flags_are_so_toight;
> > 
> > 	my_flags_are_so_toight = nla_get_b16(attr[BLAA_BLA_BLA_FLAGS]);
> > 
> > The point is - the representation can be more compact than u64 and will
> > therefore encourage anyone who doesn't have a strong reason to use
> > fixed size fields to switch to the bigint.  
> 
> Ahh looks like I got it! So you mean that at Netlink level we should
> exchange with bigint/u64arrs, but there should be an option to
> get/set only 16/32/64 bits from them to simplify (or keep simple)
> users?

Not exactly. I'd prefer if the netlink level was in u32 increments.
u64 requires padding (so the nla_put..() calls will have more args).
Netlink requires platform alignment and rounds up to 4B, so u32 is much
more convenient than u64. Similarly - it doesn't make sense to represent
sizes smaller than 4B because of the rounding up, so nla_put_b8() can
be a define to nla_put_b32(). Ethool's choice of u32 is not without
merit.

> Like, if we have `u16 uuid`, to not do
> 
> 	unsigned long uuid_bitmap;
> 
> 	nla_get_bitmap(attr[FLAGS], &uuid_bitmap, BITS_PER_TYPE(u16));
> 	uuid = (u16)uuid_bitmap;
> 
> but instead
> 
> 	uuid = nla_get_b16(attr[FLAGS]);
> 
> ?

Yes.

> > about being flexible when it comes to size, I guess, more than
> > bitmap in particular.  
> 
> Probably, but you also said above that for anything bigger than
> longs you'd go for bitmaps, didn't you? So I guess that series
> goes in the right direction, just needs a couple more inlines
> to be able to get/put u{32, 64; 8, 16 -- not sure about these two
> after reading your follow-up mail} as easy as nla_{get,put}<size>()
> and probably dropping Endianness stuff? Hope I got it right ._.

Modulo the fact that I do still want to pack to u32. Especially a
single u32 - perhaps once we cross 8B we can switch to requiring 8B
increments.

The nla_len is 16bit, which means that attrs nested inside other attrs
are quite tight for space (see the sad story of VF attrs in
RTM_GETLINK). If we don't represent u8/u16/u32 in a netlink-level
efficient manner we're back to people arguing for raw u32s rather than
using the new type.