netdev - Re: [PATCH] leds: triggers: netdev: add a check, whether device is up

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-next>] [day] [month] [year] [list]

Message-ID: <196db01b-40ff-44ed-8e45-1b855940417f@lunn.ch>
Date: Sat, 4 Nov 2023 17:41:54 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Klaus Kudielka <klaus.kudielka@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, netdev <netdev@...r.kernel.org>,
	Russell King <rmk+kernel@...linux.org.uk>
Subject: Re: [PATCH] leds: triggers: netdev: add a check, whether device is up

[Changes the Cc: list. Dropping LED people, adding a few netdev
people]

On Sat, Nov 04, 2023 at 04:27:45PM +0100, Klaus Kudielka wrote:
> On Sat, 2023-11-04 at 15:29 +0100, Andrew Lunn wrote:
> > On Sat, Nov 04, 2023 at 01:58:40PM +0100, Klaus Kudielka wrote:
> > > Some net devices do not report NO-CARRIER, if they haven't been brought
> > > up.
> > 
> > Hi Klaus
> > 
> > We should probably fix the driver. What device is it?
> > 
> > > In that case, the netdev trigger results in a wrong link state being
> > > displayed. Fix this, by adding a check, whether the device is up.
> > 
> > Is it wrong? Maybe the carrier really is up, even if the interface is
> > admin down. Broadcast packets are being received by the
> > hardware. Maybe there is a BMC sharing the link and it is active?
> 
> My particular example is Turris Omnia, eth2 (WAN), connected to SFP.
> SFP module is inserted, but no fiber connected, so definitely no carrier. 
> 
> *Without* the patch:
> 
> After booting, the device is down, but netdev trigger reports "link" active.
> This looks wrong to me.
> 
> I can then "ip link set eth2 up", and the "link" goes away - as I
> would have expected it to be from the beginning.

Thanks for the details.

A brain dump...

You do see a lot of MAC drivers doing a netif_carrier_off() in there
probe function. That suggests the carrier is on by default. I doubt we
can change that, we would break all the drivers which assume the
carrier is on by default, probably virtual devices and some real
devices.

Often the MAC and PHY are connected in the open() callback, when using
phylib. So that is too late.  phylink_create() is however mostly used
in the probe function. So it could set the carrier to off by default.
Russell, what do you think?

Turris Omnia uses mvneta. That does not turn the carrier off in its
probe function. So a quick fix would be to add it. However, that then
becomes pointless if we make phylink_create() disable the carrier.

	Andrew