| Message ID | 20220201222845.3640041-1-jeffreyji@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v6,net-next] net-core: add InMacErrors counter | expand |
On Tue, 1 Feb 2022 22:28:45 +0000 Jeffrey Ji wrote: > From: jeffreyji <jeffreyji@google.com> > > Increment InMacErrors counter when packet dropped due to incorrect dest > MAC addr. > > An example when this drop can occur is when manually crafting raw > packets that will be consumed by a user space application via a tap > device. For testing purposes local traffic was generated using trafgen > for the client and netcat to start a server > > example output from nstat: > \~# nstat -a | grep InMac > Ip6InMacErrors 0 0.0 > IpExtInMacErrors 1 0.0 I had another thing and this still doesn't sit completely well with me :( Shouldn't we count those drops as skb->dev->rx_dropped? Commonly NICs will do such filtering and if I got it right in struct rtnl_link_stats64 kdoc - report them as rx_dropped. It'd be inconsistent if on a physical interface we count these as rx_dropped and on SW interface (or with promisc enabled etc.) in the SNMP counters. Or we can add a new link stat that NICs can use as well. In fact I'm not sure this is really a IP AKA L3 statistic, it's the L2 address that doesn't match. If everyone disagrees - should we at least rename the MIB counter similarly to the drop reason? Experience shows that users call for help when they see counters with Error in their name, I'd vote for IpExtInDropOtherhost or some such. The statistic should also be documented in Documentation/networking/snmp_counter.rst
On Wed, Feb 2, 2022 at 8:59 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 1 Feb 2022 22:28:45 +0000 Jeffrey Ji wrote: > > From: jeffreyji <jeffreyji@google.com> > > > > Increment InMacErrors counter when packet dropped due to incorrect dest > > MAC addr. > > > > An example when this drop can occur is when manually crafting raw > > packets that will be consumed by a user space application via a tap > > device. For testing purposes local traffic was generated using trafgen > > for the client and netcat to start a server > > > > example output from nstat: > > \~# nstat -a | grep InMac > > Ip6InMacErrors 0 0.0 > > IpExtInMacErrors 1 0.0 > > I had another thing and this still doesn't sit completely well > with me :( > > Shouldn't we count those drops as skb->dev->rx_dropped? > Commonly NICs will do such filtering and if I got it right > in struct rtnl_link_stats64 kdoc - report them as rx_dropped. > It'd be inconsistent if on a physical interface we count > these as rx_dropped and on SW interface (or with promisc enabled > etc.) in the SNMP counters. > Or we can add a new link stat that NICs can use as well. > > In fact I'm not sure this is really a IP AKA L3 statistic, > it's the L2 address that doesn't match. > > > If everyone disagrees - should we at least rename the MIB counter > similarly to the drop reason? Experience shows that users call for > help when they see counters with Error in their name, I'd vote for > IpExtInDropOtherhost or some such. The statistic should also be > documented in Documentation/networking/snmp_counter.rst Changing the Name to IpExtInDropOtherhost and adding the documentation makes sense to me. What do others think? I'd like to get more feedback before Jeffrey sends another version.
On Wed, Feb 2, 2022 at 8:59 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 1 Feb 2022 22:28:45 +0000 Jeffrey Ji wrote: > > From: jeffreyji <jeffreyji@google.com> > > > > Increment InMacErrors counter when packet dropped due to incorrect dest > > MAC addr. > > > > An example when this drop can occur is when manually crafting raw > > packets that will be consumed by a user space application via a tap > > device. For testing purposes local traffic was generated using trafgen > > for the client and netcat to start a server > > > > example output from nstat: > > \~# nstat -a | grep InMac > > Ip6InMacErrors 0 0.0 > > IpExtInMacErrors 1 0.0 > > I had another thing and this still doesn't sit completely well > with me :( > > Shouldn't we count those drops as skb->dev->rx_dropped? > Commonly NICs will do such filtering and if I got it right > in struct rtnl_link_stats64 kdoc - report them as rx_dropped. > It'd be inconsistent if on a physical interface we count > these as rx_dropped and on SW interface (or with promisc enabled > etc.) in the SNMP counters. I like to see skb->dev->rx_dropped as a fallback-catch-all bucket for all cases we do not already have a more specific counter. > Or we can add a new link stat that NICs can use as well. Yes, this could be done, but we have to be careful about not hitting a single cache line, for the cases we receive floods of such messages on multiqueue NIC. (The single atomic in dev->rx_dropped) is suffering from this issue btw) > > In fact I'm not sure this is really a IP AKA L3 statistic, > it's the L2 address that doesn't match. > > > If everyone disagrees - should we at least rename the MIB counter > similarly to the drop reason? Experience shows that users call for > help when they see counters with Error in their name, I'd vote for > IpExtInDropOtherhost or some such. The statistic should also be > documented in Documentation/networking/snmp_counter.rst
On Thu, 3 Feb 2022 10:13:59 -0800 Eric Dumazet wrote: > > I had another thing and this still doesn't sit completely well > > with me :( > > > > Shouldn't we count those drops as skb->dev->rx_dropped? > > Commonly NICs will do such filtering and if I got it right > > in struct rtnl_link_stats64 kdoc - report them as rx_dropped. > > It'd be inconsistent if on a physical interface we count > > these as rx_dropped and on SW interface (or with promisc enabled > > etc.) in the SNMP counters. > > I like to see skb->dev->rx_dropped as a fallback-catch-all bucket > for all cases we do not already have a more specific counter. Indeed, it's a fallback so counting relatively common events like unicast filtering into generic "drops" feels wrong. I heard complaints that this is non-intuitive and makes debug harder in the past. > > Or we can add a new link stat that NICs can use as well. > > Yes, this could be done, but we have to be careful about not hitting > a single cache line, for the cases we receive floods of such messages > on multiqueue NIC. > (The single atomic in dev->rx_dropped) is suffering from this issue btw) Even more of a reason to bite the bullet and move from the atomic counters to pcpu stats? > > In fact I'm not sure this is really a IP AKA L3 statistic, > > it's the L2 address that doesn't match. > > > > > > If everyone disagrees - should we at least rename the MIB counter > > similarly to the drop reason? Experience shows that users call for > > help when they see counters with Error in their name, I'd vote for > > IpExtInDropOtherhost or some such. The statistic should also be > > documented in Documentation/networking/snmp_counter.rst
On Thu, 3 Feb 2022 10:05:17 -0800 Brian Vazquez wrote: > > If everyone disagrees - should we at least rename the MIB counter > > similarly to the drop reason? Experience shows that users call for > > help when they see counters with Error in their name, I'd vote for > > IpExtInDropOtherhost or some such. The statistic should also be > > documented in Documentation/networking/snmp_counter.rst > > Changing the Name to IpExtInDropOtherhost and adding the documentation > makes sense to me. What do others think? I'd like to get more feedback > before Jeffrey sends another version. I'm not sure we reached the "everyone disagrees with me" point at least not yet ;)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index a27bcc4f7e9a..1b1114f5c68e 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -320,6 +320,7 @@ enum skb_drop_reason { SKB_DROP_REASON_TCP_CSUM, SKB_DROP_REASON_SOCKET_FILTER, SKB_DROP_REASON_UDP_CSUM, + SKB_DROP_REASON_OTHERHOST, SKB_DROP_REASON_MAX, }; diff --git a/include/uapi/linux/snmp.h b/include/uapi/linux/snmp.h index 904909d020e2..ac2fac12dd7d 100644 --- a/include/uapi/linux/snmp.h +++ b/include/uapi/linux/snmp.h @@ -57,6 +57,7 @@ enum IPSTATS_MIB_ECT0PKTS, /* InECT0Pkts */ IPSTATS_MIB_CEPKTS, /* InCEPkts */ IPSTATS_MIB_REASM_OVERLAPS, /* ReasmOverlaps */ + IPSTATS_MIB_INMACERRORS, /* InMacErrors */ __IPSTATS_MIB_MAX }; diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c index 3a025c011971..780892526166 100644 --- a/net/ipv4/ip_input.c +++ b/net/ipv4/ip_input.c @@ -441,8 +441,11 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net) /* When the interface is in promisc. mode, drop all the crap * that it receives, do not try to analyse it. */ - if (skb->pkt_type == PACKET_OTHERHOST) - goto drop; + if (skb->pkt_type == PACKET_OTHERHOST) { + __IP_INC_STATS(net, IPSTATS_MIB_INMACERRORS); + kfree_skb_reason(skb, SKB_DROP_REASON_OTHERHOST); + return NULL; + } __IP_UPD_PO_STATS(net, IPSTATS_MIB_IN, skb->len); diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c index 28836071f0a6..2be4189197f3 100644 --- a/net/ipv4/proc.c +++ b/net/ipv4/proc.c @@ -117,6 +117,7 @@ static const struct snmp_mib snmp4_ipextstats_list[] = { SNMP_MIB_ITEM("InECT0Pkts", IPSTATS_MIB_ECT0PKTS), SNMP_MIB_ITEM("InCEPkts", IPSTATS_MIB_CEPKTS), SNMP_MIB_ITEM("ReasmOverlaps", IPSTATS_MIB_REASM_OVERLAPS), + SNMP_MIB_ITEM("InMacErrors", IPSTATS_MIB_INMACERRORS), SNMP_MIB_SENTINEL }; diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c index 80256717868e..da18d9159647 100644 --- a/net/ipv6/ip6_input.c +++ b/net/ipv6/ip6_input.c @@ -149,15 +149,17 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev, u32 pkt_len; struct inet6_dev *idev; - if (skb->pkt_type == PACKET_OTHERHOST) { - kfree_skb(skb); - return NULL; - } - rcu_read_lock(); idev = __in6_dev_get(skb->dev); + if (skb->pkt_type == PACKET_OTHERHOST) { + __IP6_INC_STATS(net, idev, IPSTATS_MIB_INMACERRORS); + rcu_read_unlock(); + kfree_skb_reason(skb, SKB_DROP_REASON_OTHERHOST); + return NULL; + } + __IP6_UPD_PO_STATS(net, idev, IPSTATS_MIB_IN, skb->len); if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL || diff --git a/net/ipv6/proc.c b/net/ipv6/proc.c index d6306aa46bb1..76e6119ba558 100644 --- a/net/ipv6/proc.c +++ b/net/ipv6/proc.c @@ -84,6 +84,7 @@ static const struct snmp_mib snmp6_ipstats_list[] = { SNMP_MIB_ITEM("Ip6InECT1Pkts", IPSTATS_MIB_ECT1PKTS), SNMP_MIB_ITEM("Ip6InECT0Pkts", IPSTATS_MIB_ECT0PKTS), SNMP_MIB_ITEM("Ip6InCEPkts", IPSTATS_MIB_CEPKTS), + SNMP_MIB_ITEM("Ip6InMacErrors", IPSTATS_MIB_INMACERRORS), SNMP_MIB_SENTINEL };