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

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: <20220926111131.252ee69f@kernel.org>
Date:   Mon, 26 Sep 2022 11:11:31 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [RFC PATCH net-next] net: linkwatch: only report
 IF_OPER_LOWERLAYERDOWN if iflink is actually down

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.