netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/1] Issue description and debug
@ 2023-09-14  5:16 Heng Guo
  2023-09-14  5:16 ` [PATCH 1/1] net: ipv4,ipv6: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check Heng Guo
  0 siblings, 1 reply; 10+ messages in thread
From: Heng Guo @ 2023-09-14  5:16 UTC (permalink / raw)
  To: davem, sahern, edumazet, kuba, pabeni; +Cc: netdev, filip.pudak, heng.guo

Hi maintainers,

The IPSTATS_MIB_OUTFORWDATAGRAMS is counted after fragment check.

Reproduce environment:
network with 3 VM linuxs is connected as below:
VM1<---->VM2(latest kernel 6.5.0-rc7)<---->VM3
VM1: eth0 ip: 192.168.122.207 MTU 1800
VM2: eth0 ip: 192.168.122.208, eth1 ip: 192.168.123.224 MTU 1500
VM3: eth0 ip: 192.168.123.240 MTU 1800

Reproduce:
VM1 send 1600 bytes UDP data to VM3 using tools scapy with flags='DF'.
scapy command:
send(IP(dst="192.168.123.240",flags='DF')/UDP()/str('0'*1600),count=1,
inter=1.000000)

Result:
Before IP data is sent.
----------------------------------------------------------------------
root@qemux86-64:~# cat /proc/net/snmp
Ip: Forwarding DefaultTTL InReceives InHdrErrors InAddrErrors
ForwDatagrams InUnknownProtos InDiscards InDelivers OutRequests
 OutDiscards OutNoRoutes ReasmTimeout ReasmReqdss
Ip: 1 64 6 0 2 2 0 0 2 4 0 0 0 0 0 0 0 0 0
......
root@qemux86-64:~#
----------------------------------------------------------------------
After IP data is sent.
----------------------------------------------------------------------
root@qemux86-64:~# cat /proc/net/snmp
Ip: Forwarding DefaultTTL InReceives InHdrErrors InAddrErrors
 ForwDatagrams InUnknownProtos InDiscards InDelivers OutRequests
 OutDiscards OutNoRoutes ReasmTimeout ReasmReqdss
Ip: 1 64 7 0 2 2 0 0 2 5 0 0 0 0 0 0 0 1 0
......
root@qemux86-64:~#
----------------------------------------------------------------------
ForwDatagrams is always keeping 2 without increment.

Issue description and patch:
ip_exceeds_mtu() in ip_forward() drops this IP datagram because skb len
(1600 sending by scapy) is over MTU(1500 in VM2) and "DF" is set.
According to RFC 4293 "3.2.3. IP Statistics Tables",
  +-------+------>------+----->-----+----->-----+
  | InForwDatagrams (6) | OutForwDatagrams (6)  |
  |                     V                       +->-+ OutFragReqds
  |                 InNoRoutes                  |   | (packets)
  / (local packet (3)                           |   |
  |  IF is that of the address                  |   +--> OutFragFails
  |  and may not be the receiving IF)           |   |    (packets)
the IPSTATS_MIB_OUTFORWDATAGRAMS should be counted before fragment
check.
The existing implementation, instead, would incease the counter after
fragment check: ip_exceeds_mtu() in ipv4 and ip6_pkt_too_big() in ipv6.
So do patch to move IPSTATS_MIB_OUTFORWDATAGRAMS counter to ip_forward()
for ipv4 and ip6_forward() for ipv6.

Test result with patch:
Before IP data is sent.
----------------------------------------------------------------------
root@qemux86-64:~# cat /proc/net/snmp
Ip: Forwarding DefaultTTL InReceives InHdrErrors InAddrErrors
 ForwDatagrams InUnknownProtos InDiscards InDelivers OutRequests
 OutDiscards OutNoRoutes ReasmTimeout ReasmReqdss
Ip: 1 64 6 0 2 2 0 0 2 4 0 0 0 0 0 0 0 0 0
......
root@qemux86-64:~#
----------------------------------------------------------------------
After IP data is sent.
----------------------------------------------------------------------
root@qemux86-64:~# cat /proc/net/snmp
Ip: Forwarding DefaultTTL InReceives InHdrErrors InAddrErrors
 ForwDatagrams InUnknownProtos InDiscards InDelivers OutRequests
 OutDiscards OutNoRoutes ReasmTimeout ReasmReqdss
Ip: 1 64 7 0 2 3 0 0 2 5 0 0 0 0 0 0 0 1 0
......
root@qemux86-64:~#
----------------------------------------------------------------------
ForwDatagrams is updated from 2 to 3.

Thanks,
Heng


Heng Guo (1):
  net: ipv4,ipv6: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after
    fragment check

 net/ipv4/ip_forward.c | 4 ++--
 net/ipv6/ip6_output.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH 1/1] net: ipv4,ipv6: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check
  2023-09-14  5:16 [PATCH 0/1] Issue description and debug Heng Guo
@ 2023-09-14  5:16 ` Heng Guo
  2023-09-14  9:34   ` kernel test robot
                     ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Heng Guo @ 2023-09-14  5:16 UTC (permalink / raw)
  To: davem, sahern, edumazet, kuba, pabeni; +Cc: netdev, filip.pudak, heng.guo

According to RFC 4293 "3.2.3. IP Statistics Tables",
  +-------+------>------+----->-----+----->-----+
  | InForwDatagrams (6) | OutForwDatagrams (6)  |
  |                     V                       +->-+ OutFragReqds
  |                 InNoRoutes                  |   | (packets)
  / (local packet (3)                           |   |
  |  IF is that of the address                  |   +--> OutFragFails
  |  and may not be the receiving IF)           |   |    (packets)
the IPSTATS_MIB_OUTFORWDATAGRAMS should be counted before fragment
check.

The existing implementation, instead, would incease the counter after
fragment check: ip_exceeds_mtu() in ipv4 and ip6_pkt_too_big() in ipv6.

So move IPSTATS_MIB_OUTFORWDATAGRAMS counter to ip_forward() for ipv4 and
ip6_forward() for ipv6.

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

diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
index 66fac1216d46..acba24fc000f 100644
--- a/net/ipv4/ip_forward.c
+++ b/net/ipv4/ip_forward.c
@@ -66,8 +66,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);
-
 #ifdef CONFIG_NET_SWITCHDEV
 	if (skb->offload_l3_fwd_mark) {
 		consume_skb(skb);
@@ -130,6 +128,8 @@ int ip_forward(struct sk_buff *skb)
 	if (opt->is_strictroute && rt->rt_uses_gateway)
 		goto sr_failed;
 
+        __IP_INC_STATS(net, IPSTATS_MIB_OUTFORWDATAGRAMS);
+
 	IPCB(skb)->flags |= IPSKB_FORWARDED;
 	mtu = ip_dst_mtu_maybe_forward(&rt->dst, true);
 	if (ip_exceeds_mtu(skb, mtu)) {
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 275b24c89ac3..8943c85d75b4 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -450,8 +450,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);
-
 #ifdef CONFIG_NET_SWITCHDEV
 	if (skb->offload_l3_fwd_mark) {
 		consume_skb(skb);
@@ -619,6 +617,8 @@ int ip6_forward(struct sk_buff *skb)
 		}
 	}
 
+	__IP6_INC_STATS(net, ip6_dst_idev(dst), IPSTATS_MIB_OUTFORWDATAGRAMS);
+
 	mtu = ip6_dst_mtu_maybe_forward(dst, true);
 	if (mtu < IPV6_MIN_MTU)
 		mtu = IPV6_MIN_MTU;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] net: ipv4,ipv6: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check
  2023-09-14  5:16 ` [PATCH 1/1] net: ipv4,ipv6: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check Heng Guo
