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
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