Re: MBuf leak in ether_output()

[Taylor R Campbell <campbell+netbsd-tech-net%mumble.net@localhost>]

> Date: Fri, 19 Apr 2024 13:06:46 +0200
> From: Edgar Fuß <ef%math.uni-bonn.de@localhost>
> 
> I'm still chasing down an MBuf leak and it looks like I've found the culprit.
> 
> static int
> ether_output(struct ifnet * const ifp0, struct mbuf * const m0,
> 	const struct sockaddr * const dst,
> 	const struct rtentry *rt)
> {
> [...]
> 	case AF_INET6:
> [...]
> 		} else {
> 			error = nd6_resolve(ifp, rt, m, dst, edst,
> 			    sizeof(edst));
> 			if (error != 0)
> 				return error == EWOULDBLOCK ? 0 : error;
> 			}
> 		}
> [...]
> }
> 
> In the EWOULDBLOCK case, 0 is returned, but who frees m?

If nd6_resolve returns nonzero, that means it has taken responsibility
for m.  Either it has freed m synchronously, or it will put m on a
queue to be freed later.  At least, that's the contract; of course
there may be bugs in its implementation of the contract.

The only path where nd6_resolve could return EWOULDBLOCK is via
nd_resolve:

   1572 int
   1573 nd6_resolve(struct ifnet *ifp, const struct rtentry *rt, struct mbuf *m,
   1574     const struct sockaddr *_dst, uint8_t *lldst, size_t dstsize)
...
   1584 		m_freem(m);
   1585 		return ENETDOWN; /* better error? */
...
   1603 		return 0;
...
   1623 			m_freem(m);
   1624 			return ENOBUFS;
...
   1630 		m_freem(m);
   1631 		return ENETDOWN; /* better error? */
...
   1634 	error = nd_resolve(ln, rt, m, lldst, dstsize);
   1635 
   1636 	if (created)
   1637 		nd6_gc_neighbors(LLTABLE6(ifp), &dst->sin6_addr);
   1638 
   1639 	return error;

https://nxr.netbsd.org/xref/src/sys/netinet6/nd6.c?r=1.282#1572


If nd_resolve can't resolve the link-layer address for the next hop
immediately, it puts the packet on a queue of packets with the same
next hop address, to be retransmitted once the link-layer address has
been resolved, and returns EWOULDBLOCK (unless the queue has
overflowed, in which case it does m_freem on oldest packets first):

    305 int
    306 nd_resolve(struct llentry *ln, const struct rtentry *rt, struct mbuf *m,
    307     uint8_t *lldst, size_t dstsize)
...
    339 	/*
    340 	 * If the neighbor cache entry has a state other than INCOMPLETE
    341 	 * (i.e. its link-layer address is already resolved), just
    342 	 * send the packet.
    343 	 */
    344 	if (ln->ln_state > ND_LLINFO_INCOMPLETE) {
...
    348 		return 0;
    349 	}
...
    365 	if (ln->ln_hold != NULL) {
    366 		struct mbuf *m_hold;
    367 		int i;
    368 
    369 		i = 0;
    370 		for (m_hold = ln->ln_hold; m_hold; m_hold = m_hold->m_nextpkt) {
    371 			i++;
    372 			if (m_hold->m_nextpkt == NULL) {
    373 				m_hold->m_nextpkt = m;
    374 				break;
    375 			}
    376 		}
    377 		while (i >= nd->nd_maxqueuelen) {
    378 			m_hold = ln->ln_hold;
    379 			ln->ln_hold = ln->ln_hold->m_nextpkt;
    380 			m_freem(m_hold);
    381 			i--;
    382 		}
    383 	} else
    384 		ln->ln_hold = m;
...
    386 	if (ln->ln_asked >= nd->nd_mmaxtries)
    387 		error = (rt != NULL && rt->rt_flags & RTF_GATEWAY) ?
    388 		    EHOSTUNREACH : EHOSTDOWN;
    389 	else
    390 		error = EWOULDBLOCK;
...
    412 	return error;

https://nxr.netbsd.org/xref/src/sys/net/nd.c?r=1.5#305


There are generally two possible outcomes:

1. The link-layer address is resolved.

   When this happens, nd6_llinfo_release_pkts is supposed to be called
   to retransmit all the packets (note: la_hold and ln_hold are the
   same member, thanks to a confusing #define at
   <https://nxr.netbsd.org/xref/src/sys/net/if_llatbl.h?r=1.19#104>):

      1295 void
      1296 nd6_llinfo_release_pkts(struct llentry *ln, struct ifnet *ifp)
      1297 {
   ...
      1305 	m_hold = ln->la_hold, ln->la_hold = NULL, ln->la_numheld = 0;
   ...
      1309 	for (; m_hold != NULL; m_hold = m_hold_next) {
      1310 		m_hold_next = m_hold->m_nextpkt;
      1311 		m_hold->m_nextpkt = NULL;
      1312 
      1313 		/*
      1314 		 * we assume ifp is not a p2p here, so
      1315 		 * just set the 2nd argument as the
      1316 		 * 1st one.
      1317 		 */
      1318 		ip6_if_output(ifp, ifp, m_hold, &sin6, NULL);

   https://nxr.netbsd.org/xref/src/sys/netinet6/nd6.c?r=1.282#1295

   ip6_if_output is presumably responsible for freeing m_hold.

2. The link-layer address resolution times out.

   When this happens, nd_timer just frees all the queued packets but
   one, and calls nd6_llinfo_missed (or arp_llinfo_missed) for the
   oldest queued packet:

        99 			m = ln->ln_hold;
       100 			for (m0 = m->m_nextpkt; m0 != NULL; m0 = mnxt) {
       101 				mnxt = m0->m_nextpkt;
       102 				m0->m_nextpkt = NULL;
       103 				m_freem(m0);
       104 			}
       105 
       106 			m->m_nextpkt = NULL;
       107 			ln->ln_hold = NULL;
   ...
       206 	if (missed != ND_LLINFO_NOSTATE)
       207 		nd->nd_missed(ifp, &taddr, missed, m);

   https://nxr.netbsd.org/xref/src/sys/net/nd.c?r=1.5#99

   nd6_llinfo_missed, in turn, winds up either in m_freem or
   ip6_output via icmp6_reflect via icmp6_error via icmp6_error2, and
   ip6_output is presumably responsible for freeing it when done.