Re: if_ethersubr.c: if_ierrors -> if_iqdrops?

[SAITOH Masanobu <msaitoh%execsw.org@localhost>]

Hi.

On 2021/11/11 17:32, RVP wrote:
> On Wed, 10 Nov 2021, Ryota Ozaki wrote:
>
> > Another option may be if_noproto.
> >
> >  ozaki-r
>
> On Wed, 10 Nov 2021, Havard Eidnes wrote:
>
> > which further supports the suggestion to use if_noproto for the
> > stated condition.
>
> I'll use `if_noproto'. Should've done that from the start--missed
> the forest for the trees, me.
>
> I'm still seeing a few error packets after hitting this test:
>
>      894         if (etype <= ETHERMTU + sizeof(struct ether_header)) {
>
> `etype' here (len by this point) is 8.

In my home's network, some machines send 802.2 LLC packet that the
etype = 6. A few machines also send LLDP packets.

I wrote a patch for better counting:
------------
Better counting for ierrors, iqdrops and noproto in ether_input().

 - Use if_noproto for unknown or unsupported protocols.
 - Use if_ierror for wrong mbuf and oversized frame.

Index: if_ethersubr.c
===================================================================
RCS file: /cvsroot/src/sys/net/if_ethersubr.c,v
retrieving revision 1.304
diff -u -p -r1.304 if_ethersubr.c
--- if_ethersubr.c	15 Nov 2021 07:07:05 -0000	1.304
+++ if_ethersubr.c	22 Nov 2021 08:04:46 -0000
@@ -584,7 +584,7 @@ ether_input_llc(struct ifnet *ifp, struc
 	struct llc *l;

 	if (m->m_len < sizeof(*eh) + sizeof(struct llc))
-		goto drop;
+		goto error;

 	l = (struct llc *)(eh+1);
 	switch (l->llc_dsap) {
@@ -593,7 +593,7 @@ ether_input_llc(struct ifnet *ifp, struc
 		switch (l->llc_control) {
 		case LLC_UI:
 			if (l->llc_ssap != LLC_SNAP_LSAP)
-				goto drop;
+				goto error;

 			if (memcmp(&(l->llc_snap_org_code)[0],
 			    at_org_code, sizeof(at_org_code)) == 0 &&
@@ -618,21 +618,25 @@ ether_input_llc(struct ifnet *ifp, struc
 			}

 		default:
-			goto drop;
+			goto error;
 		}
 		break;
 #endif
 	default:
-		goto drop;
+		goto noproto;
 	}

 	KASSERT(inq != NULL);
 	IFQ_ENQUEUE_ISR(inq, m, isr);
 	return;

-drop:
+noproto:
 	m_freem(m);
-	if_statinc(ifp, if_ierrors); /* XXX should have a dedicated counter? */
+	if_statinc(ifp, if_noproto);
+	return;
+error:
+	m_freem(m);
+	if_statinc(ifp, if_ierrors);
 	return;
 }
 #endif /* defined (LLC) || defined (NETATALK) */
@@ -699,7 +703,7 @@ ether_input(struct ifnet *ifp, struct mb
 		}
 		mutex_exit(&bigpktpps_lock);
 #endif
-		goto drop;
+		goto error;
 	}

 	if (ETHER_IS_MULTICAST(eh->ether_dhost)) {
@@ -795,7 +799,7 @@ ether_input(struct ifnet *ifp, struct mb
 		vlan_input(ifp, m);
 		return;
 #else
-		goto drop;
+		goto noproto;
 #endif
 	}

@@ -832,7 +836,7 @@ ether_input(struct ifnet *ifp, struct mb
 			return;
 		} else
 #endif
-			goto drop;
+			goto noproto;
 	}

 #if NPPPOE > 0
@@ -849,7 +853,7 @@ ether_input(struct ifnet *ifp, struct mb
 		uint8_t subtype;

 		if (m->m_pkthdr.len < sizeof(*eh) + sizeof(subtype))
-			goto drop;
+			goto error;

 		m_copydata(m, sizeof(*eh), sizeof(subtype), &subtype);
 		switch (subtype) {
@@ -897,7 +901,7 @@ ether_input(struct ifnet *ifp, struct mb
 		ether_input_llc(ifp, m, eh);
 		return;
 #else
-		goto error;
+		goto noproto;
 #endif
 	}

@@ -927,7 +931,7 @@ ether_input(struct ifnet *ifp, struct mb
 #ifdef INET6
 	case ETHERTYPE_IPV6:
 		if (__predict_false(!in6_present))
-			goto drop;
+			goto noproto;
 #ifdef GATEWAY
 		if (ip6flow_fastforward(&m))
 			return;
@@ -955,7 +959,7 @@ ether_input(struct ifnet *ifp, struct mb
 #endif

 	default:
-		goto drop;
+		goto noproto;
 	}

 	if (__predict_true(pktq)) {
@@ -976,11 +980,15 @@ ether_input(struct ifnet *ifp, struct mb

 drop:
 	m_freem(m);
-	if_statinc(ifp, if_iqdrops); /* XXX should have a dedicated counter? */
+	if_statinc(ifp, if_iqdrops);
+	return;
+noproto:
+	m_freem(m);
+	if_statinc(ifp, if_noproto);
 	return;
 error:
 	m_freem(m);
-	if_statinc(ifp, if_ierrors); /* XXX should have a dedicated counter? */
+	if_statinc(ifp, if_ierrors);
 	return;
 }
------------

The same diff is at:
	https://www.netbsd.org/~msaitoh/ether_input_noproto-20211122-0.dif

Is the above change acceptable?

> Don't know if it is the TP-Link
> WiFi extender I'm cabled to that is sending these short frames or what.
> The alc driver isn't reporting any received error counts at any rate.
>
> Thanks,
> -RVP


--
-----------------------------------------------
                SAITOH Masanobu (msaitoh%execsw.org@localhost
                                 msaitoh%netbsd.org@localhost)