@ 2023-09-14  9:34   ` kernel test robot
  2023-09-19  9:38   ` [PATCH v2 0/1] Issue description and debug Heng Guo
  2023-09-21  9:23   ` [PATCH v3 0/1] Issue description and debug Heng Guo
  2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2023-09-14  9:34 UTC (permalink / raw)
  To: Heng Guo, davem, sahern, edumazet, kuba, pabeni
  Cc: oe-kbuild-all, netdev, filip.pudak, heng.guo

Hi Heng,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]
[also build test WARNING on net/main linus/master v6.6-rc1 next-20230914]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Heng-Guo/net-ipv4-ipv6-fix-IPSTATS_MIB_OUTFORWDATAGRAMS-increment-after-fragment-check/20230914-131757
base:   net-next/main
patch link:    https://lore.kernel.org/r/20230914051623.2180843-2-heng.guo%40windriver.com
patch subject: [PATCH 1/1] net: ipv4,ipv6: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230914/202309141701.7OFxGdE5-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230914/202309141701.7OFxGdE5-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309141701.7OFxGdE5-lkp@intel.com/

All warnings (new ones prefixed by >>):

   net/ipv6/ip6_output.c: In function 'ip6_forward_finish':
>> net/ipv6/ip6_output.c:451:27: warning: unused variable 'dst' [-Wunused-variable]
     451 |         struct dst_entry *dst = skb_dst(skb);
         |                           ^~~


vim +/dst +451 net/ipv6/ip6_output.c

e21e0b5f19ac78 Ville Nuorvala    2006-09-22  447  
0c4b51f0054ce8 Eric W. Biederman 2015-09-15  448  static inline int ip6_forward_finish(struct net *net, struct sock *sk,
0c4b51f0054ce8 Eric W. Biederman 2015-09-15  449  				     struct sk_buff *skb)
^1da177e4c3f41 Linus Torvalds    2005-04-16  450  {
71a1c915238c97 Jeff Barnhill     2018-04-05 @451  	struct dst_entry *dst = skb_dst(skb);
71a1c915238c97 Jeff Barnhill     2018-04-05  452  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH v2 0/1] Issue description and debug
  2023-09-14  5:16 ` [PATCH 1/1] net: ipv4,ipv6: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check Heng Guo
  2023-09-14  9:34   ` kernel test robot
@ 2023-09-19  9:38   ` Heng Guo
  2023-09-19  9:38     ` [PATCH v2] net: ipv4,ipv6: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check Heng Guo
  2023-09-21  9:23   ` [PATCH v3 0/1] Issue description and debug Heng Guo
  2 siblings, 1 reply; 10+ messages in thread
