Re: [PATCH] udp: increment UDP_MIB_NOPORTS in mcast receive

[Eric Dumazet <eric.dumazet@gmail.com>]

On Wed, 2012-10-03 at 10:09 -0400, David Stevens wrote:
> Eric Dumazet <eric.dumazet@gmail.com> wrote on 10/03/2012 09:15:51 AM:
>  
> > So when a host receives an UDP datagram but there was no application
> > at the destination port we should increment udpNoPorts, and its not
> > an error but just a fact.
> 
>         Of course. I think our difference is on the definition of 
> "receives".

A receive is a packet delivered to this host.
Interface being promiscuous or not doesnt really matter.

> I don't think a packet delivered locally due to promiscuous mode, 
> broadcast
> or an imperfect multicast address filter match is a host UDP datagram 
> receive.
> These packets really shouldn't be delivered to UDP at all; they are not
> addressed to this host (at least the non-broadcast, no-membership ones).

Thats the bug we currently are tracking. If some error is happening and
packet is delivered instead of being forwarded or dropped, we need a
counter being incremented to catch the bug.

>         A unicast UDP packet that doesn't match a local IP address does 
> not
> increment this counter. 

It _does_ increment this counter right now, not sure what you mean.

We currently correctly increment udpNoPorts if we receive an unicast UDP
packet that doesnt find a matching socket (because socket(s) are bound
to specific addresses instead of ANY_ADDR)

This is an extension of the "there was no application at the destination
port" to "there was no application at the destination port and
destination address"

> A promiscuous mode multicast delivery is no 
> different,
> except that the destination alone doesn't tell us if it is for us.
> 
>         I think counting these will primarily lead to administrators 
> seeing
> non-zero drops and wasting their time trying to track them down.

Well, as I said, seeing increments of this counter is perfectly fine and
matches RFC. It permits better diagnostics. Hiding bugs is not very
helpful.

Most of the time I am trying to track a bug in linux network stack, the
very first thing I ask to reporters is to post "netstat -s" before/after
their tests exactly because I want to see _some_ counters be incremented
and catch obvious problems.

And alas, many drops in our stack are not correctly reported because we
forgot to increment a counter at the right place.

I am fine adding a new SNMP McastDrops counter if you feel its better.

# grep Udp: /proc/net/snmp
Udp: InDatagrams NoPorts InErrors OutDatagrams RcvbufErrors SndbufErrors McastDrops
Udp: 11449164 15473 514616 290821178 0 184352 134

"netstat -s -u" would display :

Udp:
    11449164 packets received
    15473 packets to unknown port received.
    514616 packet receive errors
    290821178 packets sent
    SndbufErrors: 184352
    McastDrops: 134


Non official patch since net-next is not open :

 include/linux/snmp.h |    1 +
 net/ipv4/proc.c      |    1 +
 net/ipv4/udp.c       |    2 ++
 net/ipv6/proc.c      |    2 ++
 net/ipv6/udp.c       |    2 ++
 5 files changed, 8 insertions(+)

diff --git a/include/linux/snmp.h b/include/linux/snmp.h
index 00bc189..321d643 100644
--- a/include/linux/snmp.h
+++ b/include/linux/snmp.h
@@ -145,6 +145,7 @@ enum
 	UDP_MIB_OUTDATAGRAMS,			/* OutDatagrams */
 	UDP_MIB_RCVBUFERRORS,			/* RcvbufErrors */
 	UDP_MIB_SNDBUFERRORS,			/* SndbufErrors */
+	UDP_MIB_MCASTDROPS,			/* McastDrops (linux extension) */
 	__UDP_MIB_MAX
 };
 
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index 957acd1..1e932ee 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -172,6 +172,7 @@ static const struct snmp_mib snmp4_udp_list[] = {
 	SNMP_MIB_ITEM("OutDatagrams", UDP_MIB_OUTDATAGRAMS),
 	SNMP_MIB_ITEM("RcvbufErrors", UDP_MIB_RCVBUFERRORS),
 	SNMP_MIB_ITEM("SndbufErrors", UDP_MIB_SNDBUFERRORS),
+	SNMP_MIB_ITEM("McastDrops", UDP_MIB_MCASTDROPS),
 	SNMP_MIB_SENTINEL
 };
 
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 2814f66..4e2a4f7 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1591,6 +1591,8 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 			sock_put(stack[i]);
 	} else {
 		kfree_skb(skb);
+		UDP_INC_STATS_BH(net, UDP_MIB_MCASTDROPS,
+				 udptable != &udp_table);
 	}
 	return 0;
 }
diff --git a/net/ipv6/proc.c b/net/ipv6/proc.c
index 745a320..f2c12ea 100644
--- a/net/ipv6/proc.c
+++ b/net/ipv6/proc.c
@@ -129,6 +129,7 @@ static const struct snmp_mib snmp6_udp6_list[] = {
 	SNMP_MIB_ITEM("Udp6OutDatagrams", UDP_MIB_OUTDATAGRAMS),
 	SNMP_MIB_ITEM("Udp6RcvbufErrors", UDP_MIB_RCVBUFERRORS),
 	SNMP_MIB_ITEM("Udp6SndbufErrors", UDP_MIB_SNDBUFERRORS),
+	SNMP_MIB_ITEM("Udp6McastDrops", UDP_MIB_MCASTDROPS),
 	SNMP_MIB_SENTINEL
 };
 
@@ -139,6 +140,7 @@ static const struct snmp_mib snmp6_udplite6_list[] = {
 	SNMP_MIB_ITEM("UdpLite6OutDatagrams", UDP_MIB_OUTDATAGRAMS),
 	SNMP_MIB_ITEM("UdpLite6RcvbufErrors", UDP_MIB_RCVBUFERRORS),
 	SNMP_MIB_ITEM("UdpLite6SndbufErrors", UDP_MIB_SNDBUFERRORS),
+	SNMP_MIB_ITEM("UdpLite6McastDrops", UDP_MIB_MCASTDROPS);
 	SNMP_MIB_SENTINEL
 };
 
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 07e2bfe..c8caf1b 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -748,6 +748,8 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 			sock_put(stack[i]);
 	} else {
 		kfree_skb(skb);
+		UDP6_INC_STATS_BH(net, UDP_MIB_MCASTDROPS,
+				  udptable != &udp_table);
 	}
 	return 0;
 }