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

[Ido Schimmel <idosch@idosch.org>]

On Mon, Jun 25, 2018 at 02:45:24PM -0600, David Ahern wrote:
> On 6/25/18 4:30 AM, Chas Williams wrote:
> > 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.
> 
> 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

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

Thanks for the Cc, David.