Page MenuHomeFreeBSD

netmap: Address errors on memory free in netmap_generic
ClosedPublic

Authored by thj on Mar 15 2024, 12:00 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, May 12, 9:26 PM
Unknown Object (File)
Fri, May 10, 12:20 PM
Unknown Object (File)
Thu, May 9, 11:03 AM
Unknown Object (File)
Thu, May 9, 6:52 AM
Unknown Object (File)
Tue, Apr 30, 6:12 PM
Unknown Object (File)
Tue, Apr 30, 6:11 PM
Unknown Object (File)
Tue, Apr 30, 6:11 PM
Unknown Object (File)
Tue, Apr 30, 6:11 PM
Subscribers

Details

Summary

netmap_generic keeps a pool of mbufs for uses in 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.

Test Plan

I've tested this using tools/tools/netmap/bridge and vpp with a netmap adapter.
I put bulk traffic through the interface to exercise the memory pooling and
requests per second traffic to try and catch any timing issues that might occur
with well paced TCP.

I've run 2TB with iperf3 and wrk doing 20,000/rps for 12 hours. I didn't
encounter mbuf exhaustion or obvious leaks running these tests.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

thj requested review of this revision.Mar 15 2024, 12:00 PM
sys/dev/netmap/netmap_generic.c
439

There's still a narrow race here: during teardown of the netmap interface, a thread may have started calling the destructor before generic_netmap_unregister() sets it to NULL. Or is there something which prevents this?

  • - if the reference has gone after taking the lock don't free the mbuf
sys/dev/netmap/netmap_generic.c
439

I agree there is a race. I've updated but I think I need to also rearrange the free'ing path a little too.

  • - Don't remove the pool lock until we have removed all mbufs
vmaffione added inline comments.
sys/dev/netmap/netmap_generic.c
260

Can you add this to the SET_MBUF_DESCTRUCTOR macro, to make the code compile also under linux?

658

Same as above, this should go into the SET_MBUF_DESTRUCTOR macro

This revision now requires changes to proceed.Mar 18 2024, 6:57 PM
  • - use SET_MBUF_DESTRUCTOR to set ext_arg1
thj marked 2 inline comments as done.Mar 19 2024, 3:05 PM

Any additional tests with pkt-gen in TX or RX mode over iflib interfaces and virtio-net ones?
E.g. two VMs/machines connected back to back with one transmitting and the other receiving, to stress test it?
Setting the sys.dev.netmap.admode sysctl to 2 will force emulated netmap even if the interface has native support.

sys/dev/netmap/netmap_generic.c
439

Same here... We should define a GEN_TX_MBUF_NA() macro to hide the internals. Better names welcome

This revision now requires changes to proceed.Mar 19 2024, 4:46 PM
  • Add a macro for getting netmap adapter from ext_arg1

Any additional tests with pkt-gen in TX or RX mode over iflib interfaces and virtio-net ones?
E.g. two VMs/machines connected back to back with one transmitting and the other receiving, to stress test it?
Setting the sys.dev.netmap.admode sysctl to 2 will force emulated netmap even if the interface has native support.

I think tests are a good idea, one barrier to freebsd test suite tests so far has been that epair didn't work. I think we can now write tests to exercise stability of netmap

vmaffione added inline comments.
sys/dev/netmap/netmap_generic.c
259

This call sets the two ext fields to NULL, so we should be explicit and pass NULL to both arguments.
(See the other comment below).

682

What about the m_freem call in generic_netmap_tx_clean? Not used by freebsd, but change that for consistency?

sys/dev/netmap/netmap_kern.h
2403

I think it is more readable if we are explicit here: set ext_arg1 unconditionally to na and pass NULL when removing the notification event.
Also, since the name of this macro mentions the destructor, it looks better to have the fn as a first argument and na as the second.

This revision now requires changes to proceed.Mar 20 2024, 2:11 PM
thj marked 3 inline comments as done.
  • swap order of arguments for destructor
  • replace an m_freem with m_free

Last modification.
Thanks

sys/dev/netmap/netmap_generic.c
471

Access ext_arg1 using the GEN_TX_MBUF_NA macro.

This revision now requires changes to proceed.Mar 25 2024, 10:04 AM
  • access ext_arg1 via the accessor
thj marked an inline comment as done.Mar 25 2024, 2:37 PM
This revision is now accepted and ready to land.Mar 25 2024, 6:31 PM