bridge(4): allow member interface vlan to be configured

[ivy]

Summary

bridge(4): allow member interface vlan to be configured

This is the first part of bridge(4) VLAN filtering, and adds supports
for assigning ports to a VLAN.

ifconfig can now configure the PVID (native vlan id) of a member port:

        # ifconfig bridge0 ifpvid ix0 30

This changes the port's behaviour:

- any untagged incoming frame on a port which has a vlan set will
  inherit the vlan id of its interface, and go through the existing
  per-vlan route lookup machinery.

- if an tagged incoming frame does not patch the port's pvid, it will be
  dropped.

- outgoing frames will only be sent to such a port if the VLAN tag
  matches the port's pvid.

This allows multiple ports to be configured on a bridge with different
vlans; ports in the same vlan can communicate, while ports in different
vlans cannot.

This only supports untagged "access" ports; there is no support for
configuring tagged ports or adding tags to outgoing frames.

Update bridge.4 to document this change, and also add an overview of the
existing vlan/.1q support in if_bridge.

Basic tests for the new functionality are included.

Bump __FreeBSD_version for struct ibfreq ABI change.

[vova_fbsd.ru]

if an tagged incoming frame does not patch the port's pvid, it will be dropped.

should be read as "if an tagged incoming frame does not match the port's pvid, it will be dropped.", right?

Will patch support the case below -
igb0 - has either untagged frames and frames tagged as pvid 20

then I have two bridges - one for untagged packets, another one for tagged:

# ifconfig vlan20 create vlandev igb0 vlan 20
# ifconfig bridge0 addm igb0
# ifconfig brgide1 addm vlan20

before, that was not working

in theory, with ifpvid I would add the interface with vlan tag to second bridge, but, according to description for untagged frames - they will auto-tagged, which is not what I am expecting
sometghing like:

# ifconfig bridge0 addm igb0 ifpvid igb0 0    # allow only non-tagged frames from igb0 
# ifconfig brgide1 addm igb0 ifpvid igb0 20  # allow only tagged PVID=20 frames from igb0

my expectation here to have two bridges with different traffic (both untagged)

bridge0 -> untagged packets from igb0
bridge1 -> packets arrived tagged as PVID = 20 from igb0 but, then tag removed

[ivy]

In D49993#1140284, @vova_fbsd.ru wrote:

if an tagged incoming frame does not patch the port's pvid, it will be dropped.

should be read as "if an tagged incoming frame does not match the port's pvid, it will be dropped.", right?

yes.

# ifconfig vlan20 create vlandev igb0 vlan 20
# ifconfig bridge0 addm igb0
# ifconfig brgide1 addm vlan20

before, that was not working

this doesn't work because the bridge intercepts the frames before vlan(4) gets a chance to see them, and processes them with its internal VLAN logic. i do not intend to change this behaviour, but rather to make to this configuration unnecessary.

in theory, with ifpvid I would add the interface with vlan tag to second bridge, but, according to description for untagged frames - they will auto-tagged

the packet will not be tagged, it will just be treated by the bridge as if it was tagged, i.e. it will use that vlan's host table and it will be forwarded to member ports which are in that vlan. if the frame was received without a tag, and it is forwarded, it will be forwarded without a tag (and resp. if it was received with a tag to begin with, it will be forwarded with a tag).

# ifconfig bridge0 addm igb0 ifpvid igb0 0    # allow only non-tagged frames from igb0 
# ifconfig brgide1 addm igb0 ifpvid igb0 20  # allow only tagged PVID=20 frames from igb0

this won't work; you can't put an interface in two bridges and this patch doesn't change that.

my expectation here to have two bridges with different traffic (both untagged)

bridge0 -> untagged packets from igb0
bridge1 -> packets arrived tagged as PVID = 20 from igb0 but, then tag removed

eventually, you will be able to do this:

# ifconfig bridge0 addm igb0 ifpvid igb0 10 ifvlans igb0 20

this will accept tagged frames in VLAN 20, while placing untagged frames in VLAN 10. the idea is that you should only need a single bridge for all your interfaces and vlans, and the bridge will handle (de)tagging as appropriate as the frames pass through the bridge.

however all that is not part of this patch, this only adds the ifpvid option.

[vova_fbsd.ru]

thanks a lot for explaining,

