Re: 5.3-RELEASE TODO

On 2004-10-08 01:41, Scott Long <scottl_at_freebsd.org> wrote:
>  |--------------------+-------------+---------------+---------------------|
>  |                    |             |               | ether_input() calls |
>  |                    |             |               | random_harvest() on |
>  |                    |             |               | the mbuf after it   |
>  |                    |             |               | has been handed off |
>  |                    |             |               | to ether_demux(),   |
>  | ether_input() may  |             |               | at which point it   |
>  | harvest entropy    | In progress | Mark Murray,  | may have been       |
>  | from free()'d mbuf |             | Robert Watson | free()'d back to    |
>  |                    |             |               | the mbuf allocator. |
>  |                    |             |               | It also passes in a |
>  |                    |             |               | pointer to the mbuf |
>  |                    |             |               | itself, rather than |
>  |                    |             |               | ethernet frame      |
>  |                    |             |               | header.             |
>  +------------------------------------------------------------------------+

Are we allowed to change the prototype of ether_demux()?

If yes, we could make ether_demux() return -1 if it frees the mbuf, and
avoid calling random_harvest() on that particular frame.  Note that I
haven't had a chance to test build or run the following patch yet, but
if anyone has a better idea or comments that would help us improve it,
they're welcome.

One note that I have taken down while writing this and I have to check
is the possibility of calling random_harvest(m->m_data, 16, ...) with an
mbuf whose m->m_data contains less than 16 bytes.

--- patch begins here ---
Index: ethernet.h
===================================================================
RCS file: /home/ncvs/src/sys/net/ethernet.h,v
retrieving revision 1.24
diff -u -u -r1.24 ethernet.h
--- ethernet.h	5 Oct 2004 19:28:52 -0000	1.24
+++ ethernet.h	8 Oct 2004 09:15:07 -0000
_at__at_ -353,7 +353,7 _at__at_
 
 extern	uint32_t ether_crc32_le(const uint8_t *, size_t);
 extern	uint32_t ether_crc32_be(const uint8_t *, size_t);
-extern	void ether_demux(struct ifnet *, struct mbuf *);
+extern	int  ether_demux(struct ifnet *, struct mbuf *);
 extern	void ether_ifattach(struct ifnet *, const u_int8_t *);
 extern	void ether_ifdetach(struct ifnet *);
 extern	int  ether_ioctl(struct ifnet *, int, caddr_t);
Index: if_ethersubr.c
===================================================================
RCS file: /home/ncvs/src/sys/net/if_ethersubr.c,v
retrieving revision 1.177
diff -u -u -r1.177 if_ethersubr.c
--- if_ethersubr.c	27 Jul 2004 23:20:45 -0000	1.177
+++ if_ethersubr.c	8 Oct 2004 09:16:49 -0000
_at__at_ -614,16 +614,14 _at__at_
 		}
 	}
 
-	ether_demux(ifp, m);
-	/* First chunk of an mbuf contains good entropy */
-	if (harvest.ethernet)
-		random_harvest(m, 16, 3, 0, RANDOM_NET);
+	if (ether_demux(ifp, m) == 0 && harvest.ethernet)
+		random_harvest(m->m_data, 16, 3, 0, RANDOM_NET);
 }
 
 /*
  * Upper layer processing for a received Ethernet packet.
  */
-void
+int
 ether_demux(struct ifnet *ifp, struct mbuf *m)
 {
 	struct ether_header *eh;
_at__at_ -666,14 +664,14 _at__at_
 		      IFP2AC(ifp)->ac_enaddr, ETHER_ADDR_LEN) != 0
 		    && (ifp->if_flags & IFF_PPROMISC) == 0) {
 			    m_freem(m);
-			    return;
+			    return (-1);
 		}
 	}
 
 	/* Discard packet if interface is not up */
 	if ((ifp->if_flags & IFF_UP) == 0) {
 		m_freem(m);
-		return;
+		return (-1);
 	}
 	if (ETHER_IS_MULTICAST(eh->ether_dhost)) {
 		if (bcmp(etherbroadcastaddr, eh->ether_dhost,
_at__at_ -691,7 +689,7 _at__at_
 		if (ether_ipfw_chk(&m, NULL, &rule, 0) == 0) {
 			if (m)
 				m_freem(m);
-			return;
+			return (-1);
 		}
 	}
 #endif
_at__at_ -709,7 +707,7 _at__at_
 		 */
 		KASSERT(vlan_input_p != NULL,("ether_input: VLAN not loaded!"));
 		(*vlan_input_p)(ifp, m);
-		return;
+		return (-1);
 	}
 
 	/*
_at__at_ -725,7 +723,7 _at__at_
 			ifp->if_noproto++;
 			m_freem(m);
 		}
-		return;
+		return (-1);
 	}
 
 	/* Strip off Ethernet header. */
_at__at_ -749,7 +747,7 _at__at_
 		if (ifp->if_flags & IFF_NOARP) {
 			/* Discard packet if ARP is disabled on interface */
 			m_freem(m);
-			return;
+			return (-1);
 		}
 		isr = NETISR_ARP;
 		break;
_at__at_ -805,7 +803,7 _at__at_
 		goto discard;
 	}
 	netisr_dispatch(isr, m);
-	return;
+	return 0;
 
 discard:
 	/*
_at__at_ -820,9 +818,10 _at__at_
 		 */
 		M_PREPEND(m, ETHER_HDR_LEN, M_DONTWAIT);
 		(*ng_ether_input_orphan_p)(ifp, m);
-		return;
+		return 0;
 	}
 	m_freem(m);
+	return (-1);
 }
 
 /*
--- patch ends here ---