[RFC PATCH net-next] net: linkwatch: only report IF_OPER_LOWERLAYERDOWN if iflink is actually down

[RFC PATCH net-next] net: linkwatch: only report IF_OPER_LOWERLAYERDOWN if iflink is actually down

[Vladimir Oltean]

RFC 2863 says:

   The lowerLayerDown state is also a refinement on the down state.
   This new state indicates that this interface runs "on top of" one or
   more other interfaces (see ifStackTable) and that this interface is
   down specifically because one or more of these lower-layer interfaces
   are down.

DSA interfaces are virtual network devices, stacked on top of the DSA
master, but they have a physical MAC, with a PHY that reports a real
link status.

But since DSA (perhaps improperly) uses an iflink to describe the
relationship to its master since commit c084080151e1 ("dsa: set ->iflink
on slave interfaces to the ifindex of the parent"), default_operstate()
will misinterpret this to mean that every time the carrier of a DSA
interface is not ok, it is because of the master being not ok.

In fact, since commit c0a8a9c27493 ("net: dsa: automatically bring user
ports down when master goes down"), DSA cannot even in theory be in the
lowerLayerDown state, because it just calls dev_close_many(), thereby
going down, when the master goes down.

We could revert the commit that creates an iflink between a DSA user
port and its master, especially since now we have an alternative
IFLA_DSA_MASTER which has less side effects. But there may be tooling in
use which relies on the iflink, which has existed since 2009.

We could also probably do something local within DSA to overwrite what
rfc2863_policy() did, in a way similar to hsr_set_operstate(), but this
seems like a hack.

What seems appropriate is to follow the iflink, and check the carrier
status of that interface as well. If that's down too, yes, keep
reporting lowerLayerDown, otherwise just down.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/core/link_watch.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/net/core/link_watch.c b/net/core/link_watch.c
index aa6cb1f90966..ae70786da9d2 100644
--- a/net/core/link_watch.c
+++ b/net/core/link_watch.c
@@ -38,9 +38,20 @@ static unsigned char default_operstate(const struct net_device *dev)
 	if (netif_testing(dev))
 		return IF_OPER_TESTING;
 
-	if (!netif_carrier_ok(dev))
-		return (dev->ifindex != dev_get_iflink(dev) ?
-			IF_OPER_LOWERLAYERDOWN : IF_OPER_DOWN);
+	if (!netif_carrier_ok(dev)) {
+		int iflink = dev_get_iflink(dev);
+		struct net_device *peer;
+
+		if (iflink == dev->ifindex)
+			return IF_OPER_DOWN;
+
+		peer = __dev_get_by_index(dev_net(dev), iflink);
+		if (!peer)
+			return IF_OPER_DOWN;
+
+		return netif_carrier_ok(peer) ? IF_OPER_DOWN :
+						IF_OPER_LOWERLAYERDOWN;
+	}
 
 	if (netif_dormant(dev))
 		return IF_OPER_DORMANT;
-- 
2.34.1

Re: [RFC PATCH net-next] net: linkwatch: only report IF_OPER_LOWERLAYERDOWN if iflink is actually down

[Jakub Kicinski]

On Thu, 22 Sep 2022 01:05:06 +0300 Vladimir Oltean wrote:
> RFC 2863 says:
> 
>    The lowerLayerDown state is also a refinement on the down state.
>    This new state indicates that this interface runs "on top of" one or
>    more other interfaces (see ifStackTable) and that this interface is
>    down specifically because one or more of these lower-layer interfaces
>    are down.
> 
> DSA interfaces are virtual network devices, stacked on top of the DSA
> master, but they have a physical MAC, with a PHY that reports a real
> link status.
> 
> But since DSA (perhaps improperly) uses an iflink to describe the
> relationship to its master since commit c084080151e1 ("dsa: set ->iflink
> on slave interfaces to the ifindex of the parent"), default_operstate()
> will misinterpret this to mean that every time the carrier of a DSA
> interface is not ok, it is because of the master being not ok.
> 
> In fact, since commit c0a8a9c27493 ("net: dsa: automatically bring user
> ports down when master goes down"), DSA cannot even in theory be in the
> lowerLayerDown state, because it just calls dev_close_many(), thereby
> going down, when the master goes down.
> 
> We could revert the commit that creates an iflink between a DSA user
> port and its master, especially since now we have an alternative
> IFLA_DSA_MASTER which has less side effects. But there may be tooling in
> use which relies on the iflink, which has existed since 2009.
> 
> We could also probably do something local within DSA to overwrite what
> rfc2863_policy() did, in a way similar to hsr_set_operstate(), but this
> seems like a hack.
> 
> What seems appropriate is to follow the iflink, and check the carrier
> status of that interface as well. If that's down too, yes, keep
> reporting lowerLayerDown, otherwise just down.

Well explained. Seems like a judgment call. IMHO the RFC is acceptable.
I'd be tempted to extend it with a comment explaining that some special
uppers (i.e. DSA) have additional sources for being down so we should
double check the lower is indeed the source of the state.