[RFC] rtnetlink: handle multiple vlan tags in set_vf_vlan

[RFC] rtnetlink: handle multiple vlan tags in set_vf_vlan

[Stephen Hemminger]

The netlink API for IFLA_VF_VLAN_LIST allows multiple VLAN tags to
be passed (and the message was validated) but only the first VLAN
tag was being passed to the device. Change to iterate over each tag received.

Fixes: 79aab093a0b5 ("net: Update API for VF vlan protocol 802.1ad support")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 net/core/rtnetlink.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a51cab95ba64..3a9ec988ae21 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2207,11 +2207,10 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
 	if (tb[IFLA_VF_VLAN_LIST]) {
 		struct ifla_vf_vlan_info *ivvl[MAX_VLAN_LIST_LEN];
 		struct nlattr *attr;
-		int rem, len = 0;
+		int i, rem, len = 0;
 
-		err = -EOPNOTSUPP;
 		if (!ops->ndo_set_vf_vlan)
-			return err;
+			return -EOPNOTSUPP;
 
 		nla_for_each_nested(attr, tb[IFLA_VF_VLAN_LIST], rem) {
 			if (nla_type(attr) != IFLA_VF_VLAN_INFO ||
@@ -2224,13 +2223,15 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
 
 			len++;
 		}
-		if (len == 0)
-			return -EINVAL;
 
-		err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan,
-					   ivvl[0]->qos, ivvl[0]->vlan_proto);
-		if (err < 0)
-			return err;
+		err = -EINVAL; /* empty list error */
+		for (i = 0; i < len; i++) {
+			err = ops->ndo_set_vf_vlan(dev, ivvl[i]->vf,
+						   ivvl[i]->vlan, ivvl[i]->qos,
+						   ivvl[i]->vlan_proto);
+			if (err < 0)
+				return err;
+		}
 	}
 
 	if (tb[IFLA_VF_TX_RATE]) {
-- 
2.17.1

Re: [RFC] rtnetlink: handle multiple vlan tags in set_vf_vlan

[Stefano Brivio]

On Thu, 21 Feb 2019 09:54:36 -0800
Stephen Hemminger <stephen@networkplumber.org> wrote:

> @@ -2224,13 +2223,15 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
>  
>  			len++;
>  		}
> -		if (len == 0)
> -			return -EINVAL;
>  
> -		err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan,
> -					   ivvl[0]->qos, ivvl[0]->vlan_proto);
> -		if (err < 0)
> -			return err;
> +		err = -EINVAL; /* empty list error */
> +		for (i = 0; i < len; i++) {
> +			err = ops->ndo_set_vf_vlan(dev, ivvl[i]->vf,
> +						   ivvl[i]->vlan, ivvl[i]->qos,
> +						   ivvl[i]->vlan_proto);
> +			if (err < 0)
> +				return err;
> +		}

I think the:

	if (err < 0)
		return err;

should be outside the loop (with a "break;" inside), otherwise you won't
return anymore if len == 0.

-- 
Stefano

Re: [RFC] rtnetlink: handle multiple vlan tags in set_vf_vlan

[Stephen Hemminger]

On Thu, 21 Feb 2019 19:34:57 +0100
Stefano Brivio <sbrivio@redhat.com> wrote:

> On Thu, 21 Feb 2019 09:54:36 -0800
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> > @@ -2224,13 +2223,15 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
> >  
> >  			len++;
> >  		}
> > -		if (len == 0)
> > -			return -EINVAL;
> >  
> > -		err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan,
> > -					   ivvl[0]->qos, ivvl[0]->vlan_proto);
> > -		if (err < 0)
> > -			return err;
> > +		err = -EINVAL; /* empty list error */
> > +		for (i = 0; i < len; i++) {
> > +			err = ops->ndo_set_vf_vlan(dev, ivvl[i]->vf,
> > +						   ivvl[i]->vlan, ivvl[i]->qos,
> > +						   ivvl[i]->vlan_proto);
> > +			if (err < 0)
> > +				return err;
> > +		}  
> 
> I think the:
> 
> 	if (err < 0)
> 		return err;
> 
> should be outside the loop (with a "break;" inside), otherwise you won't
> return anymore if len == 0.
> 

Your right with empty list it would fall through and look at other attributes
which could overwrite err.

Re: [RFC] rtnetlink: handle multiple vlan tags in set_vf_vlan

[Moshe Shemesh]

