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: <f2d293c1-bc6a-4130-b544-2216ec0b0590@hartkopp.net>
Date: Fri, 16 Jan 2026 11:31:14 +0100
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Paolo Abeni <pabeni@...hat.com>, linux-can@...r.kernel.org,
 Marc Kleine-Budde <mkl@...gutronix.de>, Jakub Kicinski <kuba@...nel.org>
Cc: 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 Paolo,

freshly created CAN skbs only contain a fixed struct can_frame (16 
byte!) where dev/priority/mark/tstamp are set together with

skb->protocol = htons(ETH_P_CAN);
skb->ip_summed = CHECKSUM_UNNECESSARY;
skb->pkt_type = PACKET_LOOPBACK;

All other settings that are relevant to ethernet/IP are unused and left 
at their initialization values (e.g. network/mac/transport headers or 
inner protocol values).

A single CAN skb can be passed to the driver layer and back several 
times. Because we need to place some additional data along with CAN skbs 
this was formerly stored in a 16 byte private skb headroom (struct 
can_skb_priv).

IIRC we had three issues (KMSAN, etc) with the headroom as someone 
between netif_rx() and can_rcv() was using the headroom for his purposes 
so that the access to struct can_skb_priv via skb->head was broken and 
not reliable for CAN skbs.

Skbs are mostly used for ethernet/IP and developers do not really 
know/care about CAN skbs. That's why this patch set aims to remove 
private CAN skb headroom infrastructure - and to minimize the (risky) 
interaction with other ethernet/IP code.

On 15.01.26 16:37, Paolo Abeni wrote:

> Could you please explain in details why the metadata_dst option has been
> deemed unsuitable?!? I *think* something vaguely alike the following
> would do?!?
> 
> ---
> diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
> index 1fc2fb03ce3f..d6ee45631fea 100644
> --- a/include/net/dst_metadata.h
> +++ b/include/net/dst_metadata.h
> @@ -13,6 +13,13 @@ enum metadata_type {
>   	METADATA_HW_PORT_MUX,
>   	METADATA_MACSEC,
>   	METADATA_XFRM,
> +	METADATA_CAN,
> +};
> +
> +struct can_md_info {
> +	int can_iif;
> +	int len;
> +	int uid;
>   };
> 
>   struct hw_port_info {
> @@ -38,6 +45,7 @@ struct metadata_dst {
>   		struct hw_port_info	port_info;
>   		struct macsec_info	macsec_info;
>   		struct xfrm_md_info	xfrm_info;
> +		struct can_md_info	can_info;
>   	} u;
>   };
> 

Yes. I came to the same simple extensions for data structures but then 
looked into dst_metadata.h and the users code with mallocs, per_cpu 
code, unclone, refcounts, etc. - which was hard to understand for me and 
introduced complexity that is again needed and maintained by ethernet/IP 
users only. Not really appropriate for a CAN skb that transports 16 byte 
of data IMO.

For that reason I propose the common pattern to wrap a union around 
dual-usable skb space, which is simple efficient and easy to understand.

On 15.01.26 16:37, Paolo Abeni wrote:

 > I don't like much that the CAN information are scattered in different
 > places (skb->hash and tunnel header section).

This is not the case. According to the documentation the skb->hash is a 
value used for RPS to identify skbs. We would use it as intended.

And the tunnel header section is marked unused in CAN skbs. By setting 
"skb->encapsulation" to false (the init value) this section is not read 
by anyone. Wrapping a union around this dual-usable skb space is a safe 
solution here.

 > Also it's unclear to me if
 > a can bus skb could end-up landing (even via completely
 > insane/intentionally evil configuration/setup) in a plain netdev 
interface.
 >
 > In the such a case this solution will be problematic.

The CAN drivers and the CAN network layer code always checks the 
processed skbs for ETH_P_[CAN|CANFD|CANXL] and ARPHDR_CAN. So CAN skbs 
created by the CAN netlayer can only be sent to ARPHDR_CAN devices.

The only way to create weird CAN skbs is via PF_PACKET sockets that 
sends ETH_P_CAN skbs to ethernet devices. Beyond such PF_PACKET skbs the 
now suggested CAN skbs would not harm any driver or network layer as the 
described skb settings do not have any problematic content.

Netdev drivers can cope with it and the netlayer code using 
ETH_P_[CAN|CANFD|CANXL] or ETH_P_ALL is fine with it too.

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.

Best regards,
Oliver