Page MenuHomeFreeBSD

Add support for stacked VLANs (IEEE 802.1ad, AKA Q-in-Q).
Needs ReviewPublic

Authored by freebsd_oprs.eu on Tue, Sep 15, 1:38 PM.

Details

Reviewers
mjoras
glebius
Group Reviewers
manpages
Summary

802.1ad interfaces are created using interface cloning ("svlan" prefix).
Eg., the following creates a 802.1Q VLAN (id #42) over a 802.1ad S-VLAN
(id #5) over a physical Ethernet interface (em0).

ifconfig svlan5 create vlandev em0 vlan 5 up
ifconfig vlan42 create vlandev svlan5 vlan 42 inet 10.5.42.1/24

Any combination of vlan and svlan interfaces is supported, as long as
VLAN IDs are unique among the children of the same interface
(ie. can't have both vlan #5 and svlan #5 with em0 as a parent).

VLAN_MTU, VLAN_HWCSUM and VLAN_TSO capabilities should be properly
supported. VLAN_HWTAGGING is only partially supported, as there is
currently no IFCAP_VLAN_* denoting the possibility to set the VLAN
EtherType to anything else than 0x8100 (802.1ad uses 0x88A8).

Sponsored by: RG Nets

Test Plan

Functional tests against OpenBSD and Linux in the following
configurations:

1. Simple IEEE 802.1Q VLANs

1.1. Create vlan2 as VLAN #2 (802.1Q) over Ethernet:

ifconfig vlan2 create vlandev em0 vlan 2 inet 10.0.2.1/24

1.2. Ping another machine on the same VLAN (eg. 10.0.2.2):

  • with default packet size,
  • with packet sizes up to 1480

Capture the traffic with tcpdump/wireshark, you shouldn't get any
IP fragmentation (assuming mtu 1500, and VLAN_MTU set on em0).

1.3. Make sure you can destroy vlan2:

ifconfig vlan2 destroy

2. Simple Q-in-Q setup

2.1. Create svlan5 (802.1ad) and vlan42 (802.1q) over em0:

ifconfig svlan5 create vlandev em0 vlan 5 up
ifconfig vlan42 create vlandev svlan5 vlan 42 inet 10.5.42.1/24

2.2. Ping another machine on the same VLAN stack (eg. 10.5.42.2):

  • with default packet size,
  • with packet sizes up to 1480

Fragmentation should be consistent with the hardware offloading
capabilities enabled on the physical interface.

2.3. Make sure you can destroy vlan42, then svlan5:

ifconfig vlan42 destroy
ifconfig svlan5 destroy

3. Deep Q-in-Q

3.1. Create svlan5, svlan6 and vlan42 as follows:

ifconfig svlan5 create vlandev em0 vlan 5 up
ifconfig svlan6 create vlandev svlan5 vlan 6 up
ifconfig vlan42 create vlandev svlan6 vlan 42 inet 10.6.42.1/24

3.2. Ping another machine on the same VLAN stack (eg. 10.6.42.2):

  • with default packet size,
  • with packet sizes up to 1480

(fragmentation: same as 2.2.)

3.3. Make sure you can destroy vlan42, then svlan6, then svlan5:

ifconfig vlan42 destroy
ifconfig svlan6 destroy
ifconfig svlan5 destroy

4. Nested legacy VLANs (802.1Q over 802.1Q)

4.1. Create vlan2 and vlan3 as follows:

ifconfig vlan2 create vlandev em0 vlan 2 up
ifconfig vlan3 create vlandev vlan2 vlan 3 inet 10.2.3.1/24

4.2. Ping another machine on the same VLAN stack (eg. 10.2.3.2):

  • with default packet size,
  • with packet sizes up to 1480

(fragmentation: same as 2.2.)

4.3. Make sure you can destroy both interfaces:

ifconfig vlan3 destroy
ifconfig vlan2 destroy

5. Use dot notation to stack an 802.1Q VLAN over an 802.1ad S-VLAN

5.1. Create svlan5 and svlan5.42 as follows:

ifconfig svlan5 create vlandev em0 vlan 5 up
ifconfig svlan5.42 create inet 10.5.42.1/24

5.2. Ping another machine on the same VLAN stack (eg. 10.5.42.2):

(same conditions as 2.2)

5.3. Make sure you can destroy svlan5.42, then svlan5:

ifconfig svlan5.42 destroy
ifconfig svlan5 destroy

6. Use dot notation to stack an 802.1Q VLAN over another 802.1Q VLAN

6.1. Create em0.2 and em0.2.3 as follows:

ifconfig em0.2 create up
ifconfig em0.2.3 create inet 10.2.3.1/24

6.2. Ping another machine on the same VLAN stack (eg. 10.2.3.2):

(same conditions as 2.2)

6.3. Make sure you can destroy both interfaces:

ifconfig em0.2.3 destroy
ifconfig em0.2 destroy

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

freebsd_oprs.eu requested review of this revision.Tue, Sep 15, 1:39 PM

Will there be an accompanying patch to tcpdump, to deal with stag / svlan headers - possibly of variable size - in the BPF code?

There should be eventually. However, unless the general consensus is that
this should be an absolute requirement, it is not on the top of my priority
list at this time (there are other things I'd like to go through first,
such as proper support for 802.1ad hardware tagging).

pi added a subscriber: pi.Tue, Sep 15, 5:18 PM

mandoc -Tlint reports:

mandoc: ifconfig.8:595:27: STYLE: no blank before trailing delimiter: Fl vlanhwtag,
mandoc: ifconfig.8:595:41: STYLE: no blank before trailing delimiter: Fl vlanhwfilter,

and: the ifconfig man page changes reference a svlan(4) man page, which is missing ?

freebsd_oprs.eu retitled this revision from Bring support for stacked VLANs (IEEE 802.1ad, AKA Q-in-Q). to Add support for stacked VLANs (IEEE 802.1ad, AKA Q-in-Q)..
freebsd_oprs.eu edited the summary of this revision. (Show Details)
  • Fix broken ifconfig.8 syntax.
  • Add svlan.4 and if_svlan.4 as symlinks to vlan.4.
  • Slight update to the revision title.

My mistake for not linting the man pages before submitting this revision.
Thanks for the heads-up !

Thank you for working on QinQ support!
Could you by chance clarify the reasoning on using svlan interface names instead of having another option to set vlan type in existing vlan interface?
Also: what is the supposed scheme of configuring qinq interfaces (c=44,s=1) and (c=44,s=2)?
Will (s)vlan1.44 and (s)vlan2.44 work?

freebsd_oprs.eu added a comment.EditedWed, Sep 16, 11:48 PM

Hi, thanks for considering this patch.

I stuck with the svlan prefix scheme for two reasons:

  • Family ties: this is essentially what OpenBSD does.
  • VLAN type segregation: vlan and svlan instances end up in separate interface groups, this plays well with ifconfig -G/-g options.

... but of course, this is open for debate.

Configuring QinQ interfaces (c=44,s=1) and (c=44,s=2) is perfectly fine: vlan and svlan instances only share the same VLAN ID space within the same parent interface.
So in your case, you have two different parent S-VLANs (#1 and #2), that's two different parent interfaces that you can populate as you like.

Create S-VLANs id #1 and #2

ifconfig svlan1 create vlandev em0 vlan 1 up
ifconfig svlan2 create vlandev em0 vlan 2 up

Create two instances of VLAN id #44, one inside each S-VLAN

ifconfig vlan144 create vlandev svlan1 vlan 44 inet 10.1.44.1/24
ifconfig vlan244 create vlandev svlan2 vlan 44 inet 10.2.44.1/24

This works just fine.

 Will (s)vlan1.44 and (s)vlan2.44 work?

This is an interesting point (assuming the question is about using the dot notation to create interface clones with ifconfig).

  • ifconfig vlan1.44 create creates a 802.1Q stack (ETHERTYPE_VLAN over ETHERTYPE_VLAN).
  • ifconfig svlan1.44 create creates a 802.1ad stack (ETHERTYPE_QINQ over ETHERTYPE_QINQ, not so useful in itself).

This is only logical (as per the vlan/svlan prefix rule), but admittedly it may seem deceptive to some users.
I would personally recommend against using the dot notation for stacked VLANs.

 Will (s)vlan1.44 and (s)vlan2.44 work?

This is an interesting point (assuming the question is about using the dot notation to create interface clones with ifconfig).

  • ifconfig vlan1.44 create creates a 802.1Q stack (ETHERTYPE_VLAN over ETHERTYPE_VLAN).
  • ifconfig svlan1.44 create creates a 802.1ad stack (ETHERTYPE_QINQ over ETHERTYPE_QINQ, not so useful in itself).

This is only logical (as per the vlan/svlan prefix rule), but admittedly it may seem deceptive to some users.
I would personally recommend against using the dot notation for stacked VLANs.

I would disagree with the notion that the dot notation is something to not recommend. Generally, humans are lazy, and if the dot notation is accepted, the expectation would be for the system to "do the right thing".

Unless I'm mistaken, 802.1ad over 802.1ad is undefined behavior; 802.1Q over 802.1Q has been used in the past by various switch vendors, and I would think that most switches will support this for legacy reasons.

Ideally, the dot notation should "do the right thing" and stack 802.1Q over 802.1ad (QinQ in the outer header, VLAN in the inner) by default for both "ifconfig vlan1.44 create" and "ifconfig svlan1.44 create" (or even "ifconfig em0.1.44"), since that is the current standard. For triple stacked, if should be QinQ, QinQ, VLAN...

If someone really wants to have a non-standard QinQ over QinQ, or VLAN over VLAN, have them use the non-dot notation with two separate config lines...

That would minimize the necessary config changes if the ifconfig is buried beneath layers of scripting, outside of the kernel. IMHO.

@rscheff I completely agree with your points. I probably should have given this more thought to begin with.
I will amend the patch accordingly later today.

Thank you.

freebsd_oprs.eu added a comment.EditedThu, Sep 17, 4:11 PM

Hmm.. parsing VLAN interface names is currently left to the kernel. It sounds like something ifconfig should handle instead, especially when non-trivial dot notation is involved (ie. ifconfig em0.x.y.z).
I mean, look at vlan_clone_create(), the first half of it is just about normalizing user input.

What do you guys think ? Am I getting ahead of myself here ?

freebsd_oprs.eu edited the test plan for this revision. (Show Details)

This updated revision improves support for dot notation.

One can't stack 802.1ad over 802.1ad "by accident" anymore, ie. ifconfig svlan1.44 create now creates a proper 802.1Q over 802.1ad VLAN stack. In fact, dot notation always adds a 802.1Q layer, no matter what.

While I wholefully agree that we should do "the right thing", trying to be too smart may be harmful. Let me illustrate that with a simple example:

ifconfig em0.5 create
ifconfig em0.5.42 create

The first command simply creates a new vlan instance (em0.5) as 802.1Q VLAN #5 over em0.
Now, assuming we push the "do the right thing" logic to the limit, should the second ifconfig command "promote" em0.5 from vlan (802.1Q) to svlan (802.1ad) before adding em0.5.42 on top of it ? It sounds like too much magic is going on there.

Here is my reasoning:

  • ifconfig is commonly used to alter the state of an existing interface, yet this would alter its type. What's more, this would alter the type of the parent interface. Quite an inference IMHO.
  • The added complexity to the code isn't negligible, and I'm not sure it's worth it (especially considering that this feature may be frustrating to some users).
  • Maybe the user genuinely wanted to stack 802.1Q over 802.1Q in the first place.

(or even "ifconfig em0.1.44")

I'm assuming the idea here is to create the whole VLAN stack in one go (with only em0 existing initially). In this instance, yes, the resulting VLAN stack should be 802.1Q over 802.1ad, since in that case the system is responsible for creating both em0.1 and em0.1.44.

However, as stated in a previous comment, I believe the mecanics of non-trivial interface creation (parsing dot notation, creating multiple interfaces and so on) should be left to ifconfig, even if that means issuing a few more ioctls() in the process. I'm generally not comfortable with the kernel directly handling more user input than it really has to.

I'd be happy to implement this in a separate patch, and keep this one within a reasonably small functional perimeter.

note: I've updated the test plan with a couple of dot notation related examples.

I'm ok with having the user interface heavy-lifting all done in ifconfig (and allowing a more simple kernel API, which doesn't strictly forbid "unusal" configurations.

E.g. in the iscsi target framework, all the config parsing and initial CHAP Authentication handling is done in userspace, and control to the then properly established socket is afterwards handed over to an in-kernel device driver. Which is IMHO a good idea to deal with things that could easily be wrongly formatted or directly influenced by botched configs to be dealt with outside the kernel.

Thanks!

gbe accepted this revision as: manpages.Sat, Sep 19, 10:45 AM
gbe added a subscriber: gbe.

Small nit, otherwise LGTM.

sbin/ifconfig/ifconfig.8
2762

lowercase

Hmm.. parsing VLAN interface names is currently left to the kernel. It sounds like something ifconfig should handle instead, especially when non-trivial dot notation is involved (ie. ifconfig em0.x.y.z).
I mean, look at vlan_clone_create(), the first half of it is just about normalizing user input.

What do you guys think ? Am I getting ahead of myself here ?

I'm all up for moving as much business logic to (lib)ifconfig as we can.
While em0.x.y.z interfaces can probably exists, it's certainly not the kernel task to create the whole chain, it needs to create only the last one.

Re "vlan"/"svlan" naming - I don't have extremely strong opinion on this, but it would be nice if we could still consider using the same name for both. "Svlan" looks a bit like low-level implementation detail somehow leaked to the upper layers.

For example, it can be implemented in the following fashion:
ifconfig vlan5 create vlan 5 vlandev em0 vlanproto 802.1ad <-- svlan
ifconfig vlan5.25 create vlan 5 vlandev vlan5 <-- 8021.q on top of 802.1ad
ifconfig em0.5 create vlanproto 802.1ad <-- svlan

Do you think it would be more confusing for users?

sys/net/ethernet.h
459

worth splitting into multiple lines?

sys/net/if_ethersubr.c
626

Is it done now? :-)

sys/net/if_vlan.c
248

Could you please consider briefly explaining why recursion is needed here?

freebsd_oprs.eu marked 2 inline comments as done.Sun, Sep 20, 6:18 PM

Re "vlan"/"svlan" naming - I don't have extremely strong opinion on this, but it would be nice if we could still consider using the same name for both. "Svlan" looks a bit like low-level implementation detail somehow leaked to the upper layers.

For example, it can be implemented in the following fashion:
ifconfig vlan5 create vlan 5 vlandev em0 vlanproto 802.1ad <-- svlan
ifconfig vlan5.25 create vlan 5 vlandev vlan5 <-- 8021.q on top of 802.1ad
ifconfig em0.5 create vlanproto 802.1ad <-- svlan

Do you think it would be more confusing for users?

Thanks for bringing up this issue again. This is really the user-facing part, so it does requires special care; in fact, I think it would be great if more people voiced their opinion about it.
As for me, I don't really have a strong opinion on this neither, I like both options. Yours definitely sounds more explicit though, so let's try that too.

I'll update this differential revision and create another one, implementing your proposed solution. This way we can easily compare both approaches, and hopefully decide which feels better/more natural from a user's standpoint.
I hope it's OK to do that, otherwise please let me know.

Thank you all for your feedback.

sbin/ifconfig/ifconfig.8
2762

Oops. I missed that one, thanks.

sys/net/if_vlan.c
248

_VLAN_SX_ID is global to the whole if_vlan driver, therefore any vlan/svlan instance can grab it. This is fine until you get to stacked VLANs, where it may end up being grabbed multiple times. For instance, consider the following excerpt from vlan_ioctl():

case SIOCGIFMEDIA:
        VLAN_SLOCK();                                          /* initial grab */
        if (TRUNK(ifv) != NULL) {
                p = PARENT(ifv);
                if_ref(p);
                error = (*p->if_ioctl)(p, SIOCGIFMEDIA, data); /* XXX recursive grab if p is also a VLAN */

Making _VLAN_SX_ID a recursive lock seemed like the most straightforward option to me.

Re "vlan"/"svlan" naming - I don't have extremely strong opinion on this, but it would be nice if we could still consider using the same name for both. "Svlan" looks a bit like low-level implementation detail somehow leaked to the upper layers.

For example, it can be implemented in the following fashion:
ifconfig vlan5 create vlan 5 vlandev em0 vlanproto 802.1ad <-- svlan
ifconfig vlan5.25 create vlan 5 vlandev vlan5 <-- 8021.q on top of 802.1ad
ifconfig em0.5 create vlanproto 802.1ad <-- svlan

Do you think it would be more confusing for users?

Thanks for bringing up this issue again. This is really the user-facing part, so it does requires special care; in fact, I think it would be great if more people voiced their opinion about it.
As for me, I don't really have a strong opinion on this neither, I like both options. Yours definitely sounds more explicit though, so let's try that too.

I'll update this differential revision and create another one, implementing your proposed solution. This way we can easily compare both approaches, and hopefully decide which feels better/more natural from a user's standpoint.
I hope it's OK to do that, otherwise please let me know.

I don't have major concerns on the kernel implementation part, it's not worth doing for the sake of comparison. I guess description of both approaches is enough, :-)
I would suggest considering looking into what UI other major players have implemented. For example, looking at Linux implementation from the OS side and Cisco/Juniper from the NOS side could potentially provide enough datapoints to make a data-driven decision on what's the preferred UI should be.

Thank you all for your feedback.

Thank you for working on this! This is certainly the long-awaited functionality that needs to exists in base.