From: Heng Guo @ 2023-09-19  9:38 UTC (permalink / raw)
  To: davem, sahern, edumazet, kuba, pabeni; +Cc: netdev, filip.pudak, heng.guo

Hi maintainers,

The IPSTATS_MIB_OUTFORWDATAGRAMS is counted after fragment check.

Reproduce environment:
network with 3 VM linuxs is connected as below:
VM1<---->VM2(latest kernel 6.5.0-rc7)<---->VM3
VM1: eth0 ip: 192.168.122.207 MTU 1800
VM2: eth0 ip: 192.168.122.208, eth1 ip: 192.168.123.224 MTU 1500
VM3: eth0 ip: 192.168.123.240 MTU 1800

Reproduce:
VM1 send 1600 bytes UDP data to VM3 using tools scapy with flags='DF'.
scapy command:
send(IP(dst="192.168.123.240",flags='DF')/UDP()/str('0'*1600),count=1,
inter=1.000000)

Result:
Before IP data is sent.
----------------------------------------------------------------------
root@qemux86-64:~# cat /proc/net/snmp
Ip: Forwarding DefaultTTL InReceives InHdrErrors InAddrErrors
ForwDatagrams InUnknownProtos InDiscards InDelivers OutRequests
 OutDiscards OutNoRoutes ReasmTimeout ReasmReqdss
Ip: 1 64 6 0 2 2 0 0 2 4 0 0 0 0 0 0 0 0 0
......
root@qemux86-64:~#
----------------------------------------------------------------------
After IP data is sent.
----------------------------------------------------------------------
root@qemux86-64:~# cat /proc/net/snmp
Ip: Forwarding DefaultTTL InReceives InHdrErrors InAddrErrors
 ForwDatagrams InUnknownProtos InDiscards InDelivers OutRequests
 OutDiscards OutNoRoutes ReasmTimeout ReasmReqdss
Ip: 1 64 7 0 2 2 0 0 2 5 0 0 0 0 0 0 0 1 0
......
root@qemux86-64:~#
----------------------------------------------------------------------
ForwDatagrams is always keeping 2 without increment.

Issue description and patch:
ip_exceeds_mtu() in ip_forward() drops this IP datagram because skb len
(1600 sending by scapy) is over MTU(1500 in VM2) and "DF" is set.
According to RFC 4293 "3.2.3. IP Statistics Tables",
  +-------+------>------+----->-----+----->-----+
  | InForwDatagrams (6) | OutForwDatagrams (6)  |
  |                     V                       +->-+ OutFragReqds
  |                 InNoRoutes                  |   | (packets)
  / (local packet (3)                           |   |
  |  IF is that of the address                  |   +--> OutFragFails
  |  and may not be the receiving IF)           |   |    (packets)
