| 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: <31868.1399491632@localhost.localdomain>
Date: Wed, 07 May 2014 12:40:32 -0700
From: Jay Vosburgh <jay.vosburgh@...onical.com>
To: Veaceslav Falico <vfalico@...hat.com>
cc: Vlad Yasevich <vyasevic@...hat.com>, netdev@...r.kernel.org,
Andy Gospodarek <andy@...yhouse.net>,
Ding Tianhong <dingtianhong@...wei.com>,
Patric McHardy <kaber@...sh.net>
Subject: Re: [PATCH v2 net] bonding: Fix stacked device detection in arp monitoring
Veaceslav Falico <vfalico@...hat.com> wrote:
>On Wed, May 07, 2014 at 02:47:36PM -0400, Vlad Yasevich wrote:
>>On 05/07/2014 02:11 PM, Veaceslav Falico wrote:
>>> On Wed, May 07, 2014 at 07:49:10PM +0200, Veaceslav Falico wrote:
>>>> On Wed, May 07, 2014 at 01:08:09PM -0400, Vlad Yasevich wrote:
>>>> ...snip...
>>>>> Yes. I verified that it works. The reason is that we are traversing
>>>>> the all_adj_list.upper list which contains all of the upper devices at
>>>>> each level. So, at vlan100 level, we will see vlan200 and all will be
>>>>> well.
>>>>
>>>> Hrm, two scenarios, with the following config:
>>>>
>>>> bond0 -> whatever1 -> vlan1 -> whatever2 -> vlan2 -> whatever3_IP
>>>>
>>>> end == whatever3_IP
>>>>
>>> ...snip...
>>>>
>>>> So, the end patch (not compiled, not tested...) would look something like
>>>> (only the bond_check_path() is changed and copied here, everything else
>>>> remains the same):
>>>>
>>>> + bool upper_found = false;
>>>> +
>>>> + netdev_for_each_all_upper_dev_rcu(start, upper, iter) {
>>>> + if (upper == end)
>>>> + upper_found = true;
>>>> +
>>>> + if (idx < 2 && is_vlan_dev(upper) &&
>>>> + bond_check_path(upper, end, tag, idx+1)) {
>>>> + tag[idx].vlan_proto = vlan_dev_vlan_proto(upper);
>>>> + tag[idx].vlan_id = vlan_dev_vlan_id(upper);
>>>> + return true;
>>>
>>> Actually, screw that, we might find here the vlan2 first and end up with
>>> only 1 vlan (vlan2, skipping vlan1).
>>>
>>
>>hmm.. I am not sure that's actually possible...
>>
>>__netdev_adjacent_dev_insert() will always insert at the tail. If you
>>have a stack of vlans:
>> vlan1 (vid 10, 802.1Q)
>> |
>> v
>> vlan2 (vid 20, 802.1AD)
>> |
>> v
>> bond0
>>
>>then vlan1 will always be at the end of the list, and after vlan2.
>>Even if we remove things, the higher the device, the later it will be in
>>the list.
>>
>>So, in the event of the configuration:
>>bond0 -> whatever1 -> vlan1 -> whatever2 -> vlan2 -> whatever3_IP
>>
>>waterver_IP will be last in the list due to list_add_tail_rcu()
>>usage.
>
>Yeah, you're right, sorry for misunderstanding (I've been tricked by the
>master thing, but it doesn't actually add anything to all_upper).
>
>Anyway, so the only concern is:
>
>bond0 -> whatever1 -> vlan1 -> whatever2 -> vlan2 -> whatever3_IP
> \-> vlan3
>bond_check_path start==bond0 idx=0
>finds vlan1, tag[0] set, recursion start==vlan1 idx=1
>\->
> bond_check_path start==vlan1 idx=1
> finds vlan2, tag[1] set, recursion start==vlan2 idx=2
> \-> returns right away with false as idx >= 2.
>
> finds vlan3 (!!!) that isn't related with whatever_IP, tag[1] set with the
> wrong vlan, recursion start==vlan3 idx=2
> \-> return right away with false as idx >= 2.
>
> finds whatever3_IP, returns true.
>returns true
>
>and this way we end up with vlan1 -> vlan3, instead of vlan1 -> vlan2.
>
>Can be fixed by that "don't go deeper/populate tag[] if idx == 2" trick.
How, exactly? I'm not sure I see a way to do this correctly
other than walking the tree layout (and I use that term loosely, given
that the upper dev list is not an actual tree data structure in the
usual sense) from bond0 -> whatever3_IP and pulling the VLANs from that
path.
Being later in the device list (and thus higher) doesn't
automatically make it part of the path, e.g.,
bond0 -> whatever1 -> vlan1 -> whatever2 -> vlan2 -> whatever3
\---> vlan3 -> whatever4_IP
if we're looking for whatever4_IP. Are we guaranteed to hit
whatever4_IP and finish up before vlan2 ends up in a tag[], even if
whatever4_IP was the last interface added to the pile?
-J
>>Did I misunderstand something in the code? I can't seem to make it
>>the above config fail in my tests.
>>
>>-vlad
>>> The way to fix this might be to get the most "lengthy" path of vlans, as
>>> in:
>>>
>>> + * Return the maximum length of stacked vlans + device found, 0 if the end
>>> + * device is not found.
>>> + */
>>> +static int bond_check_path(struct net_device *start, struct net_device
>>> *end,
>>> + struct bond_vlan_tag *tag, int idx)
>>> +{
>>> + struct net_device *upper;
>>> + struct list_head *iter;
>>> + int length, max_length = 0;
>>> +
>>> + netdev_for_each_all_upper_dev_rcu(start, upper, iter) {
>>> + if (upper == end && !max_length)
>>> + max_length = 1;
>>> +
>>> + if (idx < 2 && is_vlan_dev(upper)) {
>>> + length = bond_check_path(upper, end, tag, idx + 1);
>>> +
>>> + if (max_length < length + 1) {
>>> + tag[idx].vlan_proto =
>>> vlan_dev_vlan_proto(upper);
>>> + tag[idx].vlan_id = vlan_dev_vlan_id(upper);
>>> + max_length = length + 1;
>>> + }
>>> + }
>>> + }
>>> + return max_length;
>>> +}
>>>
>>> Hope that helps.
>>>
>>>> + }
>>>> + }
>>>> + return upper_found;
>>> ...snip...
---
-Jay Vosburgh, jay.vosburgh@...onical.com
--
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