From: jeffreyji <[email protected]>
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
Tested: Created 2 netns, sent 1 packet using trafgen from 1 to the other
with "{eth(daddr=$INCORRECT_MAC...}", verified that nstat showed the
counter was incremented.
changelog:
v6: rebase onto net-next
v5:
Change from SKB_DROP_REASON_BAD_DEST_MAC to SKB_DROP_REASON_OTHERHOST
v3-4:
Remove Change-Id
v2:
Use skb_free_reason() for tracing
Add real-life example in patch msg
Signed-off-by: jeffreyji <[email protected]>
---
include/linux/skbuff.h | 1 +
include/uapi/linux/snmp.h | 1 +
net/ipv4/ip_input.c | 7 +++++--
net/ipv4/proc.c | 1 +
net/ipv6/ip6_input.c | 12 +++++++-----
net/ipv6/proc.c | 1 +
6 files changed, 16 insertions(+), 7 deletions(-)
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
};
--
2.35.0.rc2.247.g8bbb082509-goog
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 ;)
On Wed, Feb 2, 2022 at 8:59 PM Jakub Kicinski <[email protected]> wrote:
>
> On Tue, 1 Feb 2022 22:28:45 +0000 Jeffrey Ji wrote:
> > From: jeffreyji <[email protected]>
> >
> > 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 Tue, 1 Feb 2022 22:28:45 +0000 Jeffrey Ji wrote:
> From: jeffreyji <[email protected]>
>
> 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 <[email protected]> wrote:
>
> On Tue, 1 Feb 2022 22:28:45 +0000 Jeffrey Ji wrote:
> > From: jeffreyji <[email protected]>
> >
> > 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.