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

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

[Chas Williams]

From: "Charles (Chas) Williams" <ciwillia@mail.eng.vyatta.net>

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 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 43 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);
 
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..0e59babe6651 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;
+	int err;
+	__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;
+
+		err = vlan_vid_add(real_dev, vlan_proto, vlan_id);
+		if (err)
+			return err;
+		vlan_info = rtnl_dereference(real_dev->vlan_info);
+		grp = &vlan_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]);
-- 
2.14.3

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2940 bytes --]

Hi Charles,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Chas-Williams/vlan-implement-vlan-id-and-protocol-changes/20180611-072123
config: parisc-c3000_defconfig (attached as .config)
compiler: hppa-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=parisc 

All warnings (new ones prefixed by >>):

   net/core/dev.c: In function 'netdev_cmd_to_name':
>> net/core/dev.c:1580:2: warning: enumeration value 'NETDEV_CHANGEVLAN' not handled in switch [-Wswitch]
     switch (cmd) {
     ^~~~~~

vim +/NETDEV_CHANGEVLAN +1580 net/core/dev.c

56f5aa77 Michael Chan 2017-12-16  1574  
ede2762d Kirill Tkhai 2018-03-23  1575  const char *netdev_cmd_to_name(enum netdev_cmd cmd)
ede2762d Kirill Tkhai 2018-03-23  1576  {
ede2762d Kirill Tkhai 2018-03-23  1577  #define N(val) 						\
ede2762d Kirill Tkhai 2018-03-23  1578  	case NETDEV_##val:				\
ede2762d Kirill Tkhai 2018-03-23  1579  		return "NETDEV_" __stringify(val);
ede2762d Kirill Tkhai 2018-03-23 @1580  	switch (cmd) {
ede2762d Kirill Tkhai 2018-03-23  1581  	N(UP) N(DOWN) N(REBOOT) N(CHANGE) N(REGISTER) N(UNREGISTER)
ede2762d Kirill Tkhai 2018-03-23  1582  	N(CHANGEMTU) N(CHANGEADDR) N(GOING_DOWN) N(CHANGENAME) N(FEAT_CHANGE)
ede2762d Kirill Tkhai 2018-03-23  1583  	N(BONDING_FAILOVER) N(PRE_UP) N(PRE_TYPE_CHANGE) N(POST_TYPE_CHANGE)
ede2762d Kirill Tkhai 2018-03-23  1584  	N(POST_INIT) N(RELEASE) N(NOTIFY_PEERS) N(JOIN) N(CHANGEUPPER)
ede2762d Kirill Tkhai 2018-03-23  1585  	N(RESEND_IGMP) N(PRECHANGEMTU) N(CHANGEINFODATA) N(BONDING_INFO)
ede2762d Kirill Tkhai 2018-03-23  1586  	N(PRECHANGEUPPER) N(CHANGELOWERSTATE) N(UDP_TUNNEL_PUSH_INFO)
ede2762d Kirill Tkhai 2018-03-23  1587  	N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN)
9daae9bd Gal Pressman 2018-03-28  1588  	N(CVLAN_FILTER_PUSH_INFO) N(CVLAN_FILTER_DROP_INFO)
9daae9bd Gal Pressman 2018-03-28  1589  	N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
3f5ecd8a Kirill Tkhai 2018-04-26  1590  	}
ede2762d Kirill Tkhai 2018-03-23  1591  #undef N
ede2762d Kirill Tkhai 2018-03-23  1592  	return "UNKNOWN_NETDEV_EVENT";
ede2762d Kirill Tkhai 2018-03-23  1593  }
ede2762d Kirill Tkhai 2018-03-23  1594  EXPORT_SYMBOL_GPL(netdev_cmd_to_name);
ede2762d Kirill Tkhai 2018-03-23  1595  

:::::: The code at line 1580 was first introduced by commit
:::::: ede2762d93ff16e0974f7446516b46b1022db213 net: Make NETDEV_XXX commands enum { }