the IPSTATS_MIB_OUTFORWDATAGRAMS should be counted before fragment
check.
The existing implementation, instead, would incease the counter after
fragment check: ip_exceeds_mtu() in ipv4 and ip6_pkt_too_big() in ipv6.
So do patch to move IPSTATS_MIB_OUTFORWDATAGRAMS counter to ip_forward()
for ipv4 and ip6_forward() for ipv6.

Test result with patch:
Before IP data is sent.
----------------------------------------------------------------------
root@qemux86-64:~# cat /proc/net/snmp
Ip: Forwarding DefaultTTL InReceives InHdrErrors InAddrErrors
 ForwDatagrams InUnknownProtos InDiscards InDelivers OutRequests
 OutDiscards OutNoRoutes ReasmTimeout ReasmReqdss
Ip: 1 64 6 0 2 2 0 0 2 4 0 0 0 0 0 0 0 0 0
......
root@qemux86-64:~#
----------------------------------------------------------------------
After IP data is sent.
----------------------------------------------------------------------
root@qemux86-64:~# cat /proc/net/snmp
Ip: Forwarding DefaultTTL InReceives InHdrErrors InAddrErrors
 ForwDatagrams InUnknownProtos InDiscards InDelivers OutRequests
 OutDiscards OutNoRoutes ReasmTimeout ReasmReqdss
Ip: 1 64 7 0 2 3 0 0 2 5 0 0 0 0 0 0 0 1 0
......
root@qemux86-64:~#
----------------------------------------------------------------------
ForwDatagrams is updated from 2 to 3.

Thanks,
Heng



v1->v2: fix build warning
Reported-by: kernel test robot <lkp@intel.com>
Closes:
https://lore.kernel.org/oe-kbuild-all/202309141701.7OFxGdE5-lkp@intel.com/

Heng Guo (1):
  net: ipv4,ipv6: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after
    fragment check

 net/ipv4/ip_forward.c | 4 ++--
 net/ipv6/ip6_output.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH v2] net: ipv4,ipv6: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check
  2023-09-19  9:38   ` [PATCH v2 0/1] Issue description and debug Heng Guo
@ 2023-09-19  9:38     ` Heng Guo
  2023-09-21  7:46       ` Paolo Abeni
  0 siblings, 1 reply; 10+ messages in thread
From: Heng Guo @ 2023-09-19  9:38 UTC (permalink / raw)
  To: davem, sahern, edumazet, kuba, pabeni; +Cc: netdev, filip.pudak, heng.guo

According to RFC 4293 "3.2.3. IP Statistics Tables",
  +-------+------>------+----->-----+----->-----+
  | InForwDatagrams (6) | OutForwDatagrams (6)  |
  |                     V                       +->-+ OutFragReqds
  |                 InNoRoutes                  |   | (packets)
  / (local packet (3)                           |   |
  |  IF is that of the address                  |   +--> OutFragFails
  |  and may not be the receiving IF)           |   |    (packets)
the IPSTATS_MIB_OUTFORWDATAGRAMS should be counted before fragment
check.

The existing implementation, instead, would incease the counter after
fragment check: ip_exceeds_mtu() in ipv4 and ip6_pkt_too_big() in ipv6.

So move IPSTATS_MIB_OUTFORWDATAGRAMS counter to ip_forward() for ipv4 and
ip6_forward() for ipv6.

Reviewed-by: Filip Pudak <filip.pudak@windriver.com>
Signed-off-by: Heng Guo <heng.guo@windriver.com>
---
 net/ipv4/ip_forward.c | 4 ++--
 net/ipv6/ip6_output.c | 6 ++----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
index 66fac1216d46..acba24fc000f 100644
--- a/net/ipv4/ip_forward.c
+++ b/net/ipv4/ip_forward.c
@@ -66,8 +66,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);
-
 #ifdef CONFIG_NET_SWITCHDEV
 	if (skb->offload_l3_fwd_mark) {
 		consume_skb(skb);
@@ -130,6 +128,8 @@ int ip_forward(struct sk_buff *skb)
 	if (opt->is_strictroute && rt->rt_uses_gateway)
 		goto sr_failed;
 
+        __IP_INC_STATS(net, IPSTATS_MIB_OUTFORWDATAGRAMS);
+
 	IPCB(skb)->flags |= IPSKB_FORWARDED;
 	mtu = ip_dst_mtu_maybe_forward(&rt->dst, true);
 	if (ip_exceeds_mtu(skb, mtu)) {
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 54fc4c711f2c..8a9199ab97ef 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -448,10 +448,6 @@ static int ip6_forward_proxy_check(struct sk_buff *skb)
 static inline int ip6_forward_finish(struct net *net, struct sock *sk,
 				     struct sk_buff *skb)
 {
-	struct dst_entry *dst = skb_dst(skb);
-
-	__IP6_INC_STATS(net, ip6_dst_idev(dst), IPSTATS_MIB_OUTFORWDATAGRAMS);
-
 #ifdef CONFIG_NET_SWITCHDEV
 	if (skb->offload_l3_fwd_mark) {
 		consume_skb(skb);
@@ -619,6 +615,8 @@ int ip6_forward(struct sk_buff *skb)
 		}
 	}
 
+	__IP6_INC_STATS(net, ip6_dst_idev(dst), IPSTATS_MIB_OUTFORWDATAGRAMS);
+
 	mtu = ip6_dst_mtu_maybe_forward(dst, true);
 	if (mtu < IPV6_MIN_MTU)
 		mtu = IPV6_MIN_MTU;
-- 
2.35.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH v2] net: ipv4,ipv6: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check
  2023-09-19  9:38     ` [PATCH v2] net: ipv4,ipv6: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check Heng Guo
