netdev - Re: [net-next 0/6] move CAN skb headroom content to skb extensions

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e2033d96-e900-4013-a18a-c2e0ffa269d3@hartkopp.net>
Date: Wed, 28 Jan 2026 17:14:24 +0100
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Florian Westphal <fw@...len.de>
Cc: Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
 linux-can@...r.kernel.org
Subject: Re: [net-next 0/6] move CAN skb headroom content to skb extensions

On 28.01.26 14:18, Florian Westphal wrote:
> Oliver Hartkopp <socketcan@...tkopp.net> wrote:
>>> When the first extensions were added all of them could be enabled
>>> at same time, but I think that has changed.
>>
>> IMO we do not need to 'union' extensions as long as automatic enum
>> calculation does it job with the enabled Kconfig options.
>>
>> My only concern would be distribution kernels that have an all-yes
>> config policy ;-)
> 
> Well, thats the norm, no?  Allmodconfig.
> So we have two issues:
> 1. Turn active_extensions in sk_buff into u16
> 2. Growing memory usage of the skb_ext blob.
> 
> No need to add this 'union' now of course, but I think its something
> that should be kept in mind: originally, all extensions could be
> turned on for the same skb, but it looks like we now have mutually
> exclusive ones.

Ok. But then I don't see any real pressure to do the extension right 
now. There doesn't seem so much changes adding new skb extensions users. 
  And we still have a free slot even if all users would have been 
enabled (which is not the case due to the mutually exclusive options).

>>    static void skb_extensions_init(void)
>>    {
>> -       BUILD_BUG_ON(SKB_EXT_NUM >= 8);
>> +       BUILD_BUG_ON(SKB_EXT_NUM > 8);
> 
> True, the last valid extension id can't be 8, but
> SKB_EXT_NUM could be.
> 
>> Should I send a proper patch?
> 
> You can, otoh I think we should have consensus on what
> to do when the 8th extension is added, do we just s/u8/u16' and
> eat the growing memory cost for the skb_ext growth, or do we go
> to a drawing board and figure out how to best merge mutually exclusive
> extensions to save space?

The good thing is that skb extensions are very efficient. Which leads to 
the bad thing that we only can detect the problems at build time with

BUILD_BUG_ON(SKB_EXT_NUM > 8);
BUILD_BUG_ON(skb_ext_total_length() > 255);

For (SKB_EXT_NUM > 8) the upgrade of active_extensions to u16 should 
simply make it. Probably with some #ifdef magic only.

But thinking about BUILD_BUG_ON(skb_ext_total_length() > 255):

Shouldn't this be SKB_EXT_CHUNKSIZEOF(struct skb_ext) + sum of the skb 
ext user data which can be up to 255 *  SKB_EXT_ALIGN_VALUE (= 2040) ???

The offset calculation with the multiplication of SKB_EXT_ALIGN_VALUE 
(== 8) is the real magic in skb_ext_get_ptr(). therefore it should be 
something like

BUILD_BUG_ON(skb_ext_total_length() - SKB_EXT_CHUNKSIZEOF(struct 
skb_ext) > 255 *  SKB_EXT_ALIGN_VALUE)

right?

We could create a new BUILD_BUG_ON with all SKB_EXT_CHUNKSIZEOF'ed skb 
ext user data sizeof(structs) being < 255 * SKB_EXT_ALIGN_VALUE

IMO there's already plenty of space.

Best regards,
Oliver