[NETWORKING] IPSTATS_MIB_OUTOCTETS increment is duplicated in SNMP

[NETWORKING] IPSTATS_MIB_OUTOCTETS increment is duplicated in SNMP

[heng guo]

[-- Attachment #1.1: Type: text/plain, Size: 8469 bytes --]

Hi maintainers,

We find IPSTATS_MIB_OUTOCTETS increment is duplicated in SNMP test.


*Test environment:*

Linux version: 5.10.154
Platform: ARM nxp-ls1028


*Issue description:*

As the attached network structure, SNMP is testing using tools scapy.
Test command is
"send(IP(dst="10.225.35.4",flags=0)/UDP()/Raw(load='0'*1400), count=1,
inter=1.000000)"
The test result shows that IPSTATS_MIB_OUTOCTETS increment is duplicated.


*Debug:*

Add dump_stack() in both ip_forward_finish() and ip_output(), also add
debug logs in __SNMP_UPD_PO_STATS and __IP_ADD_STATS. Then get below
test logs with core stack.
----------------------------------------------------------------------
kernel: ip_forward: NF_INET_FORWARD
kernel: ip_forward_finish: increase IPSTATS_MIB_OUTOCTETS 1428
kernel: CPU: 0 PID: 12 Comm: ksoftirqd/0 Tainted: G        W  O
5.10.154-yocto-standard-wrl10.21.20.15 #1
kernel: Hardware name: freescale ls1028a/ls1028a, BIOS 
2019.10+fsl+gd1aa7f1b3e
kernel: Call trace:
kernel: dump_backtrace+0x0/0x1a0
kernel: show_stack+0x20/0x30
kernel: dump_stack+0xcc/0x108
kernel: ip_forward_finish+0x178/0x1d4
kernel: ip_forward+0x59c/0x5fc
kernel: ip_sublist_rcv_finish+0x50/0x70
kernel: ip_sublist_rcv+0x154/0x190
kernel: ip_list_rcv+0x100/0x1d0
kernel: __netif_receive_skb_list_core+0x188/0x21c
kernel: netif_receive_skb_list_internal+0x1d8/0x2e4
kernel: napi_complete_done+0x70/0x1d4
kernel: gro_cell_poll+0x88/0xac
kernel: net_rx_action+0x1b0/0x4fc
kernel: __do_softirq+0x188/0x464
kernel: run_ksoftirqd+0x68/0x80
kernel: smpboot_thread_fn+0x204/0x240
kernel: kthread+0x14c/0x160
kernel: ret_from_fork+0x10/0x30
kernel: __IP_ADD_STATS, field:200, val:1428
kernel: __SNMP_ADD_STATS: mid:200, val:1428
kernel: __SNMP_ADD_STATS: mid:200, val:1428
kernel: ip_output: increase IPSTATS_MIB_OUT 1428
kernel: CPU: 0 PID: 12 Comm: ksoftirqd/0 Tainted: G        W  O
5.10.154-yocto-standard-wrl10.21.20.15 #1
kernel: Hardware name: freescale ls1028a/ls1028a, BIOS 
2019.10+fsl+gd1aa7f1b3e
kernel: Call trace:
kernel: dump_backtrace+0x0/0x1a0
kernel: show_stack+0x20/0x30
kernel: dump_stack+0xcc/0x108
kernel: ip_output+0x2cc/0x2e4
kernel: ip_forward_finish+0x1b0/0x1d4
kernel: ip_forward+0x59c/0x5fc
kernel: ip_sublist_rcv_finish+0x50/0x70
kernel: ip_sublist_rcv+0x154/0x190
kernel: ip_list_rcv+0x100/0x1d0
kernel: __netif_receive_skb_list_core+0x188/0x21c
kernel: netif_receive_skb_list_internal+0x1d8/0x2e4
kernel: napi_complete_done+0x70/0x1d4
kernel: gro_cell_poll+0x88/0xac
kernel: net_rx_action+0x1b0/0x4fc
kernel: __do_softirq+0x188/0x464
kernel: run_ksoftirqd+0x68/0x80
kernel: smpboot_thread_fn+0x204/0x240
kernel: kthread+0x14c/0x160
kernel: ret_from_fork+0x10/0x30
kernel: SNMP_UPD_PO_STATS: mid:200, val:1428
kernel: SNMP_UPD_PO_STATS: mid:200, val:1428
kernel: SNMP_UPD_PO_STATS: mid:200, val:1428
kernel: net_dev_queue: { len:1428, name:tn_a.1073, network_header_type:
IPV4, protocol: udp, saddr:[10.225.35.20], daddr:[10.225.35.4] }
kernel: net_dev_queue: { len:1428, name:tn_a, network_header_type: IPV4,
protocol: udp, saddr:[10.225.35.20], daddr:[10.225.35.4] }
-----------------------------------------------------------------------
So the IPSTATS_MIB_OUTOCTETS is counted in both ip_forward_finish() and
ip_output().


