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] [day] [month] [year] [list]
Message-ID: <E4CD12F19ABA0C4D8729E087A761DC3505DD1813@ORSMSX101.amr.corp.intel.com>
Date:	Tue, 30 Dec 2014 19:23:51 +0000
From:	"Arad, Ronen" <ronen.arad@...el.com>
To:	roopa <roopa@...ulusnetworks.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:	"shemminger@...tta.com" <shemminger@...tta.com>,
	"vyasevic@...hat.com" <vyasevic@...hat.com>,
	Wilson kok <wkok@...ulusnetworks.com>
Subject: RE: [PATCH 3/6] bridge: modify IFLA_AF_SPEC parser to parse
 IFLA_BRIDGE_VLAN_RANGE_INFO



>-----Original Message-----
>From: roopa [mailto:roopa@...ulusnetworks.com]
>Sent: Tuesday, December 30, 2014 4:18 PM
>To: Arad, Ronen
>Cc: netdev@...r.kernel.org; shemminger@...tta.com; vyasevic@...hat.com; Wilson
>kok
>Subject: Re: [PATCH 3/6] bridge: modify IFLA_AF_SPEC parser to parse
>IFLA_BRIDGE_VLAN_RANGE_INFO
>
>On 12/30/14, 12:40 AM, Arad, Ronen wrote:
>>
>>> -----Original Message-----
>>> From: netdev-owner@...r.kernel.org [mailto:netdev-owner@...r.kernel.org] On
>>> Behalf Of roopa@...ulusnetworks.com
>>> Sent: Monday, December 29, 2014 11:05 PM
>>> To: netdev@...r.kernel.org; shemminger@...tta.com; vyasevic@...hat.com
>>> Cc: Roopa Prabhu; Wilson kok
>>> Subject: [PATCH 3/6] bridge: modify IFLA_AF_SPEC parser to parse
>>> IFLA_BRIDGE_VLAN_RANGE_INFO
>>>
>>> From: Roopa Prabhu <roopa@...ulusnetworks.com>
>>>
>>> This patch modifies br_afspec to parse incoming IFLA_BRIDGE_VLAN_RANGE_INFO
>>>
>>> Signed-off-by: Wilson kok <wkok@...ulusnetworks.com>
>>> Signed-off-by: Roopa Prabhu <roopa@...ulusnetworks.com>
>>> ---
>>> net/bridge/br_netlink.c |   70 +++++++++++++++++++++++++++++++++-----------
>--
>>> -
>>> 1 file changed, 49 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>>> index e7d1fc0..4c47ba0 100644
>>> --- a/net/bridge/br_netlink.c
>>> +++ b/net/bridge/br_netlink.c
>>> @@ -227,48 +227,76 @@ static const struct nla_policy
>>> ifla_br_policy[IFLA_MAX+1] = {
>>> 			 .len = sizeof(struct bridge_vlan_range_info), },
>>> };
>>>
>>> +static int br_afspec_vlan_add(struct net_bridge *br,
>>> +			      struct net_bridge_port *p,
>>> +			      u16 vid, u16 flags)
>>> +{
>>> +	int err = 0;
>>> +
>>> +	if (p) {
>>> +		err = nbp_vlan_add(p, vid, flags);
>>> +		if (err)
>>> +			return err;
>>> +
>>> +		if (flags & BRIDGE_VLAN_INFO_MASTER)
>>> +			err = br_vlan_add(p->br, vid, flags);
>>> +	} else {
>>> +		err = br_vlan_add(br, vid, flags);
>>> +	}
>>> +
>>> +	return err;
>>> +}
>>> +
>>> +static void br_afspec_vlan_del(struct net_bridge *br,
>>> +			       struct net_bridge_port *p,
>>> +			       u16 vid, u16 flags)
>>> +{
>>> +	if (p) {
>>> +		nbp_vlan_delete(p, vid);
>>> +		if (flags & BRIDGE_VLAN_INFO_MASTER)
>>> +			br_vlan_delete(p->br, vid);
>>> +	} else {
>>> +		br_vlan_delete(br, vid);
>>> +	}
>>> +}
>>> +
>>> static int br_afspec(struct net_bridge *br,
>>> 		     struct net_bridge_port *p,
>>> 		     struct nlattr *af_spec,
>>> 		     int cmd)
>>> {
>>> -	struct bridge_vlan_info *vinfo;
>>> -	int err = 0;
>>> +	struct bridge_vlan_range_info *vinfo;
>>> 	struct nlattr *attr;
>>> 	int err = 0;
>>> 	int rem;
>>> 	u16 vid;
>>>
>>> 	nla_for_each_nested(attr, af_spec, rem) {
>>> -		if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO)
>>> +		if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO &&
>>> +		    nla_type(attr) != IFLA_BRIDGE_VLAN_RANGE_INFO)
>>> 			continue;
>>> -
>>> 		vinfo = nla_data(attr);
>>> -		if (!vinfo->vid || vinfo->vid >= VLAN_VID_MASK)
>>> +
>>> +		if (nla_type(attr) == IFLA_BRIDGE_VLAN_INFO)
>>> +			vinfo->vid_end = vinfo->vid;
>>> +
>>> +		if (!vinfo->vid || vinfo->vid >= VLAN_VID_MASK ||
>>> +		    vinfo->vid_end >= VLAN_VID_MASK ||
>>> +		    vinfo->vid > vinfo->vid_end)
>>> 			return -EINVAL;
>>>
>>> 		switch (cmd) {
>>> 		case RTM_SETLINK:
>>> -			if (p) {
>>> -				err = nbp_vlan_add(p, vinfo->vid, vinfo->flags);
>>> +			for (vid = vinfo->vid; vid <= vinfo->vid_end; vid++) {
>>> +				err = br_afspec_vlan_add(br, p, vid,
>>> +							 vinfo->flags);
>> vinfo->flags could have BRIDGE_VLAN_INFO_PVID set. It is really a port
>property and there could only be a single PVID for a port. The loop will make
>the port pvid set, in turn, to each vid in the range until it finally set to
>vid_end.
>> This could be avoided by turning off this flag for all but the last vid in
>range:
>>
>> 				err = br_afspec_vlan_addr(br, p, vid,
>>                                                      ((vid == vinfo-
>>vid_end)
>>                                                       ? vinfo->flags
>>                                                       : vinfo->flags &
>~BRIDGE_VLAN_INFO_PVID));
>> Another alternative could be to add explicit pvid field to
>bridge_vlan_range_info and disallow PVID flag.
>> This allows for setting the port pvid to any vid in the range.
>>
>> If both alternatives seem somewhat complex we could just disallow PVID flag
>in IFLA_BRIDGE_VLAN_RANGE_INFO and allow it only in IFLA_BRIDGE_VLAN_INFO.
>
>I believe it is a user error to set PVID flag on a range of vlans using
>IFLA_BRIDGE_VLAN_RANGE_INFO
>or for that matter using multiple IFLA_BRIDGE_VLAN_INFO with pvid flag
>set. Today, the last one probably sticks.
>I will check current behavior and mimic that or return EINVAL when
>multiple attributes come in with the PVID flag.
>   
I don't believe it was possible to have multiple IFLA_BRIDGE_VLAN_INFO before
the proposed patch. The presence of multiple IFLA_BRIDGE_VLAN_INFO and
IFLA_BRIDGE_VLAN_RANGE_INFO could arise from iproute2 that will take mixed list of vids and vid ranges as such:
# bridge vlan add dev NAME vid VLAN[-VLAN][,VLAN[-VLAN]]* [untagged] [self] \
    [master]
Non-consecutive VLANs will be represented by individual IFLA_BRIDGE_VLAN_INFO
While consecutive ranges will be represented by IFLA_BRIDGE_VLAN_RANGE_INFO.
It seems reasonable for iproute2 "bridge vlan" command to
only allow "pvid" keyword when a single vid is entered and forbid it when vid
range is entered. This will make the presence of PVID flag with multiple
IFLA_BRIDGE_VLAN_INFO or in IFLA_BRIDGE_VLAN_RANGE_INFO unlikely. Nevertheless,
br_setlink() should prevent multiple PVID setting using a single setlink
message. EINVAL seems better for this unlikely case.   
>
>
>>
>>
>>> 				if (err)
>>> 					break;
>>> -
>>> -				if (vinfo->flags & BRIDGE_VLAN_INFO_MASTER)
>>> -					err = br_vlan_add(p->br, vinfo->vid,
>>> -							  vinfo->flags);
>>> -			} else
>>> -				err = br_vlan_add(br, vinfo->vid, vinfo->flags);
>>> -
>>> +			}
>>> 			break;
>>> -
>>> 		case RTM_DELLINK:
>>> -			if (p) {
>>> -				nbp_vlan_delete(p, vinfo->vid);
>>> -				if (vinfo->flags & BRIDGE_VLAN_INFO_MASTER)
>>> -					br_vlan_delete(p->br, vinfo->vid);
>>> -			} else
>>> -				br_vlan_delete(br, vinfo->vid);
>>> +			for (vid = vinfo->vid; vid <= vinfo->vid_end; vid++)
>>> +				br_afspec_vlan_del(br, p, vid, vinfo->flags);
>>> 			break;
>>> 		}
>>> 	}
>>> --
>>> 1.7.10.4
>>>
>>> --
>>> 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

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