@ 2023-09-21  7:46       ` Paolo Abeni
  0 siblings, 0 replies; 10+ messages in thread
From: Paolo Abeni @ 2023-09-21  7:46 UTC (permalink / raw)
  To: Heng Guo, davem, sahern, edumazet, kuba; +Cc: netdev, filip.pudak

On Tue, 2023-09-19 at 17:38 +0800, Heng Guo wrote:
> diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
> index 66fac1216d46..acba24fc000f 100644
> --- a/net/ipv4/ip_forward.c
> +++ b/net/ipv4/ip_forward.c
> @@ -66,8 +66,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);
> -
>  #ifdef CONFIG_NET_SWITCHDEV
>  	if (skb->offload_l3_fwd_mark) {
>  		consume_skb(skb);
> @@ -130,6 +128,8 @@ int ip_forward(struct sk_buff *skb)
>  	if (opt->is_strictroute && rt->rt_uses_gateway)
>  		goto sr_failed;
>  
> +        __IP_INC_STATS(net, IPSTATS_MIB_OUTFORWDATAGRAMS);

The above is white-space damaged - uses spaces instead of tab.

Please fix it.

Also IMHO this is net-next material, please specify accordingly the
target tree into the subj line.

Thanks,

Paolo


^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH v3 0/1] Issue description and debug
  2023-09-14  5:16 ` [PATCH 1/1] net: ipv4,ipv6: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check Heng Guo
  2023-09-14  9:34   ` kernel test robot
  2023-09-19  9:38   ` [PATCH v2 0/1] Issue description and debug Heng Guo
@ 2023-09-21  9:23   ` Heng Guo
  2023-09-21  9:23     ` [PATCH v3 1/1] net-next: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check Heng Guo
  2 siblings, 1 reply; 10+ messages in thread
From: Heng Guo @ 2023-09-21  9:23 UTC (permalink / raw)
  To: davem, sahern, edumazet, kuba, pabeni; +Cc: netdev, filip.pudak, heng.guo

Hi maintainers,

The IPSTATS_MIB_OUTFORWDATAGRAMS is counted after fragment check.

Reproduce environment:
network with 3 VM linuxs is connected as below:
VM1<---->VM2(latest kernel 6.5.0-rc7)<---->VM3
VM1: eth0 ip: 192.168.122.207 MTU 1800
VM2: eth0 ip: 192.168.122.208, eth1 ip: 192.168.123.224 MTU 1500
VM3: eth0 ip: 192.168.123.240 MTU 1800

Reproduce:
VM1 send 1600 bytes UDP data to VM3 using tools scapy with flags='DF'.
scapy command:
send(IP(dst="192.168.123.240",flags='DF')/UDP()/str('0'*1600),count=1,
inter=1.000000)

