[ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

[ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

[Gerrit Renker]

This problem involves MIB counter inaccuracies triggered by 
failed UDP checksums.

Problem: ip_local_deliver_finish calls udp_rcv, which calls
         udp_queue_rcv_skb. 
         Unless the sk_filter is set, the checksum of the incoming
         UDP datagram is not verified. If there are no other problems
         InDatagrams (UDP_MIB_INDATAGRAMS) is then incremented.
         Now, if udp_recvmsg is called as a handler for incoming
         UDP datagrams, the checksum is verified for the first time
         (unless sk_filter was set) and if the checksum fails, the
         `goto csum_copy_err' leads to forcibly removing the datagram
         _and_ incrementing InErrors (UDP_MIB_INERRORS). 

Issue:   When problem occurs in the manner described, the datagram
         is counted twice: once as InDatagram and once as InErrors.
         RFC 2013 defines InDatagrams as counter for delivered datagrams;
         these datagrams are counted but never delivered.

How to reproduce: Send UDP datagrams with checksums enabled, use middlebox
         which corrupts part of the traffic (e.g. bit errors / NetEm) and
         use /proc/net/snmp to watch the counters. The sum of InErrors,
         NoPorts and InDatagrams exceeds the real number of sent datagrams 
         by the number of datagrams which were counted twice and forcibly
         removed by udp_recvmsg.
Non-occurrence: The problem does not occur if the sender disabled 
         UDP checksums (zero field; allowed for IPv4, but not for IPv6), 
         since then the checksum code returns success.

Fix:   Move the `UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS)' statement from 
       udp_queue_rcv_skb to udp_recvmsg. Now InDatagrams only counts those
       datagrams which were really delivered (as per RFC 2013). 

Please CC: any correspondence to gerrit@erg.abdn.ac.uk  

Signed-off-by: <gerrit@erg.abdn.ac.uk>

---

diff -Nurp  a/net/ipv4/udp.c b/net/ipv4/udp.c
--- a/net/ipv4/udp.c	2006-06-06 17:01:26.000000000 +0100
+++ b/net/ipv4/udp.c	2006-06-06 23:39:45.000000000 +0100
@@ -823,6 +823,7 @@ try_again:
 		goto out_free;
 
 	sock_recv_timestamp(msg, sk, skb);
+	UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS);
 
 	/* Copy the address. */
 	if (sin)
@@ -1032,7 +1033,6 @@ static int udp_queue_rcv_skb(struct sock
 		kfree_skb(skb);
 		return -1;
 	}
-	UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS);
 	return 0;
 }
 

Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

[David Miller]

From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Tue, 6 Jun 2006 19:25:40 +0100

> Fix:   Move the `UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS)' statement from 
>        udp_queue_rcv_skb to udp_recvmsg. Now InDatagrams only counts those
>        datagrams which were really delivered (as per RFC 2013). 
> 
> Please CC: any correspondence to gerrit@erg.abdn.ac.uk  
> 
> Signed-off-by: <gerrit@erg.abdn.ac.uk>

Unfortunately this breaks NFS and other in-kernel UDP socket usages,
which never call recvmsg() and instead take the packet via the
->data_ready() callback done by sock_queue_receive_skb().

Your patch will make the counter never get incremented when such
a user is using the UDP socket.

Probably a better way to handle this is to correct the
INDATAGRAMS value by decrementing it when we notice that
the checksum is incorrect in a deferred manner.

Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

[Gerrit Renker]

Quoting David Miller:
|  
|  > Fix:   Move the `UDP_INC_STATS_BH(UDP_MIB_INDATAGRAMS)' statement from 
|  >        udp_queue_rcv_skb to udp_recvmsg. Now InDatagrams only counts those
|  >        datagrams which were really delivered (as per RFC 2013). 
|  > 
|  
|  Unfortunately this breaks NFS and other in-kernel UDP socket usages,
|  which never call recvmsg() and instead take the packet via the
|  ->data_ready() callback done by sock_queue_receive_skb().
|  
|  Your patch will make the counter never get incremented when such
|  a user is using the UDP socket.
|  
|  Probably a better way to handle this is to correct the
|  INDATAGRAMS value by decrementing it when we notice that
|  the checksum is incorrect in a deferred manner.
This is clearly preferable - would it look like this:

csum_copy_err:
     UDP_INC_STATS_BH(UDP_MIB_INERRORS);
     UDP_DEC_STATS_BH(UDP_MIB_INDATAGRAMS); /* requires new macro */

     skb_kill_datagram(sk, skb, flags);
     /* ... */

in udp_recvmsg? Here I must pass - there is no xxx_DEC_BH macro in 
include/net/snmp.h and I don't know whether the following guess is correct:

#define SNMP_DEC_STATS_BH(mib, field) 	\
	(per_cpu_ptr(mib[0], raw_smp_processor_id())->mibs[field]--)

If this is correct, then it seems done; one could use this macro or add
a corresponding UDP_DEC_STATS_BH to include/net/udp.h .

Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

[David Miller]

From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Mon, 12 Jun 2006 07:02:45 +0100

> This is clearly preferable - would it look like this:
> 
> csum_copy_err:
>      UDP_INC_STATS_BH(UDP_MIB_INERRORS);
>      UDP_DEC_STATS_BH(UDP_MIB_INDATAGRAMS); /* requires new macro */
> 
>      skb_kill_datagram(sk, skb, flags);
>      /* ... */
> 
> in udp_recvmsg? Here I must pass - there is no xxx_DEC_BH macro in 
> include/net/snmp.h and I don't know whether the following guess is correct:
> 
> #define SNMP_DEC_STATS_BH(mib, field) 	\
> 	(per_cpu_ptr(mib[0], raw_smp_processor_id())->mibs[field]--)
> 
> If this is correct, then it seems done; one could use this macro or add
> a corresponding UDP_DEC_STATS_BH to include/net/udp.h .

