netdev - Re: [PATCH net-2.6] bonding: drop frames received with master's source MAC

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]

Message-ID: <24610.1298672888@death>
Date:	Fri, 25 Feb 2011 14:28:08 -0800
From:	Jay Vosburgh <fubar@...ibm.com>
To:	=?ISO-8859-1?Q?Nicolas_de_Peslo=FCan?= 
	<nicolas.2p.debian@...il.com>
cc:	Andy Gospodarek <andy@...yhouse.net>, netdev@...r.kernel.org,
	David Miller <davem@...emloft.net>,
	Herbert Xu <herbert@...dor.hengli.com.au>,
	Jiri Pirko <jpirko@...hat.com>
Subject: Re: [PATCH net-2.6] bonding: drop frames received with master's source MAC

Nicolas de Pesloüan 	<nicolas.2p.debian@...il.com> wrote:

>Le 25/02/2011 22:13, Andy Gospodarek a écrit :
>> I was looking at my system and wondering why I sometimes saw these
>> DAD messages in my logs:
>>
>> bond0: IPv6 duplicate address fe80::21b:21ff:fe38:2ec4 detected!
>>
>> I traced it back and realized the IPv6 Neighbor Solicitations I was
>> sending were also coming back into the stack on the slave(s) that did
>> not transmit the frames.  I could not think of a compelling reason to
>> notify the user that a NS we sent came back, so I set out to just drop
>> the frame silently in ndisc_recv_ns drop.
>>
>> That seemed to work well, but when I thought about it I could not
>> compelling reason to save any of these frames.  Dropping them as soon as
>> we get them seems like a much better idea as it fixes other issues that
>> may exist for more than just IPv6 DAD.
>>
>> I chose to check the incoming frame against the master's MAC address as
>> that should be the MAC address used anytime a broadcast frame is sent by
>> the bonding driver that had the chance to make its way back into one of
>> the other devices.
>
>I think this could break the ARP monitoring. ARP monitoring rely on a
>normal protocol handler, registered in bond_main.c.
>
>void bond_register_arp(struct bonding *bond)
>{
>        struct packet_type *pt = &bond->arp_mon_pt;
>
>        if (pt->type)
>                return;
>
>        pt->type = htons(ETH_P_ARP);
>        pt->dev = bond->dev;
>        pt->func = bond_arp_rcv;
>        dev_add_pack(pt);
>}
>
>For as far as I understand, some variants of arp_validate require the
>backup interfaces to receive ARP requests sent from the master, through
>the active interface, presumably with the master MAC as the source MAC.
>
>As this protocol handler is registered at the master level, the exact
>match logic in __netif_receive_skb(), which apply at the slave level,
>shouldn't deliver this skb to bond_arp_rcv().
>
>Can someone confirm ? Jay ?

	Yes, this is how the ARP monitor works for inactive slaves in
active-backup mode.  It expects to see the broadcast ARP requests loop
back around to the inactive slaves.  If arp_validate is on, the ARP
frames will be inspected to insure that it was sent by the appropriate
master.

	Still, though, if the NS packets are coming in on an inactive
slave, why aren't they already being dropped?  Even in alb mode, there
is a loose concept of "active" and "inactive" in the sense that only one
slave is used for things like broadcast or multicast.

	Andy, what configuration are you seeing this problem in?

	-J

>	Nicolas.
>
>> Signed-off-by: Andy Gospodarek<andy@...yhouse.net>
>> Cc: David Miller<davem@...emloft.net>
>> Cc: Herbert Xu<herbert@...dor.apana.org.au>
>> Cc: Jay Vosburgh<fubar@...ibm.com>
>> Cc: Jiri Pirko<jpirko@...hat.com>
>>
>> ---
>>
>> I realize this patch comes right in the middle of Jiri Pirko's attempts
>> to move this functionality to the bonding driver, but I think this may
>> be important enough to include now (possibly in 2.6.38 and to -stable)
>> if others agree.
>>
>> ---
>>   net/core/dev.c |    5 +++++
>>   1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 8ae6631..4a76ccd 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2971,6 +2971,11 @@ static inline void skb_bond_set_mac_by_master(struct sk_buff *skb,
>>   int __skb_bond_should_drop(struct sk_buff *skb, struct net_device *master)
>>   {
>>   	struct net_device *dev = skb->dev;
>> +	struct ethhdr *eth = eth_hdr(skb);
>> +
>> +	/* Drop all frames with the bond master's source address. */
>> +	if (unlikely(!compare_ether_addr(eth->h_source, master->dev_addr)))
>> +		return 1;
>>
>>   	if (master->priv_flags&  IFF_MASTER_ARPMON)
>>   		dev->last_rx = jiffies;

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@...ibm.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