this doesn't work because the bridge intercepts the frames before vlan(4) gets a chance to see them, and processes them with its internal VLAN logic. i do not intend to change this behaviour, but rather to make to this configuration unnecessary.

well, first of all such configuration sounds logical unless you know that bridge will not allow that,
probably, we should decline to add vlan interfaces into a bridge, as far as they will not work as expected?

the second ... different bridges as hubs for different groups of jail's or VM's - sounds very natural ... and each of these bridges should have frames without 802.1q tags
in approach you have proposed (when bridge handles properly tagged and untagged traffic):

• each jail/vm should be aware about proper tag when it connected to bridge
• each jail/vm should properly unwrap tagged traffic inside (so should be smart enough to know that it will get tagged packets) - that can be solved by enforcing vlan tag removal on some port

(please. correct me if I'm wrong in assumption)

in theory, I can introduce epair0, then epair0a to be added into one bridge, epair0b.20 to another bridge ... and this will not work again, right, due to the same reason.

so, probably the only way I can imagine how to do it right now - is to connect bridges via netgraph unwrapping tags ...

the bridge intercepts the frames before vlan(4) gets a chance to see them, and processes them

by the way ... when I think what sounds logical -

1. probably we should not intercept all interface packages if vlanX added into bridge (not main interface)
2. and probably, we should add a logic that will unwrap tagged packages when vlan interface added into a bridge ...

but I understand that this is not related to adding 802.1q functionality to bridge itself ...

[ivy]

In D49993#1140609, @vova_fbsd.ru wrote:

probably, we should decline to add vlan interfaces into a bridge, as far as they will not work as expected?

you can put vlan(4) interfaces in a bridge, as long as you don't also put the base interface in a bridge. this is a fairly common setup today. i'm not opposed to making this work with the base interface in a bridge as well, or just forbidding a vlan(4)'s base interface to be in a bridge, but that's largely orthogonal to this change (aside from that the purpose of bridge vlan filtering is to make that configuration unnecessary).

the second ... different bridges as hubs for different groups of jail's or VM's - sounds very natural ... and each of these bridges should have frames without 802.1q tags

you can keep doing that if you want, then you can ignore this change.

in approach you have proposed (when bridge handles properly tagged and untagged traffic):

• each jail/vm should be aware about proper tag when it connected to bridge
• each jail/vm should properly unwrap tagged traffic inside (so should be smart enough to know that it will get tagged packets) - that can be solved by enforcing vlan tag removal on some port

you can do it this way but you don't have to. the simplest approach, when each vm/jail is on a single vlan, would be something like:

# ifconfig bridge0 addm ix0 ifpvid 1 ifvlans 10,20
# ifconfig bridge0 addm tap0 ifpvid  10
# ifconfig bridge0 addm epair0a ifpvid 20

so ix0 is your trunk port to upstream switch/router, the vm tap0 is on vlan 10, and the jail epair0a is on vlan 20. once bridge handles (de)tagging correctly, this will be transparent to the jail/vm, which will just see untagged frames, while tagged frames will be sent on ix0.

in fact, if you want the vm/jail to do tagging itself, you can do this today by configuring it to send tagged frames. the trouble is a) you can't restrict what vlans it can access, and b) if you do that, every bridge port on those vlans has to work like that. that's why most people prefer using vlan(4) with one-bridge-per-vlan at the moment.

in theory, I can introduce epair0, then epair0a to be added into one bridge, epair0b.20 to another bridge ... and this will not work again, right, due to the same reason.

if you want to do this, you can just not use the base interface, i.e. all your traffic on that interface should be tagged. i would recommend this anyway, mixing tagged/untagged traffic on the same port is usually not desirable.

by the way ... when I think what sounds logical -

1. probably we should not intercept all interface packages if vlanX added into bridge (not main interface)

that would be a reasonable change.

2. and probably, we should add a logic that will unwrap tagged packages when vlan interface added into a bridge ...

this should already work; if you put a vlan(4) in a bridge, the bridge will only see untagged frames on that vlan, i.e. it has no idea a vlan is involved.

[vova_fbsd.ru]

Thank you for explanation ... now I better understand the "boundaries" of the problem -

what will work fine:

with two bridges:

# ifconfig create ix0.10
# ifconfig bridge10 addm ix0.10
# ifconfig create ix0.20
# ifconfig bridge20 addm ix0.20

