Refactor ip_fragment() and ip6_fragment(). Use a mbuf list to

simplify the handling of the fragment list.  Now the functions
ip_fragment() and ip6_fragment() always consume the mbuf.  They
free the mbuf and mbuf list in case of an error and take care about
the counter.  Adjust the code a bit to make v4 and v6 look similar.
Fixes a potential mbuf leak when pf_route6() called pf_refragment6()
and it failed.  Now the mbuf is always freed by ip6_fragment().
OK dlg@ mvs@

2 files changed, 40 insertions, 44 deletions

diff --git a/sys/netinet/ip_output.c b/sys/netinet/ip_output.c
index 74029056b70..cd28e534d79 100644
--- a/sys/netinet/ip_output.c
+++ b/sys/netinet/ip_output.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip_output.c,v 1.367 2021/02/23 12:14:10 bluhm Exp $	*/
+/*	$OpenBSD: ip_output.c,v 1.368 2021/03/01 11:05:42 bluhm Exp $	*/
 /*	$NetBSD: ip_output.c,v 1.28 1996/02/13 23:43:07 christos Exp $	*/
 
 /*
@@ -96,12 +96,12 @@ ip_output_ipsec_send(struct tdb *, struct mbuf *, struct route *, int);
  * The mbuf opt, if present, will not be freed.
  */
 int
