Page MenuHomeFreeBSD

Fix bridge_fragment()
ClosedPublic

Authored by olivier on Sep 4 2016, 2:33 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 23, 1:35 PM
Unknown Object (File)
Tue, Dec 10, 10:05 PM
Unknown Object (File)
Tue, Dec 10, 10:05 PM
Unknown Object (File)
Tue, Dec 10, 10:03 PM
Unknown Object (File)
Nov 22 2024, 11:51 PM
Unknown Object (File)
Nov 18 2024, 11:27 PM
Unknown Object (File)
Nov 14 2024, 2:57 PM
Unknown Object (File)
Nov 13 2024, 7:19 AM
Subscribers

Details

Summary

Fragmented UDP and ICMP packets were corrupted if a firewall with reassembling feature (like pf'scrub) is enabled on the bridge.
This patch fixes corrupted packet problem and the panic (triggered easly with low RAM) as explain in PR 185633.

bridge_pfil and bridge_fragment relationship:

  1. bridge_pfil() receive (IN direction) packets and sent it to the firewall
  2. The firewall can be configured for reassembling fragmented packet (like pf'scrubing) in one mbuf chain
  3. when bridge_pfil() need to send this reassembled packet to the outgoing interface, it needs to re-fragment it by using bridge_fragment()
  4. bridge_fragment() had to split this mbuf (using ip_fragment) first then had to M_PREPEND each packet in the mbuf chain for adding Ethernet header.
  5. But M_PREPEND can sometime create a new mbuf on the begining of the mbuf chain, then the "main" pointer of this mbuf chain should be updated and this case is tottaly forgotten.

The original bridge_fragment code (Revision 158140, 2006 April 29) came from OpenBSD, and the call to bridge_enqueue was embedded.
But on FreeBSD, bridge_enqueue() is done after bridge_fragment(), then the original OpenBSD code can't work as-it of FreeBSD.

Test Plan

Enable pf (scrub;pass) on a bridge and generate a fragmented ping (-s 2000) across the bridge.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5070
Build 5150: arc lint + arc unit

Event Timeline

olivier retitled this revision from to Fix bridge_fragment().
olivier updated this object.
olivier edited the test plan for this revision. (Show Details)
olivier added a reviewer: kp.

Just so we don't lose track of this: I think you've indeed identified the source of the problem with the bridge.

There's still a couple of problems with the error handling though.
m_freem() will not walk the m_nextpkt chain, so the error path leaks memory. Perhaps even worse is that it will repeatedly free m, which is also incorrect.
It also messes up (doesn't free anything) if the error occurs in the last packet of the m_nextpkt chain.

olivier edited edge metadata.

Rewrote the error handling part

kp added inline comments.
sys/net/if_bridge.c
3519

Doesn't this need to go before the if (snap) block? If we end up prepending an mbuf for the snap header but not for the ethernet header we still need to do the m_next/m_nextpkt pointer reshuffle.

3532

Won't this break in a (worst-case) scenario where both the snap and ethernet M_PREPEND()s end up adding a new mbuf?

3547

You don't need this check, the for loop has the same effect.

Take care of the snap M_PREPEND that end up adding a new mbuf and remove the useless test into the dropit:

olivier marked 3 inline comments as done.

Simplify the code for being more readable

Added style(9) fixes from @kristof.

Gnn suggested that Robert had ideas about this. Include him in the review.

This revision was automatically updated to reflect the committed changes.