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: Tue, 30 Dec 2014 00:04:05 -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 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 9:31 PM, roopa <roopa@...ulusnetworks.com> wrote: > On 12/29/14, 3:25 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 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? > > ack > >>> + u16 pvid; >> >> Aren't you getting an "unused" compile warning on this ^^^? > > will double check.. > >>> + >>> + /* 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? > > let me see... > >>> + 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? > > > I will check this again. Remember doing worst case tests for setlinks. will > get back with some worst cases tests in the next submission. >> >> >>> + } >>> + } >>> + >>> +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? > > Let me see. Or make it a static global ? Static global would be bad news for multiple threads doing a vlan fill. -- 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