Re: [BUG] can: RX frames silently dropped in raw_rcv() with RPS enabled since d4fb6514ff8e

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hi Jörg!

I've sent out a v2 patch set where the third patch addresses your 
reported RPS related bug.

https://patchwork.kernel.org/series/1175458/

The third patch needs patch 2 as both are changing code in can*_rcv.

Can you please test the patch set in your setup?

Many thanks,
Oliver

On 28.09.26 14:16, Oliver Hartkopp wrote:
> Hi Joerg,
> 
> many thanks for the report!
> 
> On 28.09.26 13:55, Jörg Willmann wrote:
> 
>> since commit d4fb6514ff8e ("can: use skb hash instead of private
>>   variable in headroom") we are losing received CAN frames on raw sockets
>>   when RPS is enabled on the CAN interface. The problem also shows up in
>>   the stable backports (seen on [6.12.109]).
>>
>>   Setup:
>>   - Kernel: 6.12.109
>>   - Platform: OpenWrt 25.12.5, 4 CPU cores
>>   - CAN controller / driver: mcp251xfd
>>   - OpenWrt enables packet steering (RPS) by default on multi-core
>>   systems, so rps_cpus is non-zero on can0
>>
>>   Symptom:
>>   The application on a CAN_RAW socket misses frames. /proc/net/can/stats
>>   counts more RX frames than the application receives. With
>>
>>   echo 0 > /sys/class/net/can0/queues/rx-0/rps_cpus
>>
>>   the losses disappear completely. [Optional: rate/amount of loss, number
>>   of filters on the socket, whether join_filters is used.]
>>
>>   Analysis (as far as I understand it):
>>   1. RX skbs from the driver start with skb->hash == 0.
>>   2. With RPS enabled, get_rps_cpu() calls skb_get_hash() in the
>>   netif_rx() path before can_rcv() is reached. The flow dissector
>>   finds no L3/L4 headers in CAN skbs, so every CAN frame gets the
>>   same non-zero hash with sw_hash = 1.
> 
> Ouch! I wasn't aware of this.
> 
>>   3. can_set_skb_uid() only assigns a new counter value while
>>   skb->hash is 0, so the RPS hash is kept and the frame gets no
>>   unique identifier.
> 
> Correct! That's bad and not intended here.
> 
>>   4. raw_rcv() treats a frame as a duplicate when both the skb pointer
>>   and skb->hash match the per-CPU uniq entry. Since the slab
>>   allocator often returns the same address for the next skb, a new
>>   frame with the same (constant) hash is dropped as a duplicate.
> 
> Right!
> 
>>   Before d4fb6514ff8e this could not happen because the unique id was
>>   kept in can_skb_priv::skbcnt, independent of skb->hash.
> 
> Yes. We won't get can_skb_priv::skbcnt back but I will take a look at it.
> 
> Best regards,
> Oliver
> 
>