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 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 <3ch...@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.
>
>
> > 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
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 wrote:
>
> > 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 <3ch...@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.
> >
>
> 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.
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 <3ch...@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.
Re: [PATCH v3,net-next] vlan: implement vlan id and protocol changes
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 <3ch...@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.
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.
> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
> index 73a65789271b..b5e0ad1a581a 100644
> --- a/net/8021q/vlan.c
> +++ b/net/8021q/vlan.c
> @@ -51,8 +51,8 @@ const char vlan_version[] = DRV_VERSION;
>
> /* End of global variables definitions. */
>
> -static int vlan_group_prealloc_vid(struct vlan_group *vg,
> -__be16 vlan_proto, u16 vlan_id)
> +int vlan_group_prealloc_vid(struct vlan_group *vg,
> + __be16 vlan_proto, u16 vlan_id)
> {
> struct net_device **array;
> unsigned int pidx, vidx;
> diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h
> index 44df1c3df02d..c734dd21d70d 100644
> --- a/net/8021q/vlan.h
> +++ b/net/8021q/vlan.h
> @@ -116,6 +116,8 @@ int register_vlan_dev(struct net_device *dev, struct
> netlink_ext_ack *extack);
> void unregister_vlan_dev(struct net_device *dev, struct list_head *head);
> bool vlan_dev_inherit_address(struct net_device *dev,
> struct net_device *real_dev);
> +int vlan_group_prealloc_vid(struct vlan_group *vg,
> + __be16 vlan_proto, u16 vlan_id);
>
> static inline u32 vlan_get_ingress_priority(struct net_device *dev,
> u16 vlan_tci)
> diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c
> index 9b60c1e399e2..ee27781939e0 100644
> --- a/net/8021q/vlan_netlink.c
> +++ b/net/8021q/vlan_netlink.c
> @@ -107,10 +107,48 @@ static int vlan_changelink(struct net_device *dev,
> struct nlattr *tb[],
> struct nlattr *data[],
> struct netlink_ext_ack *extack)
> {
> + struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
> struct ifla_vlan_flags *flags;
> struct ifla_vlan_qos_mapping *m;
> struct nlattr *attr;
> int rem;
> + __be16 vlan_proto = vlan->vlan_proto;
> + u16 vlan_id = vlan->vlan_id;
> +
> + if (data[IFLA_VLAN_ID])
> + vlan_id = nla_get_u16(data[IFLA_VLAN_ID]);
> +
> + if (data[IFLA_VLAN_PROTOCOL])
> + vlan_proto = nla_get_be16(data[IFLA_VLAN_PROTOCOL]);
> +
> + if (vlan->vlan_id != vlan_id || vlan->vlan_proto != vlan_proto) {
> + struct net_device *real_dev = vlan->real_dev;
> + struct vlan_info *vlan_info;
> + struct vlan_group *grp;
> + __be16 old_vlan_proto = vlan->vlan_proto;
> + u16 old_vlan_id = vlan->vlan_id;
> + int err;
> +
> + err = vlan_vid_add(real_dev, vlan_proto, vlan_id);
> + if (err)
> + return err;
> + vlan_info = rtnl_dereference(real_dev->vlan_info);
> + grp = _info->grp;
> + err = vlan_group_prealloc_vid(grp, vlan_proto, vlan_id);
> + if (err < 0) {
> + vlan_vid_del(real_dev, vlan_proto, vlan_id);
> + return err;
> + }
> + vlan_group_set_device(grp, vlan_proto, vlan_id, dev);
> + vlan->vlan_proto = vlan_proto;
> + vlan->vlan_id = vlan_id;
> +
> + vlan_group_set_device(grp, old_vlan_proto, old_vlan_id, NULL);
> + vlan_vid_del(real_dev, old_vlan_proto, old_vlan_id);
> +
> + err = call_netdevice_notifiers(NETDEV_CHANGEVLAN, dev);
> + notifier_to_errno(err);
> + }
>
> if (data[IFLA_VLAN_FLAGS]) {
> flags = nla_data(data[IFLA_VLAN_FLAGS]);
> diff --git a/net/core/dev.c b/net/core/dev.c
