SNMPv2 udpInDatagrams counter error

When I send a UDP datagrams with checksum error to target, I found that:
Under IPv6, counter udpInErrors increased, but under IPv4 counter udpInDatagrams increased. I lookup into the source code, and found that, under IPv4 UDP datagrams with checksum error will be delivered to UDP receive queue, but IPv6 does not. IPv4 delivered into UDP receive queue, increased udpInDatagrams, then discard it before delivered to UDP user. RFC said udpInDatagrams is the total number of UDP datagrams delivered to UDP users, so udpInDatagrams should not be increased while UDP datagrams with checksum error received.

Refer to RFC2013:
udpInDatagrams OBJECT-TYPE

      SYNTAX      Counter32
      MAX-ACCESS  read-only
      STATUS      current
      DESCRIPTION
              "The total number of UDP datagrams delivered to UDP
users."
      ::= { udp 1 }

Following is my patch:

@@ -1018,7 +1018,7 @@ static int udp_queue_rcv_skb(struct sock
 		/* FALLTHROUGH -- it's a UDP Packet */
 	}
 
-	if (sk->sk_filter && skb->ip_summed != CHECKSUM_UNNECESSARY) {
+	if (skb->ip_summed != CHECKSUM_UNNECESSARY) {
 		if (__udp_checksum_complete(skb)) {
 			UDP_INC_STATS_BH(UDP_MIB_INERRORS);
 			kfree_skb(skb);

This has been raised earlier, cf. http://bugzilla.kernel.org/show_bug.cgi?id=6660

Wei Yongjun wrote:

|  When I send a UDP datagrams with checksum error to target, I found that:
|  Under IPv6, counter udpInErrors increased, but under IPv4 counter
|  udpInDatagrams increased. I lookup into the source code, and found that,
|  under IPv4 UDP datagrams with checksum error will be delivered to UDP
|  receive queue, but IPv6 does not. IPv4 delivered into UDP receive queue,
|  increased udpInDatagrams, then discard it before delivered to UDP user.
|  RFC said udpInDatagrams is the total number of UDP datagrams delivered
|  to UDP users, so udpInDatagrams should not be increased while UDP
|  datagrams with checksum error received.
|  
|  Refer to RFC2013:
|    udpInDatagrams OBJECT-TYPE
|        SYNTAX      Counter32
|        MAX-ACCESS  read-only
|        STATUS      current
|        DESCRIPTION
|                "The total number of UDP datagrams delivered to UDP
|  users."
|        ::= { udp 1 }
|  
|  Following is my patch:
|  
|  --- a/net/ipv4/udp.c	2006-07-31 09:33:45.392479344 -0400
|  +++ b/net/ipv4/udp.c	2006-07-31 09:34:26.430240656 -0400
|  @@ -1018,7 +1018,7 @@ static int udp_queue_rcv_skb(struct sock
|   		/* FALLTHROUGH -- it's a UDP Packet */
|   	}
|   
|  -	if (sk->sk_filter && skb->ip_summed != CHECKSUM_UNNECESSARY) {
|  +	if (skb->ip_summed != CHECKSUM_UNNECESSARY) {
|   		if (__udp_checksum_complete(skb)) {
|   			UDP_INC_STATS_BH(UDP_MIB_INERRORS);
|   			kfree_skb(skb);
|  
|  Signed-off-by: Wei Yongjun <[email protected]>
|  
|  
|  -
|  To unsubscribe from this list: send the line "unsubscribe netdev" in
|  the body of a message to [email protected]
|  More majordomo info at  http://vger.kernel.org/majordomo-info.html
|  
|  

Hi,

|  if (!sk->sk_filter && skb->ip_summed != CHECKSUM_UNNECESSARY) {
|  
|  IPv6 doesn't do this, so I think delete condition 'sk->sk_filter' is better. 
|  Do you think so?

I think the sk->sk_filter is there for a good reason. If you delete it, that routine is forced to always compute UDP checksums, even if the only receiving application is a tcpdump process. I may be wrong here, but I think that deleting the sk_filter statement is not at a good idea.

The other alternatives discussed (afaik) so far were:

1) Move the increment of UDP_MIB_INDATAGRAMS from udp_queue_rcv_skb() to udp_recvmsg() (first patch uploaded to http://bugzilla.kernel.org/show_bug.cgi?id=6660). This was discussed: not a good idea, since in-kernel applications may use the data_ready handler rather than udp_recvmsg().

2) Decrement UDP_MIB_INDATAGRAMS in udp_recvmsg() when the checksum turns out to be wrong (second patch uploaded to above address). This would be a fix to the problem you are stating, it also solves the problem of missing out the data_ready handlers in (1), and was suggested earlier on this mailing list.

-- Gerrit