Skip to content

Commit

Permalink
netmap: Address errors on memory free in netmap_generic
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Tom Jones authored and Tom Jones committed Mar 26, 2024
1 parent 39450eb commit 73fdbfb
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 8 deletions.
29 changes: 22 additions & 7 deletions sys/dev/netmap/netmap_generic.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
}
Expand Down Expand Up @@ -434,7 +436,7 @@ generic_netmap_register(struct netmap_adapter *na, int enable)
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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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__
/*
Expand All @@ -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);
}

/*
Expand Down
4 changes: 3 additions & 1 deletion sys/dev/netmap/netmap_kern.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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 *
Expand Down

0 comments on commit 73fdbfb

Please sign in to comment.