Fix subinterface vlan creation.

[melifaro]

Summary

Overview

D26436 introduced support for stacked vlans that changed the way we configure vlans.
In particular, this change broke setups that have same-number vlans as subinterfaces.

Vlan naming details

Vlan support was initially created assuming "vlanX" semantics. In this paradigm, automatic number assignment supported by cloning (ifconfig vlan create) was a natural fit.

When "ifaceX.Y" support was added, allowing to have the same vlan number on multiple devices, cloning code became more complex, as the is no unified "vlan" namespace anymore. Such interfaces got the first spare index from "vlan" cloner. This, in turn, led to the following problem:

ifconfig ix0.333 create -> index 1
ifconfig ix0.444 create -> index 2
ifconfig vlan2 create -> allocation failure

Changes

This review targets the following:

• It restores name-based creation (2) to allow older binaries create subinterfaces as before. It was removed in D26436.
• It reverts ifc_name2unit() changes from D26436 and stops using ifc_name2unit() for sub-interfaces
• It stops using unit allocation code (ifc_alloc_unit()) for the sub-interfaces.

[• hselasky]

Test is OK:

# ifconfig mce4.4 create
# ifconfig mce5.4 create
# ifconfig vlan4 create
# ifconfig vlan4 vlandev mce4 vlan 4
ifconfig: SIOCSETVLAN: File exists
# ifconfig mce4.4 destroy
# ifconfig mce5.4 destroy
# ifconfig vlan4 vlandev mce4 vlan 4
# ifconfig vlan4 destroy

[• hselasky]

sys/net/if_clone.c

585

I think it is better to put the semicolon at the end at the next line.

[freebsd_oprs.eu]

Sorry for messing up some of the VLAN unit allocation logic. I have one suggestion though:

It removes an ability to create unnamed vlans ("ifconfig vlan create") as it doesn't make a lot of sense.

Do we really want to relinquish that ? Sometimes (ie. shell scripts, Kyua tests, etc...) it is useful to let the system assign interface names by itself, that would otherwise need to be hard coded.

ifname=$(ifconfig vlan create vlandev em0 vlan 123)
# ... do something with $ifname ...
ifconfig $ifname destroy

[melifaro]

In D27505#614852, @freebsd_oprs.eu wrote:

Sorry for messing up some of the VLAN unit allocation logic. I have one suggestion though:

It removes an ability to create unnamed vlans ("ifconfig vlan create") as it doesn't make a lot of sense.

Do we really want to relinquish that ? Sometimes (ie. shell scripts, Kyua tests, etc...) it is useful to let the system assign interface names by itself, that would otherwise need to be hard coded.

ifname=$(ifconfig vlan create vlandev em0 vlan 123)
# ... do something with $ifname ...
ifconfig $ifname destroy

I agree with the point in general - automatic interface name assignments does make a lot of sense and are really useful. Though, I'm a bit unsure about the vlan specifics as we have an explicit notation that "vlanX" corresponds to, well, vlan with id X.
In your example you're explicitly specifying interface name and vlan id. I'm not sure, why bother with dynamic allocation instead of explicit specification?

[rstone]

Removing the ability to create unnamed vlans is an unnecessary POLA violation. It can make sense for the user to establish a convention that vlanX is for vlan tag X, but we absolutely shouldn't bake that assumption into the kernel.

[• hselasky]

@melifaro : Did you have any more time to look at this? I guess we need to be backwards compatible and possible need to have a pool allocation for all interface names starting with "vlan" only? Other interface names are excluded from the unit management.

[adrian]

In D27505#615262, @hselasky wrote:

@melifaro : Did you have any more time to look at this? I guess we need to be backwards compatible and possible need to have a pool allocation for all interface names starting with "vlan" only? Other interface names are excluded from the unit management.

wlan can use the same, which is especially useful for doing automated wifi VAP testing scripts.

I'd really hate to see this functionality go away without some atomic-y userland-driven replacement for it :(

I also should go see how the dynamic interface numbering happens for wlan cloning for WDS/DWDS. That right now has /something/ create dynamic wlan interfaces when DWDS stations associate.

[melifaro]

Thank you for the feedback, folks!

My rationale for the removal was based on the tradeoff wrt conplexity vs functionality - proper implementation of a bit weird feature adds quite a bit of complexity.

I don’t have an extremely strong opinion on this and will reimplement the auto-numbering for vlan* interfaces.
However: do you folks think that this is something we should support forever?

[• hselasky]

Any updates here?

[• hselasky]

@melifaro : Any news about this issue?

13-current is closing soon!

[melifaro]

In D27505#630721, @hselasky wrote:

@melifaro : Any news about this issue?

13-current is closing soon!