The index of mib[] in those macros is always "!in_softirq()", the
*_BH() variants use zero for the index because they are called in
contexts where we know that "!in_sortirq()" evaluates to false.

So your SNMP_DEC_STATS_BH() macro is correct.

Can you cook up the patch, which adds your SNMP_DEC_STATS_BH() macro,
the UDP_DEC_STATS_BH counterpart, and the change that uses it in
net/ipv4/udp.c?

I'd appreciate this, thanks a lot.

Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

[Gerrit Renker]

The code below implements the discussed solution of decrementing
InDatagrams if a datagram fails the checksum within udp_recvmsg().

I have given it a quick test / build and checked the outcome against
previous results: I now obtained correct counter values, i.e. the application
counted exactly InDatagrams datagrams, whereas with the same settings before
this was not the case (datagrams with checksum errors were counted both as
InErrors and as InDatagrams). 

Will add this patch to http://bugzilla.kernel.org/show_bug.cgi?id=6660
where this problem is also described. Patches under 2.6.16 with no complaints.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---

 include/net/snmp.h |    2 ++
 include/net/udp.h  |    1 +
 net/ipv4/udp.c     |    1 +
 3 files changed, 4 insertions(+)


diff -Nurp  a/include/net/snmp.h b/include/net/snmp.h
--- a/include/net/snmp.h	2006-06-05 21:52:55.000000000 +0100
+++ b/include/net/snmp.h	2006-06-12 07:38:11.000000000 +0100
@@ -137,6 +137,8 @@ struct linux_mib {
 	(per_cpu_ptr(mib[!in_softirq()], raw_smp_processor_id())->mibs[field]++)
 #define SNMP_DEC_STATS(mib, field) 	\
 	(per_cpu_ptr(mib[!in_softirq()], raw_smp_processor_id())->mibs[field]--)
+#define SNMP_DEC_STATS_BH(mib, field) 	\
+	(per_cpu_ptr(mib[0], raw_smp_processor_id())->mibs[field]--)
 #define SNMP_ADD_STATS_BH(mib, field, addend) 	\
 	(per_cpu_ptr(mib[0], raw_smp_processor_id())->mibs[field] += addend)
 #define SNMP_ADD_STATS_USER(mib, field, addend) 	\
diff -Nurp  a/include/net/udp.h b/include/net/udp.h
--- a/include/net/udp.h	2006-06-06 18:04:36.000000000 +0100
+++ b/include/net/udp.h	2006-06-12 07:39:29.000000000 +0100
@@ -78,6 +78,7 @@ DECLARE_SNMP_STAT(struct udp_mib, udp_st
 #define UDP_INC_STATS(field)		SNMP_INC_STATS(udp_statistics, field)
 #define UDP_INC_STATS_BH(field)		SNMP_INC_STATS_BH(udp_statistics, field)
 #define UDP_INC_STATS_USER(field) 	SNMP_INC_STATS_USER(udp_statistics, field)
+#define UDP_DEC_STATS_BH(field)		SNMP_DEC_STATS_BH(udp_statistics, field)
 
 /* /proc */
 struct udp_seq_afinfo {
diff -Nurp  a/net/ipv4/udp.c b/net/ipv4/udp.c
--- a/net/ipv4/udp.c	2006-06-07 20:44:13.000000000 +0100
+++ b/net/ipv4/udp.c	2006-06-12 07:40:02.000000000 +0100
@@ -846,6 +846,7 @@ out:
 
 csum_copy_err:
 	UDP_INC_STATS_BH(UDP_MIB_INERRORS);
+	UDP_DEC_STATS_BH(UDP_MIB_INDATAGRAMS);
 
 	skb_kill_datagram(sk, skb, flags);
 

Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

[Herbert Xu]

David Miller <davem@davemloft.net> wrote:
> 
> Probably a better way to handle this is to correct the
> INDATAGRAMS value by decrementing it when we notice that
> the checksum is incorrect in a deferred manner.

I think sunrpc should instead increment the appropriate counters directly
as otherwise checksum errors won't be recorded correctly for sunrpc packets.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 12 Jun 2006 16:18:09 +1000

> David Miller <davem@davemloft.net> wrote:
> > 
> > Probably a better way to handle this is to correct the
> > INDATAGRAMS value by decrementing it when we notice that
> > the checksum is incorrect in a deferred manner.
> 
> I think sunrpc should instead increment the appropriate counters directly
> as otherwise checksum errors won't be recorded correctly for sunrpc packets.

Yeah.  Good point.  But how much protocol internals do we want to
slide into the ->data_ready() callbacks of such layers?  That's ugly
and something we should try to avoid.

Re: [ PATCH 2.6.17-rc6 1/1] udp.c: counting InDatagrams which are never delivered

[Herbert Xu]

On Sun, Jun 11, 2006 at 11:49:05PM -0700, David Miller wrote:
> 
> Yeah.  Good point.  But how much protocol internals do we want to
> slide into the ->data_ready() callbacks of such layers?  That's ugly
> and something we should try to avoid.

I agree with the objective of minimising the exposure of internals.
However, in this particular instance we're already exposing much more
than a couple of UDP SNMP counters in the sunrpc code.  This makes
adjusting the counters there the most expedient course of action.

Longer term we probably want to restructure the code a bit so that
more if it moves to udp.c.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt