Page MenuHomeFreeBSD

iflib: Fix several memory handling issues around iflib_encap()
ClosedPublic

Authored by gallatin on Fri, Aug 14, 12:52 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Sep 2, 9:33 AM
Unknown Object (File)
Mon, Aug 31, 12:37 PM
Unknown Object (File)
Sun, Aug 30, 11:25 PM
Unknown Object (File)
Sat, Aug 29, 11:19 PM
Unknown Object (File)
Sat, Aug 29, 10:07 AM
Unknown Object (File)
Fri, Aug 28, 8:19 PM
Unknown Object (File)
Thu, Aug 27, 10:18 PM
Unknown Object (File)
Wed, Aug 26, 7:11 PM

Details

Summary
  • Fixed memory leaks around m_dup() not freeing the original chain on failure. If we return ENOMEM, we are expected to have freed the chain, else the mbuf would be leaked. Also updated iflib_ether_pad() to follow the same structure.
  • In iflib_parse_header() o Fixed a bug where the ip/ip6 and th pointers may point into a freed chain after m_pullup. Those pointers must be reset to point into the new chain.

    o Eliminate ENXIO returns for non-TCP TSO sends (which would violate the mbuf ownership contract if they could happen). Since they cannot, I made them assertions instead.
  • in iflib_ether_pad(), return ENOMEM after freeing mbuf, so that mp_ring knows it is free. An ENOBUFS error will cause the mp_ring path to retain the mbuf and retry
  • in iflib_encap(): o Fix a leak when bus_dmamap_load_mbuf_sg() returns ENOMEM

    o Fix a use-after-free in the mp_ring path when a driver using ktls frees an mbuf and returns ENOBUFS via iflib_encap()

After this change:

mp_ring: ENOBUFS can be returned only when we run out of descriptors and the mbuf is retained

	 in that case.

simple_tx: iflib_encap() always consumes the mbuf, regardless of the return

Note that iflib_debugnet_transmit(), like simple_tx, expects that iflib_encap() always consumes
mbufs. This will be true after mp_ring is removed, and its such a rare special
case (panic dumps) that I don't think its worth fixing in the meantime.

Diff Detail

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

Event Timeline

@sumit.saxena_broadcom.com This fixes a bug around ktls handling with mp_ring.
@imp this fixes the bnxt panic you reported to me a few weeks ago

kbowling added inline comments.
sys/net/iflib.c
3562–3563

optionally set *m_head = NULL after this to make the transition explicit

This revision is now accepted and ready to land.Fri, Aug 14, 1:19 PM
sys/net/iflib.c
3562–3563

Done, thanks, will test and push with this change if there is no more feedback