diff mbox

udp: increment UDP_MIB_NOPORTS in mcast receive

Message ID 1349249328.12401.1364.camel@edumazet-glaptop
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet Oct. 3, 2012, 7:28 a.m. UTC 
On Wed, 2012-10-03 at 02:24 +0300, Julian Anastasov wrote:
> 	Hello,
> 
> On Tue, 2 Oct 2012, Eric Dumazet wrote:
> 
> > > David, shouldnt we use a nh_rth_forward instead of a nh_rth_input in
> > > __mkroute_input() ?
> > > 
> > > (And change rt_cache_route() as well ?)
> > > 
> > > I am testing a patch right now.
> > 
> > Yeah, this patch seems to fix the bug for me.
> > 
> > [PATCH] ipv4: properly cache forward routes
> > 
> > commit d2d68ba9fe8 (ipv4: Cache input routes in fib_info nexthops.)
> > introduced a regression for forwarding.
> > 
> > This was hard to reproduce but the symptom was that packets were
> > delivered to local host instead of being forwarded.
> > 
> > Add a separate cache (nh_rth_forward) to solve the problem.
> 
> 	Can it be a problem related to fib_info reuse
> from different routes. For example, when local IP address
> is created for subnet we have:
> 
> broadcast 192.168.0.255 dev DEV  proto kernel  scope link  src 192.168.0.1
> 192.168.0.0/24 dev DEV  proto kernel  scope link  src 192.168.0.1
> local 192.168.0.1 dev DEV  proto kernel  scope host  src 192.168.0.1
> 
> 	The "dev DEV  proto kernel  scope link  src 192.168.0.1" is
> a reused fib_info structure where we put cached routes.
> The result can be same fib_info for 192.168.0.255 and
> 192.168.0.0/24. RTN_BROADCAST is cached only for input
> routes. Incoming broadcast to 192.168.0.255 can be cached
> and can cause problems for traffic forwarded to 192.168.0.0/24.
> So, this patch should solve the problem because it
> separates the broadcast from unicast traffic.
> 
> 	And the ip_route_input_slow caching will work for
> local and broadcast input routes (above routes 1 and 3) just
> because they differ in scope and use different fib_info.
> 
> 	Another possible failure is for output routes:
> 
> multicast 224.0.0.0/4 fib_info
> with unicast
> 192.168.0.0/24 fib_info
> 
> 	The multicast sets RTCF_MULTICAST | RTCF_LOCAL
> and can cause problems for generated unicast traffic on
> fib_info reuse. Depends on the scope, for multicast it is
> usually scope global, so may be it is difficult to happen
> in practice.
> 
> 	__mkroute_output works for local/unicast routes
> because they differ in scope.


Thanks Julian for these informations.

BTW, it seems we dont properly increase UDP MIB counters when a
multicast message is not delivered to at least one socket.

Lets fix this to ease future bug hunting.

I hate when "netstat -s" is useless and we have to use dropwatch to
figure out where we drop a frame.

[PATCH] udp: increment UDP_MIB_NOPORTS in multicast receive

We should increment UDP_MIB_NOPORTS in the case we found
no socket to deliver a copy of one incoming UDP message.

(RFC 4113 udpNoPorts)

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/udp.c |    1 +
 net/ipv6/udp.c |    1 +
 2 files changed, 2 insertions(+)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Stevens Oct. 3, 2012, 12:45 p.m. UTC | #1 
netdev-owner@vger.kernel.org wrote on 10/03/2012 03:28:48 AM:
 
> BTW, it seems we dont properly increase UDP MIB counters when a
> multicast message is not delivered to at least one socket.

If an interface is in promiscuous mode or there are false
positives in a multicast address filter, wouldn't this count as
"drops" packets that were never intended for this machine?

I think an otherwise valid multicast or broadcast packet that doesn't
have a local receiver is not an error and shouldn't be counted.

                                                        +-DLS

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet Oct. 3, 2012, 1:15 p.m. UTC | #2 
On Wed, 2012-10-03 at 08:45 -0400, David Stevens wrote:
> netdev-owner@vger.kernel.org wrote on 10/03/2012 03:28:48 AM:
>  
> > BTW, it seems we dont properly increase UDP MIB counters when a
> > multicast message is not delivered to at least one socket.
> 
> If an interface is in promiscuous mode or there are false
> positives in a multicast address filter, wouldn't this count as
> "drops" packets that were never intended for this machine?
> 

Yes, probably. So we drop them and its expected.

> I think an otherwise valid multicast or broadcast packet that doesn't
> have a local receiver is not an error and shouldn't be counted.

Hmmm

This counter is not an "error counter", just a "counter".

RFC definitions are exactly :

udpNoPorts OBJECT-TYPE
       SYNTAX     Counter32
       MAX-ACCESS read-only
       STATUS     current
       DESCRIPTION
              "The total number of received UDP datagrams for which
               there was no application at the destination port.

udpInErrors OBJECT-TYPE
       SYNTAX     Counter32
       MAX-ACCESS read-only
       STATUS     current
       DESCRIPTION
              "The number of received UDP datagrams that could not be
               delivered for reasons other than the lack of an
               application at the destination port.


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.

Now _if_ some reader interprets udpNoPorts increases as an indication
of errors, this reader is wrong.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Stevens Oct. 3, 2012, 2:09 p.m. UTC | #3 
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".
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).
        A unicast UDP packet that doesn't match a local IP address does 
not
increment this counter. 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.

                                                                +-DLS


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rick Jones Oct. 3, 2012, 5:39 p.m. UTC | #4 
On 10/03/2012 07:09 AM, David Stevens wrote:
> Of course. I think our difference is on the definition of
> "receives". 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). A unicast UDP packet
> that doesn't match a local IP address does not increment this
> counter. 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.

I would tend to agree with David on this one. Or they might cease trying 
to track them down because they've gotten so many "false positives."

Isn't "meant for me" vs "not meant for me" at the heard of "drops" 
versus "discards?"

Once the packet is in the host, is it tagged in some way with "this was 
received as promiscuous/whatnot?"

rick

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 79c8dbe..dfa73c5 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1591,6 +1591,7 @@  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_NOPORTS, udptable != &udp_table);
 	}
 	return 0;
 }
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index fc99972..0be9ac2 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -748,6 +748,7 @@  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_NOPORTS, udptable != &udp_table);
 	}
 	return 0;
 }