Details

Reviewers

mjoras
glebius
melifaro

Group Reviewers

manpages
network

Commits

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

Summary

802.1ad interfaces are created with ifconfig using the "vlanproto" parameter.
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 vlan5 create vlandev em0 vlan 5 vlanproto 802.1ad up
ifconfig vlan42 create vlandev vlan5 vlan 42 inet 10.5.42.1/24

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).

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

freebsd_oprs.eu created this revision.Sep 15 2020, 1:38 PM

Herald added a reviewer: manpages. · View Herald TranscriptSep 15 2020, 1:39 PM

Herald added subscribers: melifaro, ae, imp. · View Herald Transcript

freebsd_oprs.eu requested review of this revision.Sep 15 2020, 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?

afedorov added a subscriber: afedorov.Sep 15 2020, 4:52 PM

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).

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 ?

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 !

olevole_olevole.ru added a subscriber: olevole_olevole.ru.Sep 16 2020, 12:47 PM

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?

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.

In D26436#588650, @freebsd_oprs.eu wrote:

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.

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 ?

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!

Small nit, otherwise LGTM.

sbin/ifconfig/ifconfig.8
2757	lowercase

In D26436#588741, @freebsd_oprs.eu wrote:

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
625–626	Is it done now? :-)
sys/net/if_vlan.c
247	Could you please consider briefly explaining why recursion is needed here?

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
2757	Oops. I missed that one, thanks.
sys/net/if_vlan.c
247	`_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.

In D26436#589646, @freebsd_oprs.eu wrote:

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.

This is mostly a cosmetic update. It fixes the small issues (typos, etc...) raised in the previous revision. The idea is to have a clean base to compare the (upcoming) alternative implementation to.

This updated revision implements the alternative solution discussed earlier. I've updated the test plan to account for the new parameter-based syntax, so I will not dwell on that here, but in a nutshell:

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

becomes

ifconfig vlan5 create vlandev em0 vlan 5 vlanproto 802.1ad up
ifconfig vlan42 create vlandev vlan5 vlan 42 inet 10.5.42.1/24

Dot notation is supported, as long as everyone agrees that any interface with a dot in its name is a VLAN.

This version is certainly closer to what people are used to elsewhere (Linux).

While the kernel part is now slightly simpler, there is still some amount of duplicate logic that I haven't been able to eliminate completely (ie. vlan_parse_ethervid() in ifconfig vs vlan_clone_match_ethervid() in the kernel).

freebsd_oprs.eu added inline comments.Sep 28 2020, 7:56 AM

share/man/man4/Makefile
527 ↗	(On Diff #77589)	note: this is due to a git rebase (not directly related to this revision).

I'll just slip in a small cosmetic update while I still can (MT_LAMBDA becomes MT_FILTER).

To summarize:

This patch adds support for stacked VLANs. It supports both 802.1Q over 802.1ad, and legacy 802.1Q over 802.1Q.
Since we now need to differentiate between IEEE 802.1Q "customer VLANs" and IEEE 802.1ad "service VLANs", two options were explored:
1. The OpenBSD-style svlan interface naming scheme, where a "svlan" prefix in the interface name denotes an IEEE 802.1ad VLAN, while a "vlan" prefix denotes an IEEE 802.1Q VLAN. This option was implemented in diff #77589.
2. A new vlanproto <vlan_proto> option to ifconfig, allowing users to explicitly set the VLAN encapsulation protocol on interface creation (possible values for vlan_proto: "802.1Q" and "802.1ad", case insensitive). Presumably what users from other obediences (i.e. the Linux crowd) may be most familiar with.

Both options are based on similar kernel-side mods and do behave well. Now the question is: which one shall we go with ?

Personal opinion: I have a marginal preference for option #1. The implementation is slightly more straightforward, and I like the fact that the choice, as presented to users, pertains more to the nature of the VLAN than to a mere property of it. This is just a gut feeling though. I'd be happy with either.

Thank you all for your time and comments.

In D26436#593961, @freebsd_oprs.eu wrote:

I'll just slip in a small cosmetic update while I still can (MT_LAMBDA becomes MT_FILTER).

To summarize:

This patch adds support for stacked VLANs. It supports both 802.1Q over 802.1ad, and legacy 802.1Q over 802.1Q.

Since we now need to differentiate between IEEE 802.1Q "customer VLANs" and IEEE 802.1ad "service VLANs", two options were explored:

The OpenBSD-style svlan interface naming scheme, where a "svlan" prefix in the interface name denotes an IEEE 802.1ad VLAN, while a "vlan" prefix denotes an IEEE 802.1Q VLAN. This option was implemented in diff #77589.

A new vlanproto <vlan_proto> option to ifconfig, allowing users to explicitly set the VLAN encapsulation protocol on interface creation (possible values for vlan_proto: "802.1Q" and "802.1ad", case insensitive). Presumably what users from other obediences (i.e. the Linux crowd) may be most familiar with.

Both options are based on similar kernel-side mods and do behave well. Now the question is: which one shall we go with ?

Personal opinion: I have a marginal preference for option #1. The implementation is slightly more straightforward, and I like the fact that the choice, as presented to users, pertains more to the nature of the VLAN than to a mere property of it. This is just a gut feeling though. I'd be happy with either.

Thank you all for your time and comments.

Thank you for working on this!

I would like to apologise for the late reply - somehow notifications slipped through cracks.
Thank you for updating and cleaning the patch, it looks pretty nice!

If you could add some basic tests, that would be awesome!
It should not be hard to re-use multipath tests in tests/netinet6/output6.sh , as they create 2 vnet jails, which would work perfectly for config / reachability tests.

Re naming: we all have different opinions and the use cases can be different. We support both vlan- and dot-notation for that reason :-)
Though, I'd still prefer to go with plan "vlan" approach for the following reasons:

I see the encapsulation type as an implementation detail and not the fundamental interface property. In the end, both vlan types are virtual broadcast domains.
One can still rename service vlan interfaces to "svlans" to add the desired notation in the name.

If no one objects, I'm going to commit it in a couple of days.

sbin/ifconfig/ifvlan.c
69	Nit: 802.1q for consistency?

This revision is now accepted and ready to land.Oct 17 2020, 2:37 PM

That's great news, thanks ! I'm currently catching up on Kyua & the ATF; I'll push some tests today.

This revision implements the test plan above as a proper set of ATF(7)/KYUA(1) test cases.
It also addresses the small nit in if_vlan.c (lowercase "802.1q" for consistency).

I'm perfectly fine with your arguments regarding the "vlan" approach. I like it as it is :-)

Cheers !

This revision now requires review to proceed.Oct 20 2020, 5:21 PM

melifaro added a reviewer: network.Oct 20 2020, 6:27 PM

I'm going to commit this tomorrow, October 21 unless there are any objections.

tests/sys/net/if_vlan.sh
54	Small nit: would it be possible to have the jail names start with the actual test name, so they are unique? Otherwise there is a possibility of multiple tests clashing over the same jail.

Ah, I should have anticipated that. It's done, thank you.

May I point you to D22076, which deals with vlan stacking, too.
And may I point you to D24179, which removes a show stopper for vlan stacking on Intel devices.

donner added inline comments.Oct 21 2020, 3:25 PM

sbin/ifconfig/ifvlan.c
227–228	May I kindly ask for 0xABCD notation for other stacking protocols in the wild, like 0x9100.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 21 2020, 9:28 PM

Closed by commit rS366917: Add support for stacked VLANs (IEEE 802.1ad, AKA Q-in-Q). (authored by melifaro). · Explain Why

This revision was automatically updated to reflect the committed changes.

melifaro added a commit: rS366917: Add support for stacked VLANs (IEEE 802.1ad, AKA Q-in-Q)..

peter_libassi.se mentioned this in D26137: Wireguard merge.Oct 25 2020, 2:22 PM

Hi Alexander,

Any vlan ending in .<N> is now treated the same regardless of network device:

Try this sequence of commands starting in any order
ifconfig vlan<N> create
ifconfig igb0.<N> create
ifconfig igb1.<N> create

You'll quickly see that only the first command you pick succeeds. The subsequent ones will fail. <N> must be the same number, for example 5.

I'm not sure exactly where the problem is yet, but investigation shows that ifc_name2unit would return EINVAL on igb0.<N> . Now it returns 0 (success) and puts <N> into the *unit argument.

diff --git a/sys/net/if_clone.c b/sys/net/if_clone.c
index a55ce9c3005..7f96757e12c 100644

a/sys/net/if_clone.c

+++ b/sys/net/if_clone.c
@@ -582,9 +582,8 @@ ifc_name2unit(const char *name, int *unit)
int             cutoff = INT_MAX / 10;
int             cutlim = INT_MAX % 10;
if ((cp = strrchr(name, '.')) == NULL)

cp = name;

for (; *cp != '\0' && (*cp < '0' || *cp > '9'); cp++);

+ for (cp = name; *cp != '\0' && (*cp < '0' || *cp > '9'); cp++)
+ ;
if (*cp == '\0') {
        *unit = -1;
} else if (cp[0] == '0' && cp[1] != '\0') {

The chunk above is not a fix. Can you have a look at this. Should be easy to reproduce!

Rolling back the kernel only to r366916 gives me:

ifconfig igb1.11 create
ifconfig: SIOCIFCREATE2: Invalid argument

Rolling back ifconfig to r366916 aswell, gives me the expected result again:

/usr/obj/usr/img/freebsd/amd64.amd64/sbin/ifconfig/ifconfig igb1.11 create
/usr/obj/usr/img/freebsd/amd64.amd64/sbin/ifconfig/ifconfig vlan11 create

--HPS

melifaro mentioned this in D27505: Fix subinterface vlan creation..Dec 7 2020, 11:55 PM

melifaro mentioned this in rG53729367d388: Fix subinterface vlan creation..Jan 29 2021, 9:43 PM

melifaro mentioned this in rG8efe1ef1c741: MFC 53729367d388: Fix subinterface vlan creation..Feb 4 2021, 10:35 PM

franco_opnsense.org added a subscriber: franco_opnsense.org.Apr 23 2021, 10:11 AM

donner mentioned this in D31871: Fix 802.1ad handling bug in input processing of ethernet frames..Sep 7 2021, 8:29 AM

bsd_kobyla.org added a subscriber: bsd_kobyla.org.Apr 25 2023, 7:20 AM

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 78509

sbin/ifconfig/ifclone.c

sbin/ifconfig/ifconfig.h

sbin/ifconfig/ifconfig.8

sbin/ifconfig/ifieee80211.c

sbin/ifconfig/ifvlan.c

sbin/ifconfig/ifvxlan.c

share/man/man4/vlan.4

sys/net/ethernet.h

sys/net/if_clone.c

sys/net/if_ethersubr.c

sys/net/if_vlan.c

sys/net/if_vlan_var.h

tests/sys/net/if_vlan.sh

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 78509

sbin/ifconfig/ifclone.c

sbin/ifconfig/ifconfig.h

sbin/ifconfig/ifconfig.8

sbin/ifconfig/ifieee80211.c

sbin/ifconfig/ifvlan.c

sbin/ifconfig/ifvxlan.c

share/man/man4/vlan.4

sys/net/ethernet.h

sys/net/if_clone.c

sys/net/if_ethersubr.c

sys/net/if_vlan.c

sys/net/if_vlan_var.h

tests/sys/net/if_vlan.sh

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

Revision Contents
Changeset List