netdev - Re: switchdev and VLAN ranges

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]

Message-ID: <561AFB2C.2010603@cumulusnetworks.com>
Date:	Mon, 12 Oct 2015 02:13:32 +0200
From:	Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:	Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
	Jiri Pirko <jiri@...nulli.us>
Cc:	Elad Raz <eladr@...lanox.com>, Scott Feldman <sfeldma@...il.com>,
	netdev <netdev@...r.kernel.org>,
	"stephen@...workplumber.org" <stephen@...workplumber.org>,
	Florian Fainelli <f.fainelli@...il.com>,
	Andrew Lunn <andrew@...n.ch>,
	Ido Schimmel <idosch@...lanox.com>,
	Or Gerlitz <ogerlitz@...lanox.com>
Subject: Re: switchdev and VLAN ranges

On 10/12/2015 12:41 AM, Vivien Didelot wrote:
> On Oct. Sunday 11 (41) 09:12 AM, Jiri Pirko wrote:
>> Sat, Oct 10, 2015 at 12:36:26PM CEST, nikolay@...ulusnetworks.com wrote:
>>> On 10/10/2015 09:49 AM, Elad Raz wrote:
>>>>
>>>>> On Oct 10, 2015, at 2:30 AM, Vivien Didelot <vivien.didelot@...oirfairelinux.com> wrote:
>>>>>
>>>>> I have two concerns in mind:
>>>>>
>>>>> a) if we imagine that drivers like Rocker allocate memory in the prepare
>>>>> phase for each VID, preparing a range like 100-4000 would definitely not
>>>>> be recommended.
>>>>>
>>>>> b) imagine that you have two Linux bridges on a switch, one using the
>>>>> hardware VLAN 100. If you request the VLAN range 99-101 for the other
>>>>> bridge members, it is not possible for the driver to say "I can
>>>>> accelerate VLAN 99 and 101, but not 100". It must return OPNOTSUPP for
>>>>> the whole range.
>>>>
>>>> Another concern I have with vid_being..vid_end range is the “flags”. Where flags can be BRIDGE_VLAN_INFO_PVID.
>>>> There is no sense having more than one VLAN as a PVID.
>>>> This leave the HW vendor the choice which VLAN id they will use as the PVID.
>>>>
>>>
>>> iproute2 doesn't allow to do it but I can see that someone can actually make it
>>> so the flags for the range have it and it doesn't look correct. Perhaps we need
>>> something like the patch below to enforce this from kernel-side.
>>>
>>>
>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>>> index d78b4429505a..02b17b53e9a6 100644
>>> --- a/net/bridge/br_netlink.c
>>> +++ b/net/bridge/br_netlink.c
>>> @@ -524,6 +524,9 @@ static int br_afspec(struct net_bridge *br,
>>> 			if (vinfo_start)
>>> 				return -EINVAL;
>>> 			vinfo_start = vinfo;
>>> +			/* don't allow range of pvids */
>>> +			if (vinfo_start->flags & BRIDGE_VLAN_INFO_PVID)
>>> +				return -EINVAL;
>>> 			continue;
>>> 		}
>>>
>>
>> Looks correct to me. Could you please submit this properly? Thanks!
> 
> The above patch is correct, but we only solve part of the problem, since
> the range and bridge flags are exposed by switchdev_obj_port_vlan as is.
> 
> Thanks,
> -v
> 

Yes, the above fixes the bridge side. About the switchdev side it seems like it's
up to the switchdev driver to do the right thing in its switchdev_ops. I took a
quick look at DSA and it seems correct, the flag isn't saved and on dump request
the flags are generated so it shouldn't be possible to export multiple pvids.
But switchdev_port_br_afspec() seems problematic, in fact I don't even see a vlan
id check, i.e. ==0 || >= VLAN_N_MASK.
Of course, I might be totally off point as I'm not that familiar with switchdev and
it's very late. :-)
But maybe it needs something like:

diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 6e4a4f9ad927..3dd52a53867f 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -16,6 +16,7 @@
 #include <linux/notifier.h>
 #include <linux/netdevice.h>
 #include <linux/if_bridge.h>
+#include <linux/if_vlan.h>
 #include <linux/list.h>
 #include <net/ip_fib.h>
 #include <net/switchdev.h>
@@ -716,10 +717,14 @@ static int switchdev_port_br_afspec(struct net_device *dev,
 			return -EINVAL;
 		vinfo = nla_data(attr);
 		vlan.flags = vinfo->flags;
+		if (!vinfo->vid || vinfo->vid >= VLAN_VID_MASK)
+                        return -EINVAL;
 		if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN) {
 			if (vlan.vid_begin)
 				return -EINVAL;
 			vlan.vid_begin = vinfo->vid;
+			if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
+				return -EINVAL;
 		} else if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END) {
 			if (!vlan.vid_begin)
 				return -EINVAL;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html