netdev - Re: [Open-FCoE] [PATCH] fcoe: correct checking for bonding

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: <20110301201220.GA2833@psychotron.redhat.com>
Date:	Tue, 1 Mar 2011 21:12:21 +0100
From:	Jiri Pirko <jpirko@...hat.com>
To:	Jay Vosburgh <fubar@...ibm.com>
Cc:	Joe Eykholt <joe.eykholt@...il.com>,
	James.Bottomley@...senPartnership.com, netdev@...r.kernel.org,
	devel@...n-fcoe.org, linux-scsi@...r.kernel.org
Subject: Re: [Open-FCoE] [PATCH] fcoe: correct checking for bonding

Tue, Mar 01, 2011 at 07:06:02PM CET, fubar@...ibm.com wrote:
>Joe Eykholt <joe.eykholt@...il.com> wrote:
>
>>On 2/28/11 10:37 PM, Jiri Pirko wrote:
>>> Mon, Feb 28, 2011 at 06:54:29PM CET, joe.eykholt@...il.com wrote:
>>>> On 2/28/11 9:15 AM, Jay Vosburgh wrote:
>>>>> Jiri Pirko<jpirko@...hat.com>   wrote:
>>>>>
>>>>>> Check for IFF_BONDING as this flag is set-up for all bonding devices.
>>>>>>
>>>>>> Signed-off-by: Jiri Pirko<jpirko@...hat.com>
>>>>>> ---
>>>>>> drivers/scsi/fcoe/fcoe.c |    4 +---
>>>>>> 1 files changed, 1 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
>>>>>> index 9f9600b..67714a4 100644
>>>>>> --- a/drivers/scsi/fcoe/fcoe.c
>>>>>> +++ b/drivers/scsi/fcoe/fcoe.c
>>>>>> @@ -285,9 +285,7 @@ static int fcoe_interface_setup(struct fcoe_interface *fcoe,
>>>>>> 	}
>>>>>>
>>>>>> 	/* Do not support for bonding device */
>>>>>> -	if ((netdev->priv_flags&   IFF_MASTER_ALB) ||
>>>>>> -	    (netdev->priv_flags&   IFF_SLAVE_INACTIVE) ||
>>>>>> -	    (netdev->priv_flags&   IFF_MASTER_8023AD)) {
>>>>>> +	if (netdev->priv_flags&   IFF_BONDING) {
>>>>>> 		FCOE_NETDEV_DBG(netdev, "Bonded interfaces not supported\n");
>>>>>> 		return -EOPNOTSUPP;
>>>>>> 	}
>>>>>
>>>>> 	Based on past discussions, I believe the intent of the code is
>>>>> to permit FCOE over bonding only for active-backup mode, and possibly
>>>>> for -xor/-rr as well.
>>>>>
>>>>> 	I'm not sure if the slave or the master is what's being tested
>>>>> here, so I'm not sure what the right thing to do is.  I suspect it's the
>>>>> master, as I recall discussion of one configuration involving
>>>>> active-backup mode balancing FCOE traffic over both the active and
>>>>> inactive slaves.  FCOE uses the "orig_dev" logic in __netif_receive_skb
>>>>> to have the packets delivered even on the nominally inactive slave.
>>>>>
>>>>> 	-J
>>>>>
>>>>> ---
>>>>> 	-Jay Vosburgh, IBM Linux Technology Center, fubar@...ibm.com
>>>>
>>>> Right.  That was the intent.  It should work on the physical dev, but probably
>>>> not on the master of the bond.
>>>>
>>>> If you have a master/slave bond for IPv4 between eth1 and eth2, say,
>>>> and they are going to two different DCE (FCoE) switches, presumably on
>>>> different VSANs but with ultimate access to the same disks,
>>>> then you want to split the FCoE traffic in active/active
>>>> mode using separate FCoE instances on eth1 and eth2 even though IP
>>>> is using active/standby on bond0.  This should work.  But, putting fcoe
>>>> on bond0 isn't going to do what you want.
>>>>
>>>> However, it seems like the check above shouldn't be checking
>>>> IFF_SLAVE_INACTIVE.   I can't test this.
>>>
>>> OK. So I guess the right check should be for:
>>> (netdev->priv_flags&  IFF_BONDING&&  netdev->flags&  IFF_MASTER)
>>
>>I think that's OK.  How about just checking for MASTER?
>>When is MASTER going to be set without BONDING?
>
>	One or two other things besides bonding use IFF_MASTER, but
>IFF_BONDING is only for bonding.
>
>>Otherwise I'd add some parens or I might code this as:
>>
>>	if ((netdev->priv_flags & (IFF_BONDING | IFF_MASTER)) ==
>>	    (IFF_BONDING | IFF_MASTER))
>
>	This doesn't work because the flags are kept in different
>places, IFF_MASTER is in flags and IFF_BONDING in priv_flags.

Exactly - I'm going to send corrected patch very soon.

Jirka

>
>	-J
>
>>Which is less clear, I know, but used to generate better code.
>>The compiler might generate the same code these days.
>>Not that this is performance-critical or anything.
>>
>>> This would disable adding all bond devices (like bond0 etc) and allows
>>> to use enslaved physdevs.
>>>
>>> Note that checking for mode is irrelevant here. Mode could be easily
>>> changed later without fcoe knowing that.
>
>	This is also true.
>
>	-J
>
>---
>	-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