| 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: Wed, 31 Dec 2014 17:50:56 -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, 9:31 PM, roopa 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 ?
>
>>
>>> + 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.
re-checked and the code is correct and does not deviate from original
logic.
The vlan_bitmap has all vids set in the untagged_bitmap.
>
> 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