*Possible cause:*

commit edf391ff1723 ("snmp: add missing counters for RFC 4293") had
already added OutOctets for RFC 4293. In commit 2d8dbb04c63e ("snmp: fix
OutOctets counter to include forwarded datagrams"), OutOctets was
counted again, but not removed from ip_output().

And according to RFC 4293 "3.2.3. IP Statistics Tables"(Case-diagram.png),
ipIfStatsOutTransmits is not equal to ipIfStatsOutForwDatagrams. So
"IPSTATS_MIB_OUTOCTETS must be incremented when incrementing" is not
accurate. And IPSTATS_MIB_OUTOCTETS should be counted after fragment.

So do a fix patch for latest master branch, please help review it.

Thanks,
Heng


*Patch:*
------------------------------------------------------
 From ffe4b1107147562c5900fdc0c2c7dcd8ed9d1f37 Mon Sep 17 00:00:00 2001
From: Heng Guo <heng.guo@windriver.com>
Date: Mon, 14 Aug 2023 14:49:47 +0800
Subject: [PATCH] SNMP: IPSTATS_MIB_OUTOCTETS increment duplicated

commit edf391ff1723 ("snmp: add missing counters for RFC 4293") had
already added OutOctets for RFC 4293. In commit 2d8dbb04c63e ("snmp: fix
OutOctets counter to include forwarded datagrams"), OutOctets was
counted again, but not removed from ip_output().

According to RFC 4293 "3.2.3. IP Statistics Tables",
ipipIfStatsOutTransmits is not equal to ipIfStatsOutForwDatagrams. So
"IPSTATS_MIB_OUTOCTETS must be incremented when incrementing" is not
accurate. And IPSTATS_MIB_OUTOCTETS should be counted after fragment.

This patch revert commit 2d8dbb04c63e and move IPSTATS_MIB_OUTOCTETS to
ip_finish_output2 for ipv4.

Reviewed-by: Filip Pudak <filip.pudak@windriver.com>
Signed-off-by: Heng Guo <heng.guo@windriver.com>
---
  net/ipv4/ip_forward.c | 1 -
  net/ipv4/ip_output.c  | 7 +++----
  net/ipv4/ipmr.c       | 1 -
  net/ipv6/ip6_output.c | 1 -
  net/ipv6/ip6mr.c      | 2 --
  5 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
index 29730edda220..0694f69ab25b 100644
--- a/net/ipv4/ip_forward.c
+++ b/net/ipv4/ip_forward.c
@@ -67,7 +67,6 @@ static int ip_forward_finish(struct net *net, struct 
sock *sk, struct sk_buff *s
      struct ip_options *opt    = &(IPCB(skb)->opt);

      __IP_INC_STATS(net, IPSTATS_MIB_OUTFORWDATAGRAMS);
-    __IP_ADD_STATS(net, IPSTATS_MIB_OUTOCTETS, skb->len);

  #ifdef CONFIG_NET_SWITCHDEV
      if (skb->offload_l3_fwd_mark) {
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index ae8a456df5ab..f983d221eb92 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -205,6 +205,9 @@ static int ip_finish_output2(struct net *net, struct 
sock *sk, struct sk_buff *s
      } else if (rt->rt_type == RTN_BROADCAST)
          IP_UPD_PO_STATS(net, IPSTATS_MIB_OUTBCAST, skb->len);

+    /* OUTOCTETS should be counted after fragment */
+    IP_UPD_PO_STATS(net, IPSTATS_MIB_OUT, skb->len);
+
      if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) {
          skb = skb_expand_head(skb, hh_len);
          if (!skb)
@@ -364,8 +367,6 @@ int ip_mc_output(struct net *net, struct sock *sk, 
struct sk_buff *skb)
      /*
       *    If the indicated interface is up and running, send the packet.
       */
-    IP_UPD_PO_STATS(net, IPSTATS_MIB_OUT, skb->len);
-
      skb->dev = dev;
      skb->protocol = htons(ETH_P_IP);

@@ -422,8 +423,6 @@ int ip_output(struct net *net, struct sock *sk, 
struct sk_buff *skb)
  {
      struct net_device *dev = skb_dst(skb)->dev, *indev = skb->dev;

-    IP_UPD_PO_STATS(net, IPSTATS_MIB_OUT, skb->len);
-
      skb->dev = dev;
      skb->protocol = htons(ETH_P_IP);

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index aea29d97f8df..cdd626539f41 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -1780,7 +1780,6 @@ static inline int ipmr_forward_finish(struct net 
*net, struct sock *sk,
      struct ip_options *opt = &(IPCB(skb)->opt);

      IP_INC_STATS(net, IPSTATS_MIB_OUTFORWDATAGRAMS);
-    IP_ADD_STATS(net, IPSTATS_MIB_OUTOCTETS, skb->len);

      if (unlikely(opt->optlen))
          ip_forward_options(skb);
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index be63929b1ac5..5ad8adeba151 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -431,7 +431,6 @@ static inline int ip6_forward_finish(struct net 
*net, struct sock *sk,
      struct dst_entry *dst = skb_dst(skb);

      __IP6_INC_STATS(net, ip6_dst_idev(dst), IPSTATS_MIB_OUTFORWDATAGRAMS);
-    __IP6_ADD_STATS(net, ip6_dst_idev(dst), IPSTATS_MIB_OUTOCTETS, 
skb->len);

  #ifdef CONFIG_NET_SWITCHDEV
      if (skb->offload_l3_fwd_mark) {
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 91f1c5f56d5f..c2c3180bde6a 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -1991,8 +1991,6 @@ static inline int ip6mr_forward2_finish(struct net 
*net, struct sock *sk, struct
  {
      IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)),
                IPSTATS_MIB_OUTFORWDATAGRAMS);
-    IP6_ADD_STATS(net, ip6_dst_idev(skb_dst(skb)),
-              IPSTATS_MIB_OUTOCTETS, skb->len);
      return dst_output(net, sk, skb);
  }

-- 
2.25.1

[-- Attachment #1.2: Type: text/html, Size: 12564 bytes --]

[-- Attachment #2: Case-diagram.png --]
[-- Type: image/png, Size: 77701 bytes --]

[-- Attachment #3: network-structure.png --]
[-- Type: image/png, Size: 49391 bytes --]

Re: [NETWORKING] IPSTATS_MIB_OUTOCTETS increment is duplicated in SNMP

[Jakub Kicinski]

On Thu, 17 Aug 2023 11:32:43 +0800 heng guo wrote:
> Linux version: 5.10.154
> Platform: ARM nxp-ls1028

Linux development doesn't happen in stable trees.
Can you reproduce this problem on Linus's tree?
If so please rebase the patch and resend it 
(after reading how to submit patches to Linux).