bridge: Don't share broadcast packets

[kp]

Summary

if_bridge duplicates broadcast packets with m_copypacket(), which
creates shared packets. In certain circumstances these packets can be
processed by udp_usrreq.c:udp_input() first, which modifies the mbuf as
part of the checksum verification. That may lead to incorrect packets
being transmitted.

Use m_dup() to create independent mbufs instead.

Reported by: Richard Russo <[e-mail]>
MFC after: 2 weeks

[afedorov]

I'm concerned about two things:

1. Is this the only case with if_epair(4)? I don't see any guarantee that some other interface won't change the shared mbuf.

2. if_bridge.c::bridge_output uses m_copypacket() for broadcast only (multicast, unknown destination, span ports, etc.). After the fix, if_epair(4) calls m_unshare() for each packet, which can increase overhead since m_unshare() allocates memory.

I think it's very dangerous to use m_copypacket() and if_bridge(4) code should use m_dup().

[kp]

In D34319#777063, @afedorov wrote:

I'm concerned about two things:

1. Is this the only case with if_epair(4)? I don't see any guarantee that some other interface won't change the shared mbuf.

It's probably the most realistic way to manifest this issue, but you're right, there could be other case.

2. if_bridge.c::bridge_output uses m_copypacket() for broadcast only (multicast, unknown destination, span ports, etc.). After the fix, if_epair(4) calls m_unshare() for each packet, which can increase overhead since m_unshare() allocates memory.

I had somehow convinced myself that m_unshare() would only do work for shared mbufs, but that's not actually the case.

I think it's very dangerous to use m_copypacket() and if_bridge(4) code should use m_dup().

And you're also right that that's going to be a better fix. There's another m_copypacket() in the span path in if_bridge. I'll test a patch to replace them both with m_dup().

[donner]

Similar to ng_bridge, but there was hope that if_bridge could avoid such problems.

[afedorov]

Looks good to me.