| 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
| ||
|
Message-ID: <54A238BD.7050707@cumulusnetworks.com>
Date: Mon, 29 Dec 2014 21:31:41 -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 5/6] bridge: new function to pack vlans using both IFLA_BRIDGE_VLAN_INFO
and IFLA_BRIDGE_VLAN_RANGE_INFO
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 ?
>
>> + 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?
yes, you can AFAIK. pvid is usually untagged.
> Maybe pvid is always in vlan_bitmap, so it doesn't
> matter. But there is a subtle logic change here, so something to
> consider.
Understand what you are saying, I will check this with v2.
>> +
>> + /* 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.
There is a clear_bit for vlan_bmp_copy below.
And all this works as expected on our boxes (Its tested code).
But, understand the difference with the original loop. Will confirm.
plus, we will see if we can use bitmap intersection here.
>> + 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