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

Eric Dumazet
2012-10-03T15:29:19Z

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;
}