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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 29 Dec 2014 22:01:37 -0800 From: roopa <roopa@...ulusnetworks.com> To: Scott Feldman <sfeldma@...il.com> CC: Netdev <netdev@...r.kernel.org>, shemminger@...tta.com, "vyasevic@...hat.com" <vyasevic@...hat.com>, Wilson Kok <wkok@...ulusnetworks.com> Subject: Re: [PATCH 1/6] bridge: add support to parse multiple vlan info attributes in IFLA_AF_SPEC On 12/29/14, 9:31 PM, Scott Feldman wrote: > On Mon, Dec 29, 2014 at 9:25 PM, roopa <roopa@...ulusnetworks.com> wrote: >> On 12/29/14, 4:26 PM, Scott Feldman wrote: >>> On Mon, Dec 29, 2014 at 4:07 PM, Scott Feldman <sfeldma@...il.com> wrote: >>>> On Mon, Dec 29, 2014 at 3:47 PM, Scott Feldman <sfeldma@...il.com> wrote: >>>>> On Mon, Dec 29, 2014 at 2:10 PM, roopa <roopa@...ulusnetworks.com> >>>>> wrote: >>>>>> On 12/29/14, 1:40 PM, Scott Feldman wrote: >>>>>>> On Mon, Dec 29, 2014 at 1:05 PM, <roopa@...ulusnetworks.com> wrote: >>>>>>>> From: Roopa Prabhu <roopa@...ulusnetworks.com> >>>>>>>> >>>>>>>> This patch changes bridge IFLA_AF_SPEC netlink attribute parser to >>>>>>>> look for more than one IFLA_BRIDGE_VLAN_INFO attribute. This allows >>>>>>>> userspace to pack more than one vlan in the setlink msg. >>>>>>>> >>>>>>>> Signed-off-by: Roopa Prabhu <roopa@...ulusnetworks.com> >>>>>>>> --- >>>>>>>> net/bridge/br_netlink.c | 18 +++++++++--------- >>>>>>>> 1 file changed, 9 insertions(+), 9 deletions(-) >>>>>>>> >>>>>>>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c >>>>>>>> index 9f5eb55..75971b1 100644 >>>>>>>> --- a/net/bridge/br_netlink.c >>>>>>>> +++ b/net/bridge/br_netlink.c >>>>>>>> @@ -230,18 +230,18 @@ static int br_afspec(struct net_bridge *br, >>>>>>>> struct nlattr *af_spec, >>>>>>>> int cmd) >>>>>>>> { >>>>>>>> - struct nlattr *tb[IFLA_BRIDGE_MAX+1]; >>>>>>>> + struct bridge_vlan_info *vinfo; >>>>>>>> int err = 0; >>>>>>>> + struct nlattr *attr; >>>>>>>> + int err = 0; >>>>>>>> + int rem; >>>>>>>> + u16 vid; >>>>>>>> >>>>>>>> - err = nla_parse_nested(tb, IFLA_BRIDGE_MAX, af_spec, >>>>>>>> ifla_br_policy); >>>>>>> Removing this call orphans ifla_br_policy...should ifla_br_policy be >>>>>>> removed? >>>>>> >>>>>> good question. Its a good place to see the type. In-fact userspace >>>>>> programs >>>>>> also copy the same policy to parse netlink attributes. hmmm.. >>>>>> I would like to keep it if it does not throw a warning. >>>>> I don't know what the policy (sorry, no pun intended) on leaving dead >>>>> code. I say remove it. >>>> You know, not using the policy seems like a step backwards, and maybe >>>> it suggests a problem with the attr packing. >>>> >>>> We had: >>>> >>>> ifla_br_policy >>>> IFLA_BRIDGE_FLAGS >>>> IFLA_BRIDGE_MODE >>>> IFLA_BRIDGE_VLAN_INFO >>>> >>>> This patch set makes it: >>>> >>>> ifla_br_policy >>>> IFLA_BRIDGE_FLAGS >>>> IFLA_BRIDGE_MODE >>>> IFLA_BRIDGE_VLAN_INFO >>>> IFLA_BRIDGE_VLAN_RANGE_INFO >>>> >>>> Which is fine, but now VLAN_INFO and VLAN_RANGE_INFO can be repeated. >>>> I think you want some nesting to clarify: >>>> >>>> ifla_br_policy >>>> IFLA_BRIDGE_FLAGS >>>> IFLA_BRIDGE_MODE >>>> IFLA_BRIDGE_VLAN_INFO >>>> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of >>>> IFLA_BRIDGE_VLAN_INFO >>>> IFLA_BRIDGE_VLAN_RANGE_INFO >>>> >>>> Now you can keep the policy for the top-level parsing, and loop only >>>> on the nested array VLAN_LIST_INFO. Actually, now you can use just >>>> RANGE_INFO in array and have: >>>> >>>> ifla_br_policy >>>> IFLA_BRIDGE_FLAGS >>>> IFLA_BRIDGE_MODE >>>> IFLA_BRIDGE_VLAN_INFO >>>> IFLA_BRIDGE_VLAN_LIST_INFO // nested array of >>>> IFLA_BRIDGE_VLAN_RANGE_INFO >>>> >>>> And use VLAN_RANGE_INFO for both ranges of vids as well as single >>>> vids. That'll simplify your filling algo in patch 5. >>> Hmmmm...do you even need VLAN_RANGE_INFO? How about just using >>> existing VLAN_INFO and add some more flags: >>> >>> #define BRIDGE_VLAN_INFO_RANGE_START (1<<3) >>> #define BRIDGE_VLAN_INFO_RANGE_END (1<<4) >>> >>> Now you can have: >>> >>> IFLA_BRIDGE_FLAGS >>> IFLA_BRIDGE_MODE >>> IFLA_BRIDGE_VLAN_INFO >>> IFLA_BRIDGE_VLAN_INFO_LIST // nested array of >>> IFLA_BRIDGE_VLAN_INFO >>> >>> Don't set START or END for single vids in list. >> >> ok. I was debating yesterday about introducing another nest. This looks >> good. >> My only reason to not use existing IFLA_BRIDGE_VLAN_INFO was to make sure it >> works for existing users. >> I see that in this case since IFLA_BRIDGE_VLAN_INFO_LIST is new, it will not >> affect existing users. >> >> But, i cant use IFLA_BRIDGE_VLAN_INFO (ie an attribute in ifla_br_policy) >> under IFLA_BRIDGE_VLAN_INFO_LIST ?. > I don't see why not. You're not going to parse the array with a > policy anyway (since it's an array). And attr INFO_LIST will be .type > = NLA_NESTED in ifla_br_policy. > > agree that it will work if everybody assumes that this is an array of just this attrtype. wasn't sure if it is a good practice to reuse an attribute from an upper nest. will work on v2. thanks for the review. -- 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