git: 73fdbfb91121 - main - netmap: Address errors on memory free in netmap_generic

The branch main has been updated by thj:

URL: https://cgit.FreeBSD.org/src/commit/?id=73fdbfb911215795c55c89870ebc5d9197bf2a23

commit 73fdbfb911215795c55c89870ebc5d9197bf2a23
Author:     Tom Jones <thj@FreeBSD.org>
AuthorDate: 2024-03-26 09:52:07 +0000
Commit:     Tom Jones <thj@FreeBSD.org>
CommitDate: 2024-03-26 09:55:55 +0000

    netmap: Address errors on memory free in netmap_generic
    
    netmap_generic keeps a pool of mbufs for handling transfers, these mbufs
    have an external buffer attached to them.
    
    If some cases other parts of the network stack can chain these mbufs,
    when this happens the normal pool destructor function can end up
    free'ing the pool mbufs twice:
    
    - A first time if a pool mbuf has been chained with another mbuf when
      its chain is freed
    - A second time when its entry in the pool is freed
    
    Additionally, if other parts of the stack demote a pool mbuf its
    interface reference will be cleared. In this case we deference a NULL
    pointer when trying to free the mbuf through the destructor. Store a
    reference to the adapter in ext_arg1 with the destructor callback so we
    can find the correct adapter when free'ing a pool mbuf.
    
    This change enables using netmap with epair interfaces.
    
    Reviewed By:    vmaffione
    MFC after:      1 week
    Relnotes:       yes
    Sponsored by:   The FreeBSD Foundation
    Differential Revision: https://reviews.freebsd.org/D44371
---
 sys/dev/netmap/netmap_generic.c | 29 ++++++++++++++++++++++-------
 sys/dev/netmap/netmap_kern.h    |  4 +++-
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/sys/dev/netmap/netmap_generic.c b/sys/dev/netmap/netmap_generic.c
index 83210ae755c7..9b1218043a88 100644
--- a/sys/dev/netmap/netmap_generic.c
+++ b/sys/dev/netmap/netmap_generic.c
@@ -256,7 +256,7 @@ generic_netmap_unregister(struct netmap_adapter *na)
 		 * TX event is consumed. */
 		mtx_lock_spin(&kring->tx_event_lock);
 		if (kring->tx_event) {
-			SET_MBUF_DESTRUCTOR(kring->tx_event, NULL);
+			SET_MBUF_DESTRUCTOR(kring->tx_event, NULL, NULL);
 		}
 		kring->tx_event = NULL;
 		mtx_unlock_spin(&kring->tx_event_lock);
@@ -271,16 +271,18 @@ generic_netmap_unregister(struct netmap_adapter *na)
 
 		for_each_tx_kring(r, kring, na) {
 			callout_drain(&kring->tx_event_callout);
-			mtx_destroy(&kring->tx_event_lock);
+
 			if (kring->tx_pool == NULL) {
 				continue;
 			}
 
 			for (i=0; i<na->num_tx_desc; i++) {
 				if (kring->tx_pool[i]) {
-					m_freem(kring->tx_pool[i]);
+					m_free(kring->tx_pool[i]);
+					kring->tx_pool[i] = NULL;
 				}
 			}
+			mtx_destroy(&kring->tx_event_lock);
 			nm_os_free(kring->tx_pool);
 			kring->tx_pool = NULL;
 		}
@@ -434,7 +436,7 @@ out:
 static void
 generic_mbuf_dtor(struct mbuf *m)
 {
-	struct netmap_adapter *na = NA(GEN_TX_MBUF_IFP(m));
+	struct netmap_adapter *na = GEN_TX_MBUF_NA(m);
 	struct netmap_kring *kring;
 	unsigned int r = MBUF_TXQ(m);
 	unsigned int r_orig = r;
@@ -458,6 +460,18 @@ generic_mbuf_dtor(struct mbuf *m)
 
 		kring = na->tx_rings[r];
 		mtx_lock_spin(&kring->tx_event_lock);
+
+		/*
+		 * The netmap destructor can be called between us getting the
+		 * reference and taking the lock, in that case the ring
+		 * reference won't be valid. The destructor will free this mbuf
+		 * so we can stop here.
+		 */
+		if (GEN_TX_MBUF_NA(m) == NULL) {
+			mtx_unlock_spin(&kring->tx_event_lock);
+			return;
+		}
+
 		if (kring->tx_event == m) {
 			kring->tx_event = NULL;
 			match = true;
@@ -525,7 +539,7 @@ generic_netmap_tx_clean(struct netmap_kring *kring, int txqdisc)
 				/* This mbuf has been dequeued but is still busy
 				 * (refcount is 2).
 				 * Leave it to the driver and replenish. */
-				m_freem(m);
+				m_free(m);
 				tx_pool[nm_i] = NULL;
 			}
 
@@ -638,7 +652,8 @@ generic_set_tx_event(struct netmap_kring *kring, u_int hwcur)
 		return;
 	}
 
-	SET_MBUF_DESTRUCTOR(m, generic_mbuf_dtor);
+	SET_MBUF_DESTRUCTOR(m, generic_mbuf_dtor, kring->na);
+
 	kring->tx_event = m;
 #ifdef __FreeBSD__
 	/*
@@ -664,7 +679,7 @@ generic_set_tx_event(struct netmap_kring *kring, u_int hwcur)
 
 	/* Decrement the refcount. This will free it if we lose the race
 	 * with the driver. */
-	m_freem(m);
+	m_free(m);
 }
 
 /*
diff --git a/sys/dev/netmap/netmap_kern.h b/sys/dev/netmap/netmap_kern.h
index 8618aaf82299..dd736b46ae70 100644
--- a/sys/dev/netmap/netmap_kern.h
+++ b/sys/dev/netmap/netmap_kern.h
@@ -102,6 +102,7 @@
 #define MBUF_TXQ(m)	((m)->m_pkthdr.flowid)
 #define MBUF_TRANSMIT(na, ifp, m)	((na)->if_transmit(ifp, m))
 #define	GEN_TX_MBUF_IFP(m)	((m)->m_pkthdr.rcvif)
+#define	GEN_TX_MBUF_NA(m)	((struct netmap_adapter *)(m)->m_ext.ext_arg1)
 
 #define NM_ATOMIC_T	volatile int /* required by atomic/bitops.h */
 /* atomic operations */
@@ -2395,9 +2396,10 @@ nm_generic_mbuf_dtor(struct mbuf *m)
 	uma_zfree(zone_clust, m->m_ext.ext_buf);
 }
 
-#define SET_MBUF_DESTRUCTOR(m, fn)	do {		\
+#define SET_MBUF_DESTRUCTOR(m, fn, na)	do {		\
 	(m)->m_ext.ext_free = (fn != NULL) ?		\
 	    (void *)fn : (void *)nm_generic_mbuf_dtor;	\
+	(m)->m_ext.ext_arg1 = na;			\
 } while (0)
 
 static inline struct mbuf *