-ip_output(struct mbuf *m0, struct mbuf *opt, struct route *ro, int flags,
+ip_output(struct mbuf *m, struct mbuf *opt, struct route *ro, int flags,
     struct ip_moptions *imo, struct inpcb *inp, u_int32_t ipsecflowinfo)
 {
 	struct ip *ip;
 	struct ifnet *ifp = NULL;
-	struct mbuf *m = m0;
+	struct mbuf_list fml;
 	int hlen = sizeof (struct ip);
 	int error = 0;
 	struct route iproute;
@@ -502,22 +502,18 @@ sendit:
 		goto bad;
 	}
 
-	error = ip_fragment(m, ifp, mtu);
-	if (error) {
-		m = m0 = NULL;
-		goto bad;
-	}
+	error = ip_fragment(m, &fml, ifp, mtu);
+	if (error)
+		goto done;
 
-	for (; m; m = m0) {
-		m0 = m->m_nextpkt;
-		m->m_nextpkt = NULL;
-		if (error == 0)
-			error = ifp->if_output(ifp, m, sintosa(dst), ro->ro_rt);
-		else
-			m_freem(m);
+	while ((m = ml_dequeue(&fml)) != NULL) {
+		error = ifp->if_output(ifp, m, sintosa(dst), ro->ro_rt);
+		if (error)
+			break;
 	}
-
-	if (error == 0)
+	if (error)
+		ml_purge(&fml);
+	else
 		ipstat_inc(ips_fragmented);
 
 done:
@@ -525,6 +521,7 @@ done:
 		rtfree(ro->ro_rt);
 	if_put(ifp);
 	return (error);
+
 bad:
 	m_freem(m);
 	goto done;
@@ -651,23 +648,24 @@ ip_output_ipsec_send(struct tdb *tdb, struct mbuf *m, struct route *ro, int fwd)
 #endif /* IPSEC */
 
 int
-ip_fragment(struct mbuf *m, struct ifnet *ifp, u_long mtu)
+ip_fragment(struct mbuf *m, struct mbuf_list *fml, struct ifnet *ifp,
+    u_long mtu)
 {
 	struct ip *ip, *mhip;
 	struct mbuf *m0;
 	int len, hlen, off;
 	int mhlen, firstlen;
-	struct mbuf **mnext;
-	int fragments = 0;
-	int error = 0;
+	int error;
+
+	ml_init(fml);
+	ml_enqueue(fml, m);
 
 	ip = mtod(m, struct ip *);
 	hlen = ip->ip_hl << 2;
-
 	len = (mtu - hlen) &~ 7;
 	if (len < 8) {
-		m_freem(m);
-		return (EMSGSIZE);
+		error = EMSGSIZE;
+		goto bad;
 	}
 
 	/*
@@ -676,7 +674,6 @@ ip_fragment(struct mbuf *m, struct ifnet *ifp, u_long mtu)
 	 */
 	in_proto_cksum_out(m, NULL);
 	firstlen = len;
-	mnext = &m->m_nextpkt;
 
 	/*
 	 * Loop through length of segment after first fragment,
@@ -687,12 +684,10 @@ ip_fragment(struct mbuf *m, struct ifnet *ifp, u_long mtu)
 	for (off = hlen + len; off < ntohs(ip->ip_len); off += len) {
 		MGETHDR(m, M_DONTWAIT, MT_HEADER);
 		if (m == NULL) {
-			ipstat_inc(ips_odropped);
 			error = ENOBUFS;
-			goto sendorfree;
+			goto bad;
 		}
-		*mnext = m;
-		mnext = &m->m_nextpkt;
+		ml_enqueue(fml, m);
 		m->m_data += max_linkhdr;
 		mhip = mtod(m, struct ip *);
 		*mhip = *ip;
@@ -715,10 +710,9 @@ ip_fragment(struct mbuf *m, struct ifnet *ifp, u_long mtu)
 			mhip->ip_off |= IP_MF;
 		mhip->ip_len = htons((u_int16_t)(len + mhlen));
 		m->m_next = m_copym(m0, off, len, M_NOWAIT);
-		if (m->m_next == 0) {
-			ipstat_inc(ips_odropped);
+		if (m->m_next == NULL) {
 			error = ENOBUFS;
-			goto sendorfree;
+			goto bad;
 		}
 		m->m_pkthdr.len = mhlen + len;
 		m->m_pkthdr.ph_ifidx = 0;
@@ -730,8 +724,6 @@ ip_fragment(struct mbuf *m, struct ifnet *ifp, u_long mtu)
 			ipstat_inc(ips_outswcsum);
 			mhip->ip_sum = in_cksum(m, mhlen);
 		}
-		ipstat_inc(ips_ofragments);
-		fragments++;
 	}
 	/*
 	 * Update first fragment by trimming what's been copied out
@@ -749,15 +741,13 @@ ip_fragment(struct mbuf *m, struct ifnet *ifp, u_long mtu)
 		ipstat_inc(ips_outswcsum);
 		ip->ip_sum = in_cksum(m, hlen);
 	}
-sendorfree:
-	if (error) {
-		for (m = m0; m; m = m0) {
-			m0 = m->m_nextpkt;
-			m->m_nextpkt = NULL;
-			m_freem(m);
-		}
-	}
 
+	ipstat_add(ips_ofragments, ml_len(fml));
+	return (0);
+
+bad:
+	ipstat_inc(ips_odropped);
+	ml_purge(fml);
 	return (error);
 }
 
diff --git a/sys/netinet/ip_var.h b/sys/netinet/ip_var.h
index b54e687110b..7ede24ce922 100644
--- a/sys/netinet/ip_var.h
+++ b/sys/netinet/ip_var.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip_var.h,v 1.86 2019/12/08 11:08:22 sashan Exp $	*/
+/*	$OpenBSD: ip_var.h,v 1.87 2021/03/01 11:05:42 bluhm Exp $	*/
 /*	$NetBSD: ip_var.h,v 1.16 1996/02/13 23:43:20 christos Exp $	*/
 
 /*
@@ -145,6 +145,12 @@ ipstat_inc(enum ipstat_counters c)
 	counters_inc(ipcounters, c);
 }
 
+static inline void
+ipstat_add(enum ipstat_counters c, uint64_t v)
+{
+	counters_add(ipcounters, c, v);
+}
+
 /*
  * Structure attached to inpcb.ip_moptions and
  * passed to ip_output when IP multicast options are in use.
@@ -218,7 +224,7 @@ struct inpcb;
 
 int	 ip_ctloutput(int, struct socket *, int, int, struct mbuf *);
 void	 ip_flush(void);
-int	 ip_fragment(struct mbuf *, struct ifnet *, u_long);
+int	 ip_fragment(struct mbuf *, struct mbuf_list *, struct ifnet *, u_long);
 void	 ip_freef(struct ipq *);
 void	 ip_freemoptions(struct ip_moptions *);
 int	 ip_getmoptions(int, struct ip_moptions *, struct mbuf *);