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] [day] [month] [year] [list]

Message-ID: <20220726101748.76aaac09@kernel.org>
Date:   Tue, 26 Jul 2022 10:17:48 -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
Subject: Re: [PATCH net-next 0/4] netlink: add 'bitmap' attribute type and
 API

On Tue, 26 Jul 2022 12:41:45 +0200 Alexander Lobakin wrote:
> > 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.  
> 
> Ah, got it. I think you're right. The only thing that bothers me
> a bit is that we'll be converting arrays of u32s to unsigned longs
> each time, unlike u64s <--> longs which are 1:1 for 64 bits and LEs.

For LE the size of the word doesn't matter, right? Don't trust me on
endian questions, but I thought for LE you can just load the data as
ulongs as long as size is divisible by sizeof(ulong) and you're gonna
be good. It'd add some #ifdefs and ifs() to the bitmap code, but the
ethtool u32 conversions would probably also benefit?

> But I guess it's better anyway than waste Netlink attrs for
> paddings?

Yes, AFAIU it's pretty bad. Say you have a nest with objects with 
3 big ints inside. With u32 the size of a nest is 4 + 3*(4+4) = 28.
With u64 it's 4 + 4+8 + 2*(4+4+8) = 48 so 28 vs 48. Netlink header
being 4B almost always puts us out of alignment.

> So I'd like to summarize for a v2:
> 
> * represent as u32s, not u64s;
> * present it as "bigints", not bitmaps. It can carry bitmaps as
>   well, but also ->
> * add getters/setters for u8, 16, 32, 64s. Bitmaps are already here.
>   Probably u32 arrays as well? Just copy them directly. And maybe
>   u64 arrays, too (convert Netlink u32s to target u64s)?

I don't think we need the array helpers.

> * should I drop Endianness stuff? With it still in place, it would
>   be easier to use this new API to exchange with e.g. __be64.

If you have a use case, sure. AFAIU the explicit endian types are there
for carrying protocol fields. I don't think there are many protocols
that we deal with in netlink that have big int fields.

> * hope I didn't miss anything?

I think that's it, but I reserve the right to remember something after
you post the code :)

> Thanks a lot, some great ideas here from your side :)