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

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

[Ido Schimmel]

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>
---
 include/net/sch_generic.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index d635c5b47eba..d48c657191cd 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -851,7 +851,7 @@ static inline int qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 }
 
 static inline void _bstats_update(struct gnet_stats_basic_sync *bstats,
-				  __u64 bytes, __u32 packets)
+				  __u64 bytes, __u64 packets)
 {
 	u64_stats_update_begin(&bstats->syncp);
 	u64_stats_add(&bstats->bytes, bytes);
-- 
2.48.1

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

[Simon Horman]

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>

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 4 Feb 2025 14:38:39 +0200 you 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:
> 
> [...]

Here is the summary with links:
  - [net] net: sched: Fix truncation of offloaded action statistics
    https://git.kernel.org/netdev/net/c/811b8f534fd8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html