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>] [day] [month] [year] [list]

Message-ID: <20180626155727.GA19066@splinter.mtl.com>
Date:   Tue, 26 Jun 2018 18:57:27 +0300
From:   Ido Schimmel <idosch@...lanox.com>
To:     Chas Williams <3chas3@...il.com>
Cc:     dsa@...ulusnetworks.com, "David S. Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Roopa Prabhu <roopa@...ulusnetworks.com>
Subject: 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 <dsa@...ulusnetworks.com> 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.
> >
> 
> 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.
> 
> 
> > 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.

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