Result:
Before IP data is sent.
----------------------------------------------------------------------
root@qemux86-64:~# cat /proc/net/snmp
Ip: Forwarding DefaultTTL InReceives InHdrErrors InAddrErrors
ForwDatagrams InUnknownProtos InDiscards InDelivers OutRequests
OutDiscards OutNoRoutes ReasmTimeout ReasmReqdss
Ip: 1 64 6 0 2 2 0 0 2 4 0 0 0 0 0 0 0 0 0
......
root@qemux86-64:~#
----------------------------------------------------------------------
After IP data is sent.
----------------------------------------------------------------------
root@qemux86-64:~# cat /proc/net/snmp
Ip: Forwarding DefaultTTL InReceives InHdrErrors InAddrErrors
ForwDatagrams InUnknownProtos InDiscards InDelivers OutRequests
OutDiscards OutNoRoutes ReasmTimeout ReasmReqdss
Ip: 1 64 7 0 2 2 0 0 2 5 0 0 0 0 0 0 0 1 0
......
root@qemux86-64:~#
----------------------------------------------------------------------
ForwDatagrams is always keeping 2 without increment.

Issue description and patch:
ip_exceeds_mtu() in ip_forward() drops this IP datagram because skb len
(1600 sending by scapy) is over MTU(1500 in VM2) and "DF" is set.
According to RFC 4293 "3.2.3. IP Statistics Tables",
  +-------+------>------+----->-----+----->-----+
  | InForwDatagrams (6) | OutForwDatagrams (6)  |
  |                     V                       +->-+ OutFragReqds
  |                 InNoRoutes                  |   | (packets)
  / (local packet (3)                           |   |
  |  IF is that of the address                  |   +--> OutFragFails
  |  and may not be the receiving IF)           |   |    (packets)
the IPSTATS_MIB_OUTFORWDATAGRAMS should be counted before fragment
check.
The existing implementation, instead, would incease the counter after
fragment check: ip_exceeds_mtu() in ipv4 and ip6_pkt_too_big() in ipv6.
So do patch to move IPSTATS_MIB_OUTFORWDATAGRAMS counter to ip_forward()
for ipv4 and ip6_forward() for ipv6.

Test result with patch:
Before IP data is sent.
----------------------------------------------------------------------
root@qemux86-64:~# cat /proc/net/snmp
Ip: Forwarding DefaultTTL InReceives InHdrErrors InAddrErrors
 ForwDatagrams InUnknownProtos InDiscards InDelivers OutRequests
 OutDiscards OutNoRoutes ReasmTimeout ReasmReqdss
Ip: 1 64 6 0 2 2 0 0 2 4 0 0 0 0 0 0 0 0 0
......
root@qemux86-64:~#
----------------------------------------------------------------------
After IP data is sent.
----------------------------------------------------------------------
root@qemux86-64:~# cat /proc/net/snmp
Ip: Forwarding DefaultTTL InReceives InHdrErrors InAddrErrors
 ForwDatagrams InUnknownProtos InDiscards InDelivers OutRequests
 OutDiscards OutNoRoutes ReasmTimeout ReasmReqdss
Ip: 1 64 7 0 2 3 0 0 2 5 0 0 0 0 0 0 0 1 0
......
root@qemux86-64:~#
----------------------------------------------------------------------
ForwDatagrams is updated from 2 to 3.

Thanks,
Heng



v1->v2: fix build warning
Reported-by: kernel test robot <lkp@intel.com>
Closes:
https://lore.kernel.org/oe-kbuild-all/202309141701.7OFxGdE5-lkp@intel.com/

v2->v3:
1) Fix white-space damaged - uses spaces instead of tab.
2) Change subj to net-next.


