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 16:26:55 -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 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. -- 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