Yep, I’ll get to this one before the start of the next week - currently trying to finalise routing changes.

[melifaro]

Update to address the comments.

[• hselasky]

Hi,

Can you please test the patch? I get bogus results:

ifconfig vlan0 create
vlan00

ifconfig mce4.2 create

  1913 ifconfig CALL  ioctl(0x3,SIOCIFCREATE2,0x7fffffffe110)
  1913 ifconfig RET   ioctl -1 errno 22 Invalid argument

[• hselasky]

sys/net/if_vlan.c

1018–1034

wildcard variable is uninitialized in this case!

[melifaro]

Fix the diff.

[• hselasky]

sys/net/if_vlan.c

1142

Variables should be declared at the top of the function.
Testing right now.

[• hselasky]

Still doesn't work like it should:

ifconfig mce4.3 create
ifconfig: SIOCIFCREATE2: Invalid argument

[melifaro]

In D27505#634281, @hselasky wrote:

Still doesn't work like it should:

ifconfig mce4.3 create
ifconfig: SIOCIFCREATE2: Invalid argument

Are you sure you're testing the last version?
I'm able to create similar interfaces:

ifconfig -l
em0.5 em0.6 vlan2 vlan2.3 em0.7 em1.2 em1.7

[melifaro]

Address style comments.

[• hselasky]

sys/net/if_vlan.c

1018–1034

Can you fix initialization of wildcard in the "p_tmp == NULL" case. I think that is what is causing it!

[• hselasky]

sys/net/if_vlan.c

1055

Put the semicolon on next line.

[• hselasky]

It is still not working. Trying to investigate why.

1) reboot
2) kldload mlx5en
3) ifconfig mceN.2 create # fails

--HPS

[• hselasky]

sys/net/if_vlan.c

1023

Need to set error = 0; here ! Else we always end up in the error case below.

[• hselasky]

Hi,

The following additional diff make things work. Can you integrate and test?

diff --git a/sys/net/if_vlan.c b/sys/net/if_vlan.c
index 503c6e6a9fd8..3c3dedda3080 100644
--- a/sys/net/if_vlan.c
+++ b/sys/net/if_vlan.c
@@ -977,8 +977,8 @@ static int
 vlan_clone_create(struct if_clone *ifc, char *name, size_t len, caddr_t params)
 {
        char *dp;
-       bool wildcard;
-       bool subinterface = false;
+       bool wildcard = false;
+       bool subinterface = true;
        int unit;
        int error;
        int vid = 0;
@@ -1019,25 +1019,20 @@ vlan_clone_create(struct if_clone *ifc, char *name, size_t len, caddr_t params)
 
        if ((error = ifc_name2unit(name, &unit)) == 0) {
                wildcard = (unit < 0);
+               subinterface = false;
        } else {
                struct ifnet *p_tmp = vlan_clone_match_ethervid(name, &vid);
                if (p_tmp != NULL) {
-                       subinterface = true;
-                       unit = IF_DUNIT_NONE;
-                       wildcard = false;
                        if (p != NULL) {
                                if_rele(p_tmp);
-                               if (p != p_tmp)
-                                       error = EINVAL;
-                       } else
+                               if (p != p_tmp) {
+                                       if_rele(p);
+                                       return (EINVAL);
+                               }
+                       } else
                                p = p_tmp;
                }
-       }
-
-       if (error != 0) {
-               if (p != NULL)
-                       if_rele(p);
-               return (error);
+               unit = IF_DUNIT_NONE;
        }
 
        if (!subinterface) {
@@ -1052,7 +1047,8 @@ vlan_clone_create(struct if_clone *ifc, char *name, size_t len, caddr_t params)
 
        /* In the wildcard case, we need to update the name. */
        if (wildcard) {
-               for (dp = name; *dp != '\0'; dp++);
+               for (dp = name; *dp != '\0'; dp++)
+                       ;
                if (snprintf(dp, len - (dp-name), "%d", unit) >
                    len - (dp-name) - 1) {
                        panic("%s: interface name too long", __func__);

[melifaro]

Zero error variable.

[melifaro]

In D27505#634605, @hselasky wrote:

Hi,

The following additional diff make things work. Can you integrate and test?

Unfortunately, this patch doesn't account for both ifc_name2unit() and vlan_clone_match_ethervid () returning NULL.

[• hselasky]

Let me test your latest patch tomorrow. Thank you!

[melifaro]

In D27505#634834, @hselasky wrote:

Let me test your latest patch tomorrow. Thank you!

Sorry for iterating back and forth and thank you for the patience!

[• hselasky]

Test OK!

[• hselasky]

sys/net/if_vlan.c

1037

This error case will only be used inside the "else" case of the previous "if" and can be moved there!