netdev - Re: [PATCH net-next] bonding: do not pass link-local packets to master-interface

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] [day] [month] [year] [list]

Message-ID: <9142.1492215625@famine>
Date:   Fri, 14 Apr 2017 17:20:25 -0700
From:   Jay Vosburgh <jay.vosburgh@...onical.com>
To:     =?UTF-8?B?TWFoZXNoIEJhbmRld2FyICjgpK7gpLngpYfgpLYg4KSs4KSC4KSh4KWH4KS14KS+4KSwKQ==?= 
        <maheshb@...gle.com>
cc:     Chonggang Li <chonggangli@...gle.com>, andy@...yhouse.net,
        vfalico@...il.com, nikolay@...hat.com,
        Eric Dumazet <edumazet@...gle.com>,
        David Miller <davem@...emloft.net>,
        linux-netdev <netdev@...r.kernel.org>,
        =?UTF-8?Q?Maciej_=C5=BBenczykowski?= 
        <maze@...gle.com>
Subject: Re: [PATCH net-next] bonding: do not pass link-local packets to master-interface

Mahesh Bandewar (महेश बंडेवार) wrote:

>On Fri, Apr 14, 2017 at 12:30 PM, Jay Vosburgh
><jay.vosburgh@...onical.com> wrote:
>>
>>
>> Chonggang Li <chonggangli@...gle.com> wrote:
>>
>> >Previously link-local packets excluding LACP (which are handled by
>> >the recv_probe) received on bond slave interfaces are delivered to
>> >stack with bond-master device with RX_HANDLER_ANOTHER, however all
>> >link-local packets are link specific and should be delivered with
>> >exact same link/dev.
>>
>>         In what case is the current behavior a problem (my guess would
>> be something related to LLDP)?  What if, e.g., the bond is a bridge
>> port, will STP frames no longer propagate to the bridge?
>>
>When a link-local frame made appear to arrive on bond-master, it
>looses value since 'the info' about link on which it arrived is lost.

	This information should really be in the commit message, along
with something about the LLDP issue being solved.

>So idea behind this is to pass the frame to the stack with the link on
>which it arrived without involving the bonding-master. The same will
>happen for the STP frames too. Do you see problem with this approach?

	My look through the STP code suggests not, but I'm far from an
expert there.  I'm just concerned that this change will cause some
obscure regression in something that depends on the current behavior.

>>         Also, I think the description would be better if it mentioned
>> specifically that the patch is changing how skb->dev is set for link
>> local frames (bond->dev vs receiving interface), e.g.,
>>
>>         "[...] however all link-local packets are link specific and
>>         should be delivered with skb->dev set to the original device."
>>
>yes, makes sense.
>
>> >Signed-off-by: Chonggang Li <chonggangli@...gle.com>
>> >Signed-off-by: Mahesh Bandewar <maheshb@...gle.com>
>> >Signed-off-by: Maciej Żenczykowski <maze@...gle.com>
>> >---
>> > drivers/net/bonding/bond_main.c | 3 +++
>> > 1 file changed, 3 insertions(+)
>> >
>> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> >index 01e4a69af421..aeca3d8541b9 100644
>> >--- a/drivers/net/bonding/bond_main.c
>> >+++ b/drivers/net/bonding/bond_main.c
>> >@@ -1176,6 +1176,9 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
>> >               }
>> >       }
>> >
>> >+      /* link-local packets should not be passed to master interface */
>> >+      if (is_link_local_ether_addr(eth_hdr(skb)->h_dest))
>> >+              return RX_HANDLER_PASS;
>>
>>         Since this returns _PASS and not _EXACT, the packet will go
>> through the ptype_base packet handlers, so is the comment strictly
>> correct?
>>
>The stack does not have all link-local specific packet-type handlers
>registered by default so returning _EXACT would find nothing and
>packet will be dropped, right? So returning _PASS will deliver packet
>to the stack with skb->dev as the link on which packet arrived and
>stack can take whatever (default) action it has to take.

	Fair enough; I do think the comment would be better phrased as
something like "don't change skb->dev of link local frames"; on first
reading, I thought it meant the frames would be dropped.

	-J

>> >       if (bond_should_deliver_exact_match(skb, slave, bond))
>> >               return RX_HANDLER_EXACT;
>> >
>> >--
>> >2.12.2.762.g0e3151a226-goog
>> >
>>
---
	-Jay Vosburgh, jay.vosburgh@...onical.com