Re: [PATCH net] net: sched: Fix truncation of offloaded action statistics

[Simon Horman <horms@kernel.org>]

On Tue, Feb 04, 2025 at 02:38:39PM +0200, Ido Schimmel wrote:
> In case of tc offload, when user space queries the kernel for tc action
> statistics, tc will query the offloaded statistics from device drivers.
> Among other statistics, drivers are expected to pass the number of
> packets that hit the action since the last query as a 64-bit number.
> 
> Unfortunately, tc treats the number of packets as a 32-bit number,
> leading to truncation and incorrect statistics when the number of
> packets since the last query exceeds 0xffffffff:
> 
> $ tc -s filter show dev swp2 ingress
> filter protocol all pref 1 flower chain 0
> filter protocol all pref 1 flower chain 0 handle 0x1
>   skip_sw
>   in_hw in_hw_count 1
>         action order 1: mirred (Egress Redirect to device swp1) stolen
>         index 1 ref 1 bind 1 installed 58 sec used 0 sec
>         Action statistics:
>         Sent 1133877034176 bytes 536959475 pkt (dropped 0, overlimits 0 requeues 0)
> [...]
> 
> According to the above, 2111-byte packets were redirected which is
> impossible as only 64-byte packets were transmitted and the MTU was
> 1500.
> 
> Fix by treating packets as a 64-bit number:
> 
> $ tc -s filter show dev swp2 ingress
> filter protocol all pref 1 flower chain 0
> filter protocol all pref 1 flower chain 0 handle 0x1
>   skip_sw
>   in_hw in_hw_count 1
>         action order 1: mirred (Egress Redirect to device swp1) stolen
>         index 1 ref 1 bind 1 installed 61 sec used 0 sec
>         Action statistics:
>         Sent 1370624380864 bytes 21416005951 pkt (dropped 0, overlimits 0 requeues 0)
> [...]
> 
> Which shows that only 64-byte packets were redirected (1370624380864 /
> 21416005951 = 64).
> 
> Fixes: 380407023526 ("net/sched: Enable netdev drivers to update statistics of offloaded actions")
> Reported-by: Joe Botha <joe@atomic.ac>
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> Reviewed-by: Petr Machata <petrm@nvidia.com>

Thanks Ido, all,

I agree that this function operates on packets as if it was 64-bit.  And in
a quick audit it seems that all callers, except qfq_enqueue() pass a 64-bit
rather than 32-bit integer (I did not check if the values passed can indeed
exceed 0xffffffff).

I also agree that the problem was introduced by the cited commit.

Reviewed-by: Simon Horman <horms@kernel.org>