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:25:35 -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, 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 ?. 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