Re: [PATCH v3,net-next] vlan: implement vlan id and protocol changes

On Tue, Jun 26, 2018 at 09:31:55AM -0400, Chas Williams wrote:

On Mon, Jun 25, 2018 at 4:45 PM David Ahern [off-list ref] wrote:

quoted

On 6/25/18 4:30 AM, Chas Williams wrote:

quoted

vlan_changelink silently ignores attempts to change the vlan id
or protocol id of an existing vlan interface.  Implement by adding
the new vlan id and protocol to the interface's vlan group and then
removing the old vlan id and protocol from the vlan group.

Signed-off-by: Chas Williams <3chas3@gmail.com>
---
 include/linux/netdevice.h |  1 +
 net/8021q/vlan.c          |  4 ++--
 net/8021q/vlan.h          |  2 ++
 net/8021q/vlan_netlink.c  | 38 ++++++++++++++++++++++++++++++++++++++
 net/core/dev.c            |  1 +
 5 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 3ec9850c7936..a95ae238addf 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h

@@ -2409,6 +2409,7 @@ enum netdev_cmd {
      NETDEV_CVLAN_FILTER_DROP_INFO,
      NETDEV_SVLAN_FILTER_PUSH_INFO,
      NETDEV_SVLAN_FILTER_DROP_INFO,
+     NETDEV_CHANGEVLAN,
 };
 const char *netdev_cmd_to_name(enum netdev_cmd cmd);

you add the new notifier, but do not add any hooks to catch and process it.

I can remove it.  I thought it would be prudent to add it now.
This could also really be NETDEV_CHANGE.  I wasn't sure
which would be more acceptable.

quoted

Personally, I think it is a bit sketchy to change the vlan id on an
existing device and I suspect it will cause latent errors.

It's not any different than changing any other layer 2 property on a device.
If you change the MTU or the MAC address, you are potentially going to
cause latent errors.

It is different in switch ASICs, at least. The MTU and MAC don't have
any state associated with them. The VLAN does.

For example, when you assign an IP address to a VLAN device configured
on top of an mlxsw port (e.g., swp1.10), then you are basically creating
a router interface (RIF) that is able to route packets. This RIF is
bound to the port and the VLAN {1, 10} which cannot be changed during
the lifetime of the RIF (at least w/o impacting traffic). The MAC and
the MTU can be easily changed and are changed following
NETDEV_CHANGEADDR and NETDEV_CHANGEMTU events.

Similar problems exist in bridged VLAN devices.

quoted

What's your use case for trying to implement the change versus causing
it to generate an unsupported error?

It's far more convenient to be able to change the VLAN ID and proto
instead of having to delete the link and put it back.  That's a lot of
churn (netlink mesages, kernel calls) for something relatively simple.

quoted

If this patch does get accepted, I believe the mlxsw switchdev driver
will be impacted.

How so?  It was relying on the fact that VLAN changes were ignored?

It is relying on existing kernel behavior which doesn't allow to change
the VLAN.

tl;dr - I'm still not convinced this is actually needed, but if you're
going to allow such behavior, then please also include a notification
that enables existing in-kernel users to refuse the operation.

Thanks