netdev - Re: [PATCH net] bonding: fix arp requests sends with isolated routes

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 17 Feb 2014 12:07:28 +0100
From:	François Cachereul <f.cachereul@...halink.fr>
To:	Veaceslav Falico <vfalico@...hat.com>
CC:	Jay Vosburgh <fubar@...ibm.com>,
	Andy Gospodarek <andy@...yhouse.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net] bonding: fix arp requests sends with isolated routes

Le 17/02/2014 10:36, Veaceslav Falico a écrit :
> On Fri, Feb 14, 2014 at 04:59:23PM +0100, François Cachereul wrote:
>> Make arp_send_all() try to send arp packets through slave devices event
>> if no route to arp_ip_target is found. This is useful when the route
>> is in an isolated routing table with routing rule parameters like oif or
>> iif in which case ip_route_output() return an error.
>> Thus, the arp packet is send without vlan and with the bond ip address
>> as sender.
> 
> I'm not sure I understand it completely, specifically I don't really
> understand the term "isolated routing table". Do you mean that it's an
> routing table different from local/main, which is enabled by
> CONFIG_IP_MULTIPLE_TABLES=y ? I think they should be all 'catched' by
> ip_route_output(), or am I missing something?

That what I meant, but like I said when the rule to lookup a table contains
iif or oif parameters (for example with the bond device value),
ip_route_output() can't lookup this table as flowi4_iif is set to
LOOPBACK_INDEX and we set flowi4_oif to 0 because we are searching for it.

> 
> Anyway, with this fix bonding will send packets even if it doesn't find
> route AND src addr (bond_confirm_addr() can return 0 if bond doesn't have
> the required ip assigned), which isn't good at all.

We may had a test to avoid this case but that should also be done if the
route is found because bond_confirm_addr() could likewise return 0.

> 
> If my assumption about the routing tables is correct and ip_route_output()
> doesn't find addition tables, we should at least try to fix it via scanning
> those tables.

ip_route_output() scan all tables for which the rule match parameters like
src network, dst network, oif or iif. In my case, it can't match iif or oif.
If we want to search all tables in any configurations we would have to bypass
route lookup process or at least the "rule match" part. I don't know if it's
a good idea.

Anyway, what I understand is that we are searching for a route only to find
a vlan to add to the output packet, but we don't care of the route. We just
want the packet to pass filtering switches to test the link.
I just propose a default configuration in order to succeed most time
(when we can't find any route) in a way to find a previous behavior in which
this use case worked.

> Sorry if I didn't understand you correctly...

No problem, you understand it well. Thanks for the reply.

> 
>>
>> Signed-off-by: François CACHEREUL <f.cachereul@...halink.fr>
>> ---
>> This previously worked, the problem was added in 2.6.35 with vlan 0
>> added by default when the module 8021q is loaded. Before that no route
>> lookup was done if the bond device did not have any vlan. The problem
>> now exists event if the module 8021q is not loaded.
>>
>> drivers/net/bonding/bond_main.c |    9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 8676649..300e5b8 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -2168,17 +2168,19 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>>     for (i = 0; i < BOND_MAX_ARP_TARGETS && targets[i]; i++) {
>>         pr_debug("basa: target %pI4\n", &targets[i]);
>>
>> +        vlan_id = 0;
>> +
>>         /* Find out through which dev should the packet go */
>>         rt = ip_route_output(dev_net(bond->dev), targets[i], 0,
>>                      RTO_ONLINK, 0);
>>         if (IS_ERR(rt)) {
>>             pr_debug("%s: no route to arp_ip_target %pI4\n",
>>                  bond->dev->name, &targets[i]);
>> -            continue;
>> +            /* no route found, trying with bond->dev */
>> +            addr = bond_confirm_addr(bond->dev, targets[i], 0);
>> +            goto rt_err_try;
>>         }
>>
>> -        vlan_id = 0;
>> -
>>         /* bond device itself */
>>         if (rt->dst.dev == bond->dev)
>>             goto found;
>> @@ -2232,6 +2234,7 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
>> found:
>>         addr = bond_confirm_addr(rt->dst.dev, targets[i], 0);
>>         ip_rt_put(rt);
>> +rt_err_try:
>>         bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
>>                   addr, vlan_id);
>>     }
>> -- 
>> 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