Heng Guo (1):
  net-next: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment
    check

 net/ipv4/ip_forward.c | 4 ++--
 net/ipv6/ip6_output.c | 6 ++----
 2 files changed, 4 insertions(+), 6 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH v3 1/1] net-next: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check
  2023-09-21  9:23   ` [PATCH v3 0/1] Issue description and debug Heng Guo
@ 2023-09-21  9:23     ` Heng Guo
  2023-09-28  2:11       ` heng guo
  0 siblings, 1 reply; 10+ messages in thread
From: Heng Guo @ 2023-09-21  9:23 UTC (permalink / raw)
  To: davem, sahern, edumazet, kuba, pabeni; +Cc: netdev, filip.pudak, heng.guo

According to RFC 4293 "3.2.3. IP Statistics Tables",
  +-------+------>------+----->-----+----->-----+
  | InForwDatagrams (6) | OutForwDatagrams (6)  |
  |                     V                       +->-+ OutFragReqds
  |                 InNoRoutes                  |   | (packets)
  / (local packet (3)                           |   |
  |  IF is that of the address                  |   +--> OutFragFails
  |  and may not be the receiving IF)           |   |    (packets)
the IPSTATS_MIB_OUTFORWDATAGRAMS should be counted before fragment
check.

The existing implementation, instead, would incease the counter after
fragment check: ip_exceeds_mtu() in ipv4 and ip6_pkt_too_big() in ipv6.

So move IPSTATS_MIB_OUTFORWDATAGRAMS counter to ip_forward() for ipv4 and
ip6_forward() for ipv6.

Reviewed-by: Filip Pudak <filip.pudak@windriver.com>
Signed-off-by: Heng Guo <heng.guo@windriver.com>
---
 net/ipv4/ip_forward.c | 4 ++--
 net/ipv6/ip6_output.c | 6 ++----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
index 66fac1216d46..8b65f12583eb 100644
--- a/net/ipv4/ip_forward.c
+++ b/net/ipv4/ip_forward.c
@@ -66,8 +66,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);
-
 #ifdef CONFIG_NET_SWITCHDEV
 	if (skb->offload_l3_fwd_mark) {
 		consume_skb(skb);
@@ -130,6 +128,8 @@ int ip_forward(struct sk_buff *skb)
 	if (opt->is_strictroute && rt->rt_uses_gateway)
 		goto sr_failed;
 
+	__IP_INC_STATS(net, IPSTATS_MIB_OUTFORWDATAGRAMS);
+
 	IPCB(skb)->flags |= IPSKB_FORWARDED;
 	mtu = ip_dst_mtu_maybe_forward(&rt->dst, true);
 	if (ip_exceeds_mtu(skb, mtu)) {
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 54fc4c711f2c..8a9199ab97ef 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -448,10 +448,6 @@ static int ip6_forward_proxy_check(struct sk_buff *skb)
 static inline int ip6_forward_finish(struct net *net, struct sock *sk,
 				     struct sk_buff *skb)
 {
-	struct dst_entry *dst = skb_dst(skb);
-
-	__IP6_INC_STATS(net, ip6_dst_idev(dst), IPSTATS_MIB_OUTFORWDATAGRAMS);
-
 #ifdef CONFIG_NET_SWITCHDEV
 	if (skb->offload_l3_fwd_mark) {
 		consume_skb(skb);
@@ -619,6 +615,8 @@ int ip6_forward(struct sk_buff *skb)
 		}
 	}
 
+	__IP6_INC_STATS(net, ip6_dst_idev(dst), IPSTATS_MIB_OUTFORWDATAGRAMS);
+
 	mtu = ip6_dst_mtu_maybe_forward(dst, true);
 	if (mtu < IPV6_MIN_MTU)
 		mtu = IPV6_MIN_MTU;
-- 
2.35.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] net-next: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check
  2023-09-21  9:23     ` [PATCH v3 1/1] net-next: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check Heng Guo
@ 2023-09-28  2:11       ` heng guo
  2023-10-02 19:32         ` Jakub Kicinski
  0 siblings, 1 reply; 10+ messages in thread
From: heng guo @ 2023-09-28  2:11 UTC (permalink / raw)
  To: davem, sahern, edumazet, kuba, pabeni; +Cc: netdev, filip.pudak

Could you please take time to review this patch?


Thanks a lot,

Heng

