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 21:31:45 -0800 From: Scott Feldman <sfeldma@...il.com> To: roopa <roopa@...ulusnetworks.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 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. > > IFLA_BRIDGE_VLAN_INFO_LIST will have its own policy and its own attributes. > > Which will make it look something like below ? > > IFLA_BRIDGE_FLAGS > IFLA_BRIDGE_MODE > IFLA_BRIDGE_VLAN_INFO > IFLA_BRIDGE_VLAN_INFO_LIST // nested array of > IFLA_BRIDGE_VLAN_INFO_LIST_ENTRY > > > Thanks, > Roopa -- 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