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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 26 Jun 2018 13:32:16 +0300
From:   Ido Schimmel <idosch@...sch.org>
To:     David Ahern <dsa@...ulusnetworks.com>
Cc:     Chas Williams <3chas3@...il.com>, davem@...emloft.net,
        netdev@...r.kernel.org, Roopa Prabhu <roopa@...ulusnetworks.com>,
        Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH v3,net-next] vlan: implement vlan id and protocol changes

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