linux-kernel - Re: [can-next,v2,6/6] can: gw: use can_gw_hops instead of sk_buff::csum

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: <4ae24c74-0f72-4565-9468-6fa43fa872a3@hartkopp.net>
Date: Wed, 28 Jan 2026 17:24:52 +0100
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Jakub Kicinski <kuba@...nel.org>, mkl@...gutronix.de
Cc: pabeni@...hat.com, davem@...emloft.net, edumazet@...gle.com,
 horms@...nel.org, kernel@...gutronix.de, mailhol@...nel.org,
 linux-can@...r.kernel.org, linux-kernel@...r.kernel.org,
 netdev@...r.kernel.org, robin@...tonic.nl, o.rempel@...gutronix.de
Subject: Re: [can-next,v2,6/6] can: gw: use can_gw_hops instead of
 sk_buff::csum_start



On 28.01.26 17:12, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> 
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> can: gw: use can_gw_hops instead of sk_buff::csum_start
> 
> This patch moves the hop counter storage from skb->csum_start to the CAN
> skb extension field can_skb_ext::can_gw_hops.
> 
>> diff --git a/net/can/gw.c b/net/can/gw.c
>> index ad89a1913b34..2c134c9e5159 100644
>> --- a/net/can/gw.c
>> +++ b/net/can/gw.c
> 
> [ ... ]
> 
>> @@ -480,19 +480,8 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
>>   	/* Do not handle CAN frames routed more than 'max_hops' times.
>>   	 * In general we should never catch this delimiter which is intended
>>   	 * to cover a misconfiguration protection (e.g. circular CAN routes).
>> -	 *
>> -	 * The Controller Area Network controllers only accept CAN frames with
>> -	 * correct CRCs - which are not visible in the controller registers.
>> -	 * According to skbuff.h documentation the csum_start element for IP
>> -	 * checksums is undefined/unused when ip_summed == CHECKSUM_UNNECESSARY.
>> -	 * Only CAN skbs can be processed here which already have this property.
>>   	 */
>> -
>> -#define cgw_hops(skb) ((skb)->csum_start)
>> -
>> -	BUG_ON(skb->ip_summed != CHECKSUM_UNNECESSARY);
>> -
>> -	if (cgw_hops(skb) >= max_hops) {
>> +	if (csx->can_gw_hops >= max_hops) {
>              ^^^^^^^^^^^^^^^^
> Is csx->can_gw_hops initialized to zero for fresh CAN frames?
> 
> The old code used skb->csum_start which was zeroed by skbuff_clear() during
> skb allocation. The new code reads from can_skb_ext::can_gw_hops, but looking
> at alloc_can_skb() in drivers/net/can/dev/skb.c, only can_iif is initialized:
> 
>      csx = can_skb_ext_add(skb);
>      ...
>      csx->can_iif = dev->ifindex;
> 
> The skb extension memory comes from kmem_cache_alloc() without __GFP_ZERO in
> __skb_ext_alloc(), so can_gw_hops will contain garbage from the slab.

Correct. It is not only a leftover of using csum_start but also of my 
first implementation attempt using another space in struct sk_buff which 
was also zero-initialized.

I'll add some initialization in can_skb_ext_add() for the next respin.

Excellent feedback!

Many thanks,
Oliver

> 
> With default max_hops=1, if the garbage value is >= 1 (probability ~99.6%),
> the frame will be dropped at this check before any routing occurs.
> 
> Should alloc_can_skb(), alloc_canfd_skb(), and alloc_canxl_skb() initialize
> csx->can_gw_hops = 0?
> 
>>   		/* indicate deleted frames due to misconfiguration */
>>   		gwj->deleted_frames++;
>>   		return;
> 
> [ ... ]