On 2/21/2019 7:54 PM, Stephen Hemminger wrote:
> The netlink API for IFLA_VF_VLAN_LIST allows multiple VLAN tags to
> be passed (and the message was validated) but only the first VLAN
> tag was being passed to the device. Change to iterate over each tag received.
> 
> Fixes: 79aab093a0b5 ("net: Update API for VF vlan protocol 802.1ad support")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   net/core/rtnetlink.c | 19 ++++++++++---------
>   1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index a51cab95ba64..3a9ec988ae21 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2207,11 +2207,10 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
>   	if (tb[IFLA_VF_VLAN_LIST]) {
>   		struct ifla_vf_vlan_info *ivvl[MAX_VLAN_LIST_LEN];
>   		struct nlattr *attr;
> -		int rem, len = 0;
> +		int i, rem, len = 0;
>   
> -		err = -EOPNOTSUPP;
>   		if (!ops->ndo_set_vf_vlan)
> -			return err;
> +			return -EOPNOTSUPP;
>   
>   		nla_for_each_nested(attr, tb[IFLA_VF_VLAN_LIST], rem) {
>   			if (nla_type(attr) != IFLA_VF_VLAN_INFO ||
> @@ -2224,13 +2223,15 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
>   
>   			len++;
>   		}
> -		if (len == 0)
> -			return -EINVAL;
>   
> -		err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan,
> -					   ivvl[0]->qos, ivvl[0]->vlan_proto);
> -		if (err < 0)
> -			return err;
> +		err = -EINVAL; /* empty list error */
> +		for (i = 0; i < len; i++) {
> +			err = ops->ndo_set_vf_vlan(dev, ivvl[i]->vf,
> +						   ivvl[i]->vlan, ivvl[i]->qos,
> +						   ivvl[i]->vlan_proto);

Doing that each vlan will just overwrite the vf vlan configuration set 
by its preceding one.
Note #define MAX_VLAN_LIST_LEN 1
The point here was that I had to add the rtnl interface to set vf vlan 
with option to set vf vlan protocol. While doing that I was asked to add 
option to get a list of vlans from user to support QinQ, so once it will 
be needed there won't be a need to add another rtnl interface.

The driver which will support setting double vlan or more per vf will 
change MAX_VLAN_LIST_LEN and change the ndo function.

> +			if (err < 0)
> +				return err;
> +		}
>   	}
>   
>   	if (tb[IFLA_VF_TX_RATE]) {
> 

Re: [RFC] rtnetlink: handle multiple vlan tags in set_vf_vlan

[Stephen Hemminger]

On Fri, 22 Feb 2019 04:58:06 +0000
Moshe Shemesh <moshe@mellanox.com> wrote:

> On 2/21/2019 7:54 PM, Stephen Hemminger wrote:
> > The netlink API for IFLA_VF_VLAN_LIST allows multiple VLAN tags to
> > be passed (and the message was validated) but only the first VLAN
> > tag was being passed to the device. Change to iterate over each tag received.
> > 
> > Fixes: 79aab093a0b5 ("net: Update API for VF vlan protocol 802.1ad support")
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >   net/core/rtnetlink.c | 19 ++++++++++---------
> >   1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index a51cab95ba64..3a9ec988ae21 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -2207,11 +2207,10 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
> >   	if (tb[IFLA_VF_VLAN_LIST]) {
> >   		struct ifla_vf_vlan_info *ivvl[MAX_VLAN_LIST_LEN];
> >   		struct nlattr *attr;
> > -		int rem, len = 0;
> > +		int i, rem, len = 0;
> >   
> > -		err = -EOPNOTSUPP;
> >   		if (!ops->ndo_set_vf_vlan)
> > -			return err;
> > +			return -EOPNOTSUPP;
> >   
> >   		nla_for_each_nested(attr, tb[IFLA_VF_VLAN_LIST], rem) {
> >   			if (nla_type(attr) != IFLA_VF_VLAN_INFO ||
> > @@ -2224,13 +2223,15 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
> >   
> >   			len++;
> >   		}
> > -		if (len == 0)
> > -			return -EINVAL;
> >   
> > -		err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan,
> > -					   ivvl[0]->qos, ivvl[0]->vlan_proto);
> > -		if (err < 0)
> > -			return err;
> > +		err = -EINVAL; /* empty list error */
> > +		for (i = 0; i < len; i++) {
> > +			err = ops->ndo_set_vf_vlan(dev, ivvl[i]->vf,
> > +						   ivvl[i]->vlan, ivvl[i]->qos,
> > +						   ivvl[i]->vlan_proto);  
> 
> Doing that each vlan will just overwrite the vf vlan configuration set 
> by its preceding one.
> Note #define MAX_VLAN_LIST_LEN 1
> The point here was that I had to add the rtnl interface to set vf vlan 
> with option to set vf vlan protocol. While doing that I was asked to add 
> option to get a list of vlans from user to support QinQ, so once it will 
> be needed there won't be a need to add another rtnl interface.
> 
> The driver which will support setting double vlan or more per vf will 
> change MAX_VLAN_LIST_LEN and change the ndo function.
> 

OK, then the original code kind of makes sense now. Maybe the whole loop
could be eliminated.

I expected that the vlan list was for allowing multiple vlan's to go to
a single VF but it is really a future (unlikely to ever be implemented)
support for QinQ.