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

On Tue, Jun 26, 2018 at 09:33:40AM -0400, Chas Williams wrote:

On Tue, Jun 26, 2018 at 6:32 AM Ido Schimmel [off-list ref] wrote:

quoted

On Mon, Jun 25, 2018 at 02:45:24PM -0600, David Ahern 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.

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.

+1

quoted

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

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

Yes, at minimum we need to return an error for NETDEV_CHANGEVLAN, but
looking at the code it seems that there's no proper rollback.

I would prefer not to bother with error handling on the notification.  If
something misses the notification, something misses the notification.
It happens.

The notification is used so that relevant users in the kernel can
potentially veto the operation and refuse it. See other notifications
such as NETDEV_PRECHANGEUPPER.

The driver David mentioned is one existing user that needs to refuse the
VLAN change as it can't support it.