On 9/21/23 17:23, Heng Guo wrote:
> According to RFC 4293 "3.2.3. IP Statistics Tables",
>    +-------+------>------+----->-----+----->-----+
>    | InForwDatagrams (6) | OutForwDatagrams (6)  |
>    |                     V                       +->-+ OutFragReqds
>    |                 InNoRoutes                  |   | (packets)
>    / (local packet (3)                           |   |
>    |  IF is that of the address                  |   +--> OutFragFails
>    |  and may not be the receiving IF)           |   |    (packets)
> the IPSTATS_MIB_OUTFORWDATAGRAMS should be counted before fragment
> check.
>
> The existing implementation, instead, would incease the counter after
> fragment check: ip_exceeds_mtu() in ipv4 and ip6_pkt_too_big() in ipv6.
>
> So move IPSTATS_MIB_OUTFORWDATAGRAMS counter to ip_forward() for ipv4 and
> ip6_forward() for ipv6.
>
> Reviewed-by: Filip Pudak <filip.pudak@windriver.com>
> Signed-off-by: Heng Guo <heng.guo@windriver.com>
> ---
>   net/ipv4/ip_forward.c | 4 ++--
>   net/ipv6/ip6_output.c | 6 ++----
>   2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
> index 66fac1216d46..8b65f12583eb 100644
> --- a/net/ipv4/ip_forward.c
> +++ b/net/ipv4/ip_forward.c
> @@ -66,8 +66,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);
> -
>   #ifdef CONFIG_NET_SWITCHDEV
>   	if (skb->offload_l3_fwd_mark) {
>   		consume_skb(skb);
> @@ -130,6 +128,8 @@ int ip_forward(struct sk_buff *skb)
>   	if (opt->is_strictroute && rt->rt_uses_gateway)
>   		goto sr_failed;
>   
> +	__IP_INC_STATS(net, IPSTATS_MIB_OUTFORWDATAGRAMS);
> +
>   	IPCB(skb)->flags |= IPSKB_FORWARDED;
>   	mtu = ip_dst_mtu_maybe_forward(&rt->dst, true);
>   	if (ip_exceeds_mtu(skb, mtu)) {
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 54fc4c711f2c..8a9199ab97ef 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -448,10 +448,6 @@ static int ip6_forward_proxy_check(struct sk_buff *skb)
>   static inline int ip6_forward_finish(struct net *net, struct sock *sk,
>   				     struct sk_buff *skb)
>   {
> -	struct dst_entry *dst = skb_dst(skb);
> -
> -	__IP6_INC_STATS(net, ip6_dst_idev(dst), IPSTATS_MIB_OUTFORWDATAGRAMS);
> -
>   #ifdef CONFIG_NET_SWITCHDEV
>   	if (skb->offload_l3_fwd_mark) {
>   		consume_skb(skb);
> @@ -619,6 +615,8 @@ int ip6_forward(struct sk_buff *skb)
>   		}
>   	}
>   
> +	__IP6_INC_STATS(net, ip6_dst_idev(dst), IPSTATS_MIB_OUTFORWDATAGRAMS);
> +
>   	mtu = ip6_dst_mtu_maybe_forward(dst, true);
>   	if (mtu < IPV6_MIN_MTU)
>   		mtu = IPV6_MIN_MTU;

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] net-next: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check
  2023-09-28  2:11       ` heng guo
@ 2023-10-02 19:32         ` Jakub Kicinski
  0 siblings, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2023-10-02 19:32 UTC (permalink / raw)
  To: heng.guo; +Cc: davem, edumazet, pabeni, netdev, filip.pudak

On Thu, 28 Sep 2023 10:11:59 +0800 heng guo wrote:
> Could you please take time to review this patch?

Please don't top post.

You need to CC the area maintainer (dsahern@kernel.org), 
looks like you mis-typed his address by skipping the leading "d".

Please repost with David CCed as a new thread, not in-reply-to.

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-10-02 19:32 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-14  5:16 [PATCH 0/1] Issue description and debug Heng Guo
2023-09-14  5:16 ` [PATCH 1/1] net: ipv4,ipv6: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check Heng Guo
2023-09-14  9:34   ` kernel test robot
2023-09-19  9:38   ` [PATCH v2 0/1] Issue description and debug Heng Guo
2023-09-19  9:38     ` [PATCH v2] net: ipv4,ipv6: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check Heng Guo
2023-09-21  7:46       ` Paolo Abeni
2023-09-21  9:23   ` [PATCH v3 0/1] Issue description and debug Heng Guo
2023-09-21  9:23     ` [PATCH v3 1/1] net-next: fix IPSTATS_MIB_OUTFORWDATAGRAMS increment after fragment check Heng Guo
2023-09-28  2:11       ` heng guo
2023-10-02 19:32         ` Jakub Kicinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).