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
| ||
|
Date: Mon, 29 Dec 2014 15:25:48 -0800 From: Scott Feldman <sfeldma@...il.com> To: Roopa Prabhu <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 5/6] bridge: new function to pack vlans using both IFLA_BRIDGE_VLAN_INFO and IFLA_BRIDGE_VLAN_RANGE_INFO On Mon, Dec 29, 2014 at 1:05 PM, <roopa@...ulusnetworks.com> wrote: > From: Roopa Prabhu <roopa@...ulusnetworks.com> > > This patch adds new function to compress vlans into ranges. > Vlans are compressed into ranges only if the fill request is called with > RTEXT_FILTER_BRVLAN_COMPRESSED in filtermask. > > Old vlan packing code is moved to a new function and continues to be > called when filter_mask is RTEXT_FILTER_BRVLAN > > Signed-off-by: Wilson kok <wkok@...ulusnetworks.com> > Signed-off-by: Roopa Prabhu <roopa@...ulusnetworks.com> > --- > net/bridge/br_netlink.c | 157 +++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 137 insertions(+), 20 deletions(-) > > diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c > index 4c47ba0..16bdd5a 100644 > --- a/net/bridge/br_netlink.c > +++ b/net/bridge/br_netlink.c > @@ -67,6 +67,133 @@ static int br_port_fill_attrs(struct sk_buff *skb, > return 0; > } > > +static int br_fill_ifvlaninfo_bitmap(struct sk_buff *skb, > + const unsigned long *vlan_bmp, u16 flags) > +{ > + struct bridge_vlan_range_info vinfo_range; > + struct bridge_vlan_info vinfo; > + u16 vid, start = 0, end = 0; One per line? > + u16 pvid; Aren't you getting an "unused" compile warning on this ^^^? > + > + /* handle the untagged */ This comment ^^^ doesn't make sense here. > + for_each_set_bit(vid, vlan_bmp, VLAN_N_VID) { > + if (start == 0) { > + start = vid; > + end = vid; > + } > + if ((vid - end) > 1) { > + memset(&vinfo_range, 0, sizeof(vinfo_range)); > + vinfo_range.flags |= flags; > + vinfo_range.vid = start; > + vinfo_range.vid_end = end; > + if (nla_put(skb, IFLA_BRIDGE_VLAN_RANGE_INFO, > + sizeof(vinfo_range), &vinfo_range)) > + goto nla_put_failure; > + start = vid; > + end = vid; > + } else { > + end = vid; > + } > + } What happens with this set {1-10, 12, 20-25, 30}? On vid 12, will that put a VLAN_RANGE_INFO with start=end=12? Seems strange to use RANGE_INFO for single vlan. Can the algorithm be simplified? Maybe there are other examples in the kernel of finding ranges/singles from a bitmap we could borrow code from? > + if (start != 0 && end != 0) { > + if (start != end) { > + memset(&vinfo_range, 0, sizeof(vinfo_range)); > + vinfo_range.flags |= flags; > + vinfo_range.vid = start; > + vinfo_range.vid_end = end; > + if (nla_put(skb, IFLA_BRIDGE_VLAN_RANGE_INFO, > + sizeof(vinfo_range), &vinfo_range)) > + goto nla_put_failure; > + } else { > + memset(&vinfo, 0, sizeof(vinfo)); > + vinfo.flags |= flags; > + vinfo.vid = start; > + if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO, > + sizeof(vinfo), &vinfo)) > + goto nla_put_failure; How many IFLA_BRIDGE_VLAN_INFOs can you fit into skb? It seems the worst case is you'll have 4094/2 = 2047 VLAN_INFOs if using all even-numbered vids, for example. Will that fit? I guess the original code had the same concern...wonder if anyone checked this worst-case? > + } > + } > + > +nla_put_failure: > + return -EMSGSIZE; > +} > + > +static int br_fill_ifvlaninfo_compressed(struct sk_buff *skb, > + const struct net_port_vlans *pv) > +{ > + unsigned long vlan_bmp_copy[BR_VLAN_BITMAP_LEN]; > + unsigned long untagged_bmp_copy[BR_VLAN_BITMAP_LEN]; Lots of automatic space on the stack...can you use dynamic mem (kzalloc) for these? > + struct bridge_vlan_range_info vinfo_range; > + struct bridge_vlan_info vinfo; > + u16 pvid; > + > + memset(vlan_bmp_copy, 0, > + sizeof(unsigned long) * BR_VLAN_BITMAP_LEN); > + bitmap_copy(vlan_bmp_copy, pv->vlan_bitmap, VLAN_N_VID); > + > + memset(untagged_bmp_copy, 0, > + sizeof(unsigned long) * BR_VLAN_BITMAP_LEN); > + bitmap_copy(untagged_bmp_copy, pv->untagged_bitmap, VLAN_N_VID); > + > + /* send the pvid separately first */ > + pvid = br_get_pvid(pv); > + if (pvid && (pvid != VLAN_N_VID)) { > + memset(&vinfo, 0, sizeof(vinfo)); > + vinfo.flags |= BRIDGE_VLAN_INFO_PVID; > + if (test_bit(pvid, untagged_bmp_copy)) { > + vinfo.flags |= BRIDGE_VLAN_INFO_UNTAGGED; > + clear_bit(pvid, untagged_bmp_copy); > + } > + clear_bit(pvid, vlan_bmp_copy); > + vinfo.vid = pvid; > + if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO, > + sizeof(vinfo), &vinfo)) > + goto nla_put_failure; > + } What if pvid is not in vlan_bmp_copy? This statement block seems slightly different than the original logic where BRIDGE_VLAN_INFO_PVID was set only when looping thru vlan_bitmap and vid == pvid. Now can you send a IFLA_BRIDGE_VLAN_INFO with pvid even if pvid isn't in vlan_bitmap? Maybe pvid is always in vlan_bitmap, so it doesn't matter. But there is a subtle logic change here, so something to consider. > + > + /* fill untagged vlans */ > + br_fill_ifvlaninfo_bitmap(skb, untagged_bmp_copy, > + BRIDGE_VLAN_INFO_UNTAGGED); This might be doing more than original logic. Original logic looped thru vid in vlan_btimap and checked if vid was in untagged_bitmap. Here you're dumping all bits in untagged_bitmap, without looking if bit was set in vlan_bitmap. Maybe you want to do an intersection of sets vlan_bitmap and untagged_bitmap. > + for_each_set_bit(vid, untagged_bmp_copy, VLAN_N_VID) > + clear_bit(vid, vlan_bmp_copy); > + > + /* fill tagged vlans */ > + br_fill_ifvlaninfo_bitmap(skb, vlan_bmp_copy, 0); > + > + return 0; > + > +nla_put_failure: > + return -EMSGSIZE; > +} > + > +static int br_fill_ifvlaninfo(struct sk_buff *skb, > + const struct net_port_vlans *pv) > +{ > + struct bridge_vlan_info vinfo; > + u16 pvid, vid; > + > + pvid = br_get_pvid(pv); > + for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID) { > + vinfo.vid = vid; > + vinfo.flags = 0; > + if (vid == pvid) > + vinfo.flags |= BRIDGE_VLAN_INFO_PVID; > + > + if (test_bit(vid, pv->untagged_bitmap)) > + vinfo.flags |= BRIDGE_VLAN_INFO_UNTAGGED; > + > + if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO, > + sizeof(vinfo), &vinfo)) > + goto nla_put_failure; > + } > + > + return 0; > + > +nla_put_failure: > + return -EMSGSIZE; > +} > + > /* > * Create one netlink message for one interface > * Contains port and master info as well as carrier and bridge state. > @@ -121,12 +248,11 @@ static int br_fill_ifinfo(struct sk_buff *skb, > } > > /* Check if the VID information is requested */ > - if (filter_mask & RTEXT_FILTER_BRVLAN) { > - struct nlattr *af; > + if ((filter_mask & RTEXT_FILTER_BRVLAN) || > + (filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED)) { > const struct net_port_vlans *pv; > - struct bridge_vlan_info vinfo; > - u16 vid; > - u16 pvid; > + struct nlattr *af; > + int err; > > if (port) > pv = nbp_get_vlan_info(port); > @@ -140,21 +266,12 @@ static int br_fill_ifinfo(struct sk_buff *skb, > if (!af) > goto nla_put_failure; > > - pvid = br_get_pvid(pv); > - for_each_set_bit(vid, pv->vlan_bitmap, VLAN_N_VID) { > - vinfo.vid = vid; > - vinfo.flags = 0; > - if (vid == pvid) > - vinfo.flags |= BRIDGE_VLAN_INFO_PVID; > - > - if (test_bit(vid, pv->untagged_bitmap)) > - vinfo.flags |= BRIDGE_VLAN_INFO_UNTAGGED; > - > - if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO, > - sizeof(vinfo), &vinfo)) > - goto nla_put_failure; > - } > - > + if (filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED) > + err = br_fill_ifvlaninfo_compressed(skb, pv); > + else > + err = br_fill_ifvlaninfo(skb, pv); > + if (err) > + goto nla_put_failure; > nla_nest_end(skb, af); > } > > -- > 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