All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 net-next] net-core: add InMacErrors counter
@ 2022-02-01 22:28 Jeffrey Ji
  2022-02-03  4:59 ` Jakub Kicinski
  0 siblings, 1 reply; 6+ messages in thread
From: Jeffrey Ji @ 2022-02-01 22:28 UTC (permalink / raw)
  To: Eric Dumazet, David S . Miller
  Cc: Brian Vazquez, linux-kernel, kuba, netdev, jeffreyjilinux, jeffreyji

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

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 <jeffreyji@google.com>
---
 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 net-next] net-core: add InMacErrors counter
  2022-02-01 22:28 [PATCH v6 net-next] net-core: add InMacErrors counter Jeffrey Ji
@ 2022-02-03  4:59 ` Jakub Kicinski
  2022-02-03 18:05   ` Brian Vazquez
  2022-02-03 18:13   ` Eric Dumazet
  0 siblings, 2 replies; 6+ messages in thread
From: Jakub Kicinski @ 2022-02-03  4:59 UTC (permalink / raw)
  To: Jeffrey Ji
  Cc: Eric Dumazet, David S . Miller, Brian Vazquez, linux-kernel,
	netdev, jeffreyji

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 net-next] net-core: add InMacErrors counter
  2022-02-03  4:59 ` Jakub Kicinski
@ 2022-02-03 18:05   ` Brian Vazquez
  2022-02-03 19:35     ` Jakub Kicinski
  2022-02-03 18:13   ` Eric Dumazet
  1 sibling, 1 reply; 6+ messages in thread
From: Brian Vazquez @ 2022-02-03 18:05 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Jeffrey Ji, Eric Dumazet, David S . Miller, linux-kernel, netdev,
	jeffreyji

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 net-next] net-core: add InMacErrors counter
  2022-02-03  4:59 ` Jakub Kicinski
  2022-02-03 18:05   ` Brian Vazquez
@ 2022-02-03 18:13   ` Eric Dumazet
  2022-02-03 19:34     ` Jakub Kicinski
  1 sibling, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2022-02-03 18:13 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Jeffrey Ji, David S . Miller, Brian Vazquez, LKML, netdev, jeffreyji

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 net-next] net-core: add InMacErrors counter
  2022-02-03 18:13   ` Eric Dumazet
@ 2022-02-03 19:34     ` Jakub Kicinski
  0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2022-02-03 19:34 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Jeffrey Ji, David S . Miller, Brian Vazquez, LKML, netdev, jeffreyji

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  


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 net-next] net-core: add InMacErrors counter
  2022-02-03 18:05   ` Brian Vazquez
@ 2022-02-03 19:35     ` Jakub Kicinski
  0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2022-02-03 19:35 UTC (permalink / raw)
  To: Brian Vazquez
  Cc: Jeffrey Ji, Eric Dumazet, David S . Miller, linux-kernel, netdev,
	jeffreyji

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-02-03 19:35 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-01 22:28 [PATCH v6 net-next] net-core: add InMacErrors counter Jeffrey Ji
2022-02-03  4:59 ` Jakub Kicinski
2022-02-03 18:05   ` Brian Vazquez
2022-02-03 19:35     ` Jakub Kicinski
2022-02-03 18:13   ` Eric Dumazet
2022-02-03 19:34     ` Jakub Kicinski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.