with one bridge with tags: (after your patches)

# ifconfig bridge0 addm ix0 ifpvid 1 ifvlans 10,20
# ifconfig bridge0 addm tap0 ifpvid  10
# ifconfig bridge0 addm epair0a ifpvid 20

but, if I will want to use instead of tagged frames - non-tagged frames - this will not work, as whole interface (ix0) - assumed as "all frames tagged and untagged" and what is missed here - something which will represent "only untagged frames"
with your patches, I, probably, will be able to do (one bridge case):

# ifconfig bridge0 addm ix0 ifpvid 10 ifvlans 10,20
# ifconfig bridge0 addm tap0 ifpvid  10
# ifconfig bridge0 addm epair0a ifpvid 20

then on tap0 - I'll have what ever what arrived on ix0 as untagged,
but questions here:

• will be packets on tap0 untagged? (pvid set on bridge port, packets arrived from ix0 as untagged)
• will be packets on epair0 untagged? (pvid set on bridge port, packets arrived from ix0 as tagged)

and what will not work - two bridges case:
logically it should be something like

# ifconfig create ix0.0    // same as: ifconfig create vlan vlandev ix0 vlan 0 name ix0.0
# ifconfig bridge10 addm ix0.10
# ifconfig create ix0.20
# ifconfig bridge20 addm ix0.20

where ix0.0 to be represent "only untagged frames from ix0" (not the same as all just ix0 - "all frames")
but we do not support "vlan 0" as a way to say untagged packages after vlan dmux - sounds like "missed feature"

ng_vlan has similar capability (nomatch hook), but I will need to check also ether[12:2] != 0x8100 to make sure that other tagged packets not get there ...

[ivy]

In D49993#1141202, @vova_fbsd.ru wrote:

with two bridges:

# ifconfig create ix0.10
# ifconfig bridge10 addm ix0.10
# ifconfig create ix0.20
# ifconfig bridge20 addm ix0.20

right, this works today as long as you don't put ix0 itself in a bridge.

# ifconfig bridge0 addm ix0 ifpvid 1 ifvlans 10,20
# ifconfig bridge0 addm tap0 ifpvid  10
# ifconfig bridge0 addm epair0a ifpvid 20

but, if I will want to use instead of tagged frames - non-tagged frames - this will not work, as whole interface (ix0) - assumed as "all frames tagged and untagged" and what is missed here - something which will represent "only untagged frames"

in my example, pvid of ix0 is 1, so only frames sent to ix0 on vlan 1 would have the tag removed. instead you could do:

# ifconfig bridge0 addm0 ix0 ifpvid ix0 10 ifvlans ix0 20

now frames on vlan 10 will be sent without a tag, but frames in vlan 20 will have a tag.

# ifconfig bridge0 addm ix0 ifpvid 10 ifvlans 10,20
# ifconfig bridge0 addm tap0 ifpvid  10
# ifconfig bridge0 addm epair0a ifpvid 20

then on tap0 - I'll have what ever what arrived on ix0 as untagged,

yes, except tap0 will also have the tagged frames that arrived on ix0 with a tag for vlan 10, but the tag will be removed before sending it to tap0, since pvid of tap0 is 10.

• will be packets on tap0 untagged? (pvid set on bridge port, packets arrived from ix0 as untagged)
• will be packets on epair0 untagged? (pvid set on bridge port, packets arrived from ix0 as tagged)

with the code in this patch, whether a frame is sent with a tag depends on whether the incoming frame has a tag, i.e., the bridge will never add or remove a tag. this is the existing behaviour of bridge, the only difference is now frames whose vlan doesn't match the pvid will be dropped instead of being sent. note that the vlan of a frame is not necessarily the same as its tag, since ifpvid causes incoming untagged frames to be implicitly associated with a vlan.

with the full set of vlan filtering changes, when a frame is being sent on a port, and the frame's vlan matches the pvid of the port, the frame will always be sent untagged. otherwise, it will always be sent with a tag, if it's sent at all.

and what will not work - two bridges case:
logically it should be something like

# ifconfig create ix0.0    // same as: ifconfig create vlan vlandev ix0 vlan 0 name ix0.0
# ifconfig bridge10 addm ix0.10
# ifconfig create ix0.20
# ifconfig bridge20 addm ix0.20