:::::: TO: Kirill Tkhai <ktkhai@virtuozzo.com>
:::::: CC: David S. Miller <davem@davemloft.net>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 14440 bytes --]

[PATCH v2 net-next] vlan: implement vlan id and protocol changes

[Chas Williams]

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);
 
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..0e59babe6651 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;
+	int err;
+	__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;
+
+		err = vlan_vid_add(real_dev, vlan_proto, vlan_id);
+		if (err)
+			return err;
+		vlan_info = rtnl_dereference(real_dev->vlan_info);
+		grp = &vlan_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
index 6e18242a1cae..849fdb60fd21 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1587,6 +1587,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd)
 	N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN)
 	N(CVLAN_FILTER_PUSH_INFO) N(CVLAN_FILTER_DROP_INFO)
 	N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
+	N(CHANGEVLAN)
 	}
 #undef N
 	return "UNKNOWN_NETDEV_EVENT";
-- 
2.14.3

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

[Chas Williams]

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);
 
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 = &vlan_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
index a5aa1c7444e6..4bda5d2e3c85 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1587,6 +1587,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd)
 	N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN)
 	N(CVLAN_FILTER_PUSH_INFO) N(CVLAN_FILTER_DROP_INFO)
 	N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
+	N(CHANGEVLAN)
 	}
 #undef N
 	return "UNKNOWN_NETDEV_EVENT";
-- 
2.14.4

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

[David Ahern]

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.

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 = &vlan_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
> index a5aa1c7444e6..4bda5d2e3c85 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1587,6 +1587,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd)
>  	N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN)
>  	N(CVLAN_FILTER_PUSH_INFO) N(CVLAN_FILTER_DROP_INFO)
>  	N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
> +	N(CHANGEVLAN)
>  	}
>  #undef N
>  	return "UNKNOWN_NETDEV_EVENT";
> 

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

[Ido Schimmel]

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.

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

[Ido Schimmel]

On Tue, Jun 26, 2018 at 09:33:40AM -0400, Chas Williams wrote:
> On Tue, Jun 26, 2018 at 6:32 AM Ido Schimmel <idosch@idosch.org> 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 <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.
> >
> 
> 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

[Ido Schimmel]

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

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

[Chas Williams]

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.  This
avoids the netlink churn of deleting and re-adding an interface
to change the vlan id.

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  | 49 ++++++++++++++++++++++++++++++++++++++++++++++-
 net/core/dev.c            |  1 +
 5 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8bf8d6149f79..bf3f557eed8c 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2416,6 +2416,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);
 
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 99141986efa0..b6d0b2e2ada0 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..0a4ae6b15d89 100644
--- a/net/8021q/vlan_netlink.c
+++ b/net/8021q/vlan_netlink.c
@@ -107,10 +107,56 @@ 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;
+	int err = 0;
+	__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;
+
+		err = vlan_vid_add(real_dev, vlan_proto, vlan_id);
+		if (err)
+			goto out;
+		vlan_info = rtnl_dereference(real_dev->vlan_info);
+		grp = &vlan_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;
+
+		err = call_netdevice_notifiers(NETDEV_CHANGEVLAN, dev);
+		err = notifier_to_errno(err);
+		if (err) {
+			/* rollback */
+			vlan_group_set_device(grp, vlan_proto, vlan_id, NULL);
+			vlan_vid_del(real_dev, vlan_proto, vlan_id);
+			vlan->vlan_proto = old_vlan_proto;
+			vlan->vlan_id = old_vlan_id;
+		} else {
+			vlan_group_set_device(grp, old_vlan_proto,
+					      old_vlan_id, NULL);
+			vlan_vid_del(real_dev, old_vlan_proto, old_vlan_id);
+		}
+	}
 
 	if (data[IFLA_VLAN_FLAGS]) {
 		flags = nla_data(data[IFLA_VLAN_FLAGS]);
@@ -128,7 +174,8 @@ static int vlan_changelink(struct net_device *dev, struct nlattr *tb[],
 			vlan_dev_set_egress_priority(dev, m->from, m->to);
 		}
 	}
