Page MenuHomeFreeBSD

Wrap a vlan's parent's if_output in a separate function.
ClosedPublic

Authored by mjoras on Aug 10 2019, 7:44 PM.

Details

Summary

When a vlan interface is created, its if_output is set directly to the
parent interface's if_output. This is fine in the normal case but has an
unfortunate consequence if you end up with a certain combination of vlan
and lagg interfaces.

Consider you have a lagg interface with a single laggport member. When
an interface is added to a lagg its if_output is set to
lagg_port_output, which blackholes traffic from the normal networking
stack but not certain frames from BPF (pseudo_AF_HDRCMPLT). If you now
create a vlan with the laggport member (not the lagg interface) as its
parent, its if_output is set to lagg_port_output as well. While this is
confusing conceptually and likely represents a misconfigured system, it
is not itself a problem. The problem arises when you then remove the
lagg interface. Doing this resets the if_output of the laggport member
back to its original state, but the vlan's if_output is left pointing to
lagg_port_output. This gives rise to the possibility that the system
will panic when e.g. bpf is used to send any frames on the vlan
interface.

Fix this by creating a new function, vlan_output, which simply wraps the
parent's current if_output. That way when the parent's if_output is
restored there is no stale usage of lagg_port_output.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

If you know that the problem occurs only with BPF, bpfwrite() invokes if_output() already in epoch section.

In D21209#461161, @ae wrote:

If you know that the problem occurs only with BPF, bpfwrite() invokes if_output() already in epoch section.

That's a good point. It's the only place I know of a reported panic, but I think the panic can happen with any direct call to if_output. Probably not every such call is currently in an epoch section. Do we care about pointlessly refreshing the epoch section?

This revision is now accepted and ready to land.Aug 27 2019, 8:54 PM