right, this isn't possible (at least not using if_bridge alone) and i don't plan to fix this.

[zarychtam_plan-b.pwste.edu.pl]

Encouraged by the invitation on the freebsd-net@ mailing list, I was curious to try it out. However, the patch does not apply cleanly. Could you please publish a rebased version, if feasible?

cat /root/D49993.diff | git apply

error: patch failed: share/man/man4/bridge.4:36
error: share/man/man4/bridge.4: patch does not apply
error: patch failed: sys/net/if_bridge.c:335
error: sys/net/if_bridge.c: patch does not apply
error: patch failed: tests/sys/net/if_bridge_test.sh:718
error: tests/sys/net/if_bridge_test.sh: patch does not apply

[ivy]

rebase against freebsd/main

[zarychtam_plan-b.pwste.edu.pl]

Thank you! Now it applies fine.
I have given it a try and run a few tests between two bridged VNET jails. It works as described above. No observable regression, filtering works, and setting ifpvid 0 passes all the traffic, which is sometimes important.

The patch works fine with regard to filtering. I presumed it would add/strip vlan tags for frames belonging to native vlan set by new ifpvid option, but it doesn't happen, ie, the setting:

ifconfig vm-public addm epair1a ifpvid epair1a 0 addm epair2a ifpvid epair2a 100

adds filter for untagged frames on epair2, but to access VLAN 100, creating a child vlan(4) subinterface on epair2b is still required.
Perhaps it's how it's supposed to work, but I presumed that inside the jail, epari2b will be passing untagged frames from VLAN 100, not tagged ones from this VLAN.

[ivy]

In D49993#1149954, @zarychtam_plan-b.pwste.edu.pl wrote:

The patch works fine with regard to filtering. I presumed it would add/strip vlan tags for frames belonging to native vlan set by new ifpvid option, but it doesn't happen

right, this diff does not add any functionality that involves adding or removing tags. that will come in a later patch, this is only about the pvid functionality.

[kp]

tests/sys/net/if_bridge_test.sh

816

Is not setting ifpvid here deliberate?

857

It's not clear to me how this test case differs from vlan_pvid_tagged.

[ivy]

fix tests

[ivy]

tests/sys/net/if_bridge_test.sh

857

these are supposed to test that hosts can communicate using tagged packets on the same pvid, and cannot communicate using tagged packets if their pvid is wrong. not sure what went wrong here but both tests should make more sense now!

[kevans]

sys/net/if_bridgevar.h

144

I believe we should leave the padding alone in main. You would drop it in stable branches for KBI stability if you need to MFC it, but here you can just bump __FreeBSD_version and get the heartache over with now.

[ivy]

leave ifbrq::pad alone, and bump __FreeBSD_version instead.

[kevans]

Approved, but please amend the commit message to note that the bump is due to changing struct ifbreq to make it easier for others to pick out the KBI change.

[ivy]

i've updated the diff summary to include the current commit message from my local branch.

[p.mousavizadeh_protonmail.com]

With your changes, without VLAN filtering, can I assume that all interfaces in a bridge are treated as trunk ports?

I don't have access to the recent revisions of IEEE-802.1Q. However, based on observations in newer network devices, we should not accept received tagged packets on access ports, even if they have the same VLAN ID as our interface. All packets received on those interfaces should be untagged.

Even if we want to accept those, we should tag the packet on top of the existing packet. Why? 802.1Q tunneling (Q-in-Q) scenarios.

Did you consider this in your future changes? For consistency with other network devices, there should be an option to drop tagged packets on an access port or only tag on top of it.

[ivy]

In D49993#1152822, @p.mousavizadeh_protonmail.com wrote:

With your changes, without VLAN filtering, can I assume that all interfaces in a bridge are treated as trunk ports?

yes -- but that's only because this is already the case; bridge(4) is VLAN-aware and handles incoming tagged frames transparently. however it's not very useful since you can't do per-port VLAN restrictions, or add or remove tags.

I don't have access to the recent revisions of IEEE-802.1Q. However, based on observations in newer network devices, we should not accept received tagged packets on access ports, even if they have the same VLAN ID as our interface. All packets received on those interfaces should be untagged.

the original version of this patch had another port setting called “type”, which could be set to “access”, “trunk” or “hybrid” depending on what sort of frames we want to accept. i haven't included that in this version because i'm not sure it's useful, i.e., if the port pvid is 20, do we care whether the frame is untagged, or has a tag for vlan 20?

Even if we want to accept those, we should tag the packet on top of the existing packet. Why? 802.1Q tunneling (Q-in-Q) scenarios.

Did you consider this in your future changes? For consistency with other network devices, there should be an option to drop tagged packets on an access port or only tag on top of it.

i will have a think about this. QinQ is not on my immediate todo list, but we should handle these somehow, perhaps by dropping them to avoid frames jumping VLANs. i'm not sure if this information is stored in the mbuf, or perhaps we need to pull up the Ethernet header and see what the nested protocol is.

[p.mousavizadeh_protonmail.com]

Thank you for your quick response and working on this awesome feature.

the original version of this patch had another port setting called “type”, which could be set to “access”, “trunk” or “hybrid” depending on what sort of frames we want to accept. i haven't included that in this version because i'm not sure it's useful, i.e., if the port pvid is 20, do we care whether the frame is untagged, or has a tag for vlan 20?

We care, there are a lots of networks out there using double tagging (of course most of them are doing it incorrectly, but I have seen legitimate cases) and there are attacks like you mentioned: vlan hopping.

i will have a think about this. QinQ is not on my immediate todo list, but we should handle these somehow, perhaps by dropping them to avoid frames jumping VLANs. i'm not sure if this information is stored in the mbuf, or perhaps we need to pull up the Ethernet header and see what the nested protocol is.

So this review or D50500 would get an update by dropping the tagged packets in access ports before commit? If yes, let me know to apply "request change" in phabricator for that review.
Otherwise, there would be strong inconsistency in collaboration with other network devices and users could harm from VLAN Hopping attacks.

[ivy]

i don't want to make significant changes to the existing reviews if possible, so i'll add this as a new review in the stack and tag you as reviewer.

[p.mousavizadeh_protonmail.com]

In D49993#1152831, @ivy wrote:

i don't want to make significant changes to the existing reviews if possible, so i'll add this as a new review in the stack and tag you as reviewer.

At least we should inform the user about this behaviour in the manual.

Also, I think you should update the date of ifconfig manual too.

share/man/man4/bridge.4

269

At least we should inform the user about this behaviour in the manual:

An interface with a PVID configured will only receive frames for its
configured PVID, whether tagged or untagged.

[ivy]

based on feedback (here and in the original PR on GitHub) i am going to rework the syntax a bit for this feature.

firstly, there will now be an explicit flag to enable VLAN filtering on a port:

# ifconfig bridge0 vlanfilter ix0
# ifconfig bridge0 -vlanfilter ix0

if VLAN filtering is not enabled, then all the new code will be skipped for that port.

if VLAN filtering is enabled, then we do strict filtering:

• if the port's pvid is set to 0, never pass untagged frames on that port (except STP).
• if a tagged frame doesn't match the port's tagged access list, never pass the frame regardless of the port's pvid.

this behaviour is more explicit and more useful:

• you no longer need to set ifpvid to enable filtering
• removing the pvid or access list doesn't astonish the user by disabling filtering
• it's very clear whether filtering is enabled or not

at the same time i will rename the ifpvid and ifvlans options to untagged resp. tagged:

# ifconfig bridge0 untagged ix0 20 
# ifconfig bridge0 -untagged ix0
# ifconfig bridge0 tagged ix0 100-199
# ifconfig bridge0 -tagged ix0 150
# ifconfig bridge0 +tagged ix0 200

setting either option will automatically enable vlan filtering, to reduce the amount of redundant configuration required. however, removing vlan config will not automatically disable the flag.

in summary, this means we can support the 3 standard switchport types without needing the iftype option from the original filtering patch:

• trunk port: vlanfilter + tagged
• access port: vlanfilter + untagged
• hybrid port: vlanfilter + tagged + untagged

i'll update and rebase the stack for this once it's ready, probably today or tomorrow, but in the mean time i invite comments on this proposal.

[ivy]

update to new behaviour, per previous comment

[ivy]

share/man/man4/bridge.4

269

this should no longer be necessary since the behaviour has changed, and the language now reflects the new behaviour.

[allanjude]

The description says 'patched' (if an tagged incoming frame does not patch the port's pvid) where I think you meant 'matched'