-	return 0;
+out:
+	return err;
 }
 
 static int vlan_newlink(struct net *src_net, struct net_device *dev,
diff --git a/net/core/dev.c b/net/core/dev.c
index 08d58e0debe5..63adc37e3548 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1587,6 +1587,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd)
 	N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN)
 	N(CVLAN_FILTER_PUSH_INFO) N(CVLAN_FILTER_DROP_INFO)
 	N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
+	N(CHANGEVLAN)
 	}
 #undef N
 	return "UNKNOWN_NETDEV_EVENT";
-- 
2.14.4

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

[David Miller]

From: Chas Williams <3chas3@gmail.com>
Date: Mon,  2 Jul 2018 18:35:28 -0400

> 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.  This
> avoids the netlink churn of deleting and re-adding an interface
> to change the vlan id.
> 
> Signed-off-by: Chas Williams <3chas3@gmail.com>

Chas, it seems to me that you add the new notifier by not even one
driver is listening for the event.

Either it is necessary, and you should show at least one example
use case, or it not necessary and therefore should not be added.

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

[Ido Schimmel]

On Sat, Jul 07, 2018 at 08:11:16PM +0900, David Miller wrote:
> Chas, it seems to me that you add the new notifier by not even one
> driver is listening for the event.
> 
> Either it is necessary, and you should show at least one example
> use case, or it not necessary and therefore should not be added.

Chas, I'll take a look and send you a patch for mlxsw that you can fold
into your v5.

In the future, please Cc those who commented on earlier versions of your
patch.

Thanks!

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

[David Ahern]

On 7/7/18 7:14 AM, Ido Schimmel wrote:
> On Sat, Jul 07, 2018 at 08:11:16PM +0900, David Miller wrote:
>> Chas, it seems to me that you add the new notifier by not even one
>> driver is listening for the event.
>>
>> Either it is necessary, and you should show at least one example
>> use case, or it not necessary and therefore should not be added.
> 
> Chas, I'll take a look and send you a patch for mlxsw that you can fold
> into your v5.
> 
> In the future, please Cc those who commented on earlier versions of your
> patch.

What about the impacts to vlan devices enslaved to bridges?

What about neighbor entries? Shouldn't entries associated with prior
device (vlan id) be removed?

I think in the end the idea of changing a vid or protocol for vlan
device is the wrong thing to allow. I don't buy your response last time
of "That's a lot of churn (netlink mesages, kernel calls) for something
relatively simple." It's not a simple change.

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

[David Miller]

From: David Ahern <dsahern@gmail.com>
Date: Sat, 7 Jul 2018 19:23:16 -0600

> On 7/7/18 7:14 AM, Ido Schimmel wrote:
>> On Sat, Jul 07, 2018 at 08:11:16PM +0900, David Miller wrote:
>>> Chas, it seems to me that you add the new notifier by not even one
>>> driver is listening for the event.
>>>
>>> Either it is necessary, and you should show at least one example
>>> use case, or it not necessary and therefore should not be added.
>> 
>> Chas, I'll take a look and send you a patch for mlxsw that you can fold
>> into your v5.
>> 
>> In the future, please Cc those who commented on earlier versions of your
>> patch.
> 
> What about the impacts to vlan devices enslaved to bridges?
> 
> What about neighbor entries? Shouldn't entries associated with prior
> device (vlan id) be removed?
> 
> I think in the end the idea of changing a vid or protocol for vlan
> device is the wrong thing to allow. I don't buy your response last time
> of "That's a lot of churn (netlink mesages, kernel calls) for something
> relatively simple." It's not a simple change.

I agree, there are so many things tied to the VLAN configuration.  And
this can propagate through one or more layers of stacked devices, some
of which have forwarding tables which which use the VLAN ID and/or
protocol as one of the primaries keys.

Changing the VLAN id and protocol is a huge operation with many far
reaching implications.