netdev - Re: [can-next 0/5] can: remove private skb headroom infrastructure

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: <39794159-c0cc-498b-a162-44e77b6c371a@hartkopp.net>
Date: Wed, 21 Jan 2026 13:55:47 +0100
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Marc Kleine-Budde <mkl@...gutronix.de>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>
Cc: linux-can@...r.kernel.org, Oleksij Rempel <o.rempel@...gutronix.de>,
 Vincent Mailhol <mailhol@...nel.org>, netdev@...r.kernel.org,
 Eric Dumazet <edumazet@...gle.com>, Simon Horman <horms@...nel.org>,
 davem@...emloft.net
Subject: Re: [can-next 0/5] can: remove private skb headroom infrastructure

Hello Marc,

I'm not sure how intense you followed my discussion with Jakub and Paolo 
about my attempt to move the CAN skb specifc content (8 bytes) away from 
the problematic skb->head reference an hold it directly in struct sk_buff?

Meanwhile I sent a v2 patch set which has been dropped from netdev 
patchwork because of its can-next mail subject:

https://lore.kernel.org/linux-can/20260117132824.3649-1-socketcan@hartkopp.net/

I've been running the patches for quite a while now and feel very 
confident that the solution is very efficient and safe for either CAN 
skbs and non-CAN skbs.

To be more clear in the struct sk_buff changes I would change the 
comments in my next respin like this:

	union {
		/* skb->encapsulation = true */
		struct {
			/* eth/ip encapsulation / tunneling */
			union {
				__be16	inner_protocol;
				__u8	inner_ipproto;
			};

			__u16	inner_transport_header;
			__u16	inner_network_header;
			__u16	inner_mac_header;
		};

		/* skb->encapsulation = false */
#if IS_ENABLED(CONFIG_CAN)
		struct {
			/* CAN skb content (ETH_P_CAN*) */
			int	can_iif;
			__u16	can_framelen;
			__u16	can_gw_hops;
		};
#endif
	};

And I wonder if it would make sense to add a WARN_ON_ONCE() in can_rcv() 
and friends?

What is your opinion about the patch set?

Should I make it a net-next patch set to restart the discussion there?

Best regards,
Oliver


On 18.01.26 13:53, Oliver Hartkopp wrote:
> 
> On 17.01.26 18:15, Jakub Kicinski wrote:
>> On Fri, 16 Jan 2026 11:31:14 +0100 Oliver Hartkopp wrote:
>>> Long story short: Using the common pattern to wrap a union around
>>> dual-usable skb space is the most efficient and least risky solution 
>>> IMHO.
>>
>> The concern is that we're making a precedent for, let's call it -
>> not-routable-networking technology to redefine fields in skb that
>> it doesn't need. From the maintainability perspective that's a big
>> risk, IMHO. I fully acknowledge tho that using md dst will be a lot
>> more work. Which makes this situation an unpleasant judgment call :(
>>
> 
> I checked out more of the "destination cache" code, the dst_metadata and 
> its users.
> 
> And also this scary union in struct sk_buff:
> 
> union {
>          struct {
>                  unsigned long   _skb_refdst;
>                  void            (*destructor)(struct sk_buff *skb);
>          };
>          struct list_head        tcp_tsorted_anchor;
> #ifdef CONFIG_NET_SOCK_MSG
>          unsigned long           _sk_redir;
> #endif
> };
> 
> Despite the fact that the required 8 bytes content for the CAN skb has 
> nothing to do with destinations or other of these above use cases that 
> share the unsigned long pointer magic above, I doubt that the current 
> flow of CAN skbs between socket layer, driver layer and forth and back 
> would survive this.
> 
> My first approach to "just" extend the skb header space for CAN skbs did 
> not work out because people obviously have other things in mind with 
> skb->head. I'm sure I can count the days until something breaks with the 
> CAN specific SKB data when attaching them to the next infrastructure 
> build for ethernet/IP use cases for the same reason.
> 
> After all these experiences I would tend to add the required 8 bytes 
> directly to struct sk_buff covered by "#if IS_ENABLED(CONFIG_CAN)".
> 
> Or save those 8 bytes by using the "inner protocol space" and 
> additionally setting skb->encapsulation to false. Which is a safe thing 
> to protect the CAN specific content against accidentally assaults and 
> can be clearly proved to be correct.
> 
> Best regards,
> Oliver