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 for Android: free password hash cracker in your pocket

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

Message-ID: <5213A916.2020505@xdin.com>
Date:	Tue, 20 Aug 2013 19:36:22 +0200
From:	Arvid Brodin <arvid.brodin@...n.com>
To:	<netdev@...r.kernel.org>
CC:	David Miller <davem@...emloft.net>, <shemminger@...tta.com>,
	<joe@...ches.com>, <jboticario@...il.com>,
	<balferreira@...glemail.com>
Subject: Re: [PATCH v2] net/hsr: Add support for the High-availability Seamless
 Redundancy protocol (HSRv0)

On 2013-07-02 01:48, Arvid Brodin wrote:
> 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.
[snip]

Stephen Hemminger explained to me (in private email) that many of the errors
from dev_open() aren't soft errors - i.e. they should be propagated back up
even if the HSR driver doesn't care (out of memory, and hardware dead being
examples).

But HSR is often used in safety critical systems, and it may be important that
a hsr_dev->dev_open() does not fail just because one of the slave's cable
happens to be unplugged (or cut) - redundancy being the whole point of HSR.

I think the only reasonable thing to do is not to call dev_open() at all here,
and instead just inform the user if a slave is not already up. This way user
space must explicitly set both slaves UP and thus must also decide how to treat
different kinds of "slave errors".

I will send a new patch with this change.

Thanks to Stephen for helping me with this!

-- 
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