netdev - Re: [PATCH v2] net/hsr: Add support for the High-availability Seamless Redundancy protocol (HSRv0)

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: <51D2156A.3070403@xdin.com>
Date:	Tue, 2 Jul 2013 01:48:58 +0200
From:	Arvid Brodin <arvid.brodin@...n.com>
To:	David Miller <davem@...emloft.net>
CC:	<netdev@...r.kernel.org>, <shemminger@...tta.com>,
	<joe@...ches.com>, <jboticario@...il.com>,
	<balferreira@...glemail.com>, Arvid Brodin <Arvid.Brodin@...n.com>
Subject: Re: [PATCH v2] net/hsr: Add support for the High-availability Seamless
 Redundancy protocol (HSRv0)

On 2013-06-29 06:16, David Miller wrote:
> From: Arvid Brodin <arvid.brodin@...n.com>
> Date: Wed, 26 Jun 2013 17:16:11 +0200
>
>> +static int hsr_dev_open(struct net_device *dev)
>> +{
>> +	struct hsr_priv *hsr_priv;
>> +
>> +	hsr_priv = netdev_priv(dev);
>> +
>> +	if (hsr_priv->slave[0])
>> +		dev_open(hsr_priv->slave[0]);
>> +	if (hsr_priv->slave[1])
>> +		dev_open(hsr_priv->slave[1]);
>> +
>> +	return 0;
>> +}
>
> dev_open() can and does fail, you must thus check for error returns, undo any
> necessary state, and propagate that error to callers of hsr_dev_open.
>

I'm not sure it's an error (from the HSR interface's point of view) if the slave(s)
won't come up here. The calls to dev_open() can be seen more like a convenience
than a necessity (I actually left them out to begin with). If none of the slaves
could go up the HSR interface would end up as admin UP but with operstate
IF_OPER_LOWERLAYERDOWN until any of the slaves went up.

I see that the VLAN code, in its vlan_dev_open(), checks the underlying interface's
IFF_UP flag and returns -ENETDOWN if it's not set. That might be an option here -
but then one would have to set three interfaces up to get a working HSR interface,
which is a bit tedious... Also, the HSR interface could end up in the state above
anyway if both slaves went down after the HSR interface went up successfully.

What about checking the return values and printing a warning if a slave dev_open()
fails, but still return success for the HSR interface no matter what?

-- 
Arvid Brodin | Consultant (Linux)
XDIN AB | Knarrarnäsgatan 7 | SE-164 40 Kista | Sweden | xdin.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