Message-ID: <6b63fe0f-a435-1fdf-bc56-10622b832419@gmail.com>
Date:   Mon, 23 Jan 2023 10:57:10 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     "Sverdlin, Alexander" <alexander.sverdlin@...mens.com>,
        "olteanv@...il.com" <olteanv@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "andrew@...n.ch" <andrew@...n.ch>
Cc:     "Freihofer, Adrian" <adrian.freihofer@...mens.com>
Subject: Re: dsa: phy started on dsa_register_switch()

On 1/23/23 10:35, Sverdlin, Alexander wrote:
> Dear DSA maintainers,
> 
> I've been puzzled by the fact ports of the DSA switches are enabled on
> bootup by default (PHYs configured, LEDs ON, etc) in contrast to the
> normal Ethernet ports.
> 
> Some people tend to think this is a security issue that port is "open"
> even though no configuration has been performed on the port, so I
> looked into the differences between Ethernet drivers and DSA
> infrastructure.

If you are concerned about security with a switch, then clearly the 
switch should have an EEPROM which configures it to isolate all of the 
ports from one another, and possibly do additional configuration to 
prevent any packets from leaking. The PHY and Ethernet link being active 
would not be a reliable way to ensure you start up in a secure state, it 
may participate into it, but it is not the only thing you can rely upon.

> 
> Traditionally phylink_of_phy_connect() and phylink_connect_phy() are
> being called from _open() callbacks of the Ethernet drivers so
> as long as the Ethernet ports are !IFF_UP PHYs are not running,
> LEDs are OFF, etc.

This is what is advised for Ethernet controller drivers to do, but is 
not strictly enforced or true throughout the entire tree, that is, it 
depends largely upon whether people writing/maintaining those drivers 
are sensitive to that behavior.

> 
> Now with DSA phylink_of_phy_connect() is being called by
> dsa_slave_phy_setup() which in turn is being called already in
> dsa_slave_create(), at the time a switch is being DT-parsed and
> created.
> 
> This confuses users a bit because neither CPU nor user ports have
> been setup yet from userspace via netlink, yet the LEDs are ON.

You seem to be assuming a certain user visible behavior that actually 
relies on both hardware and software to be configured properly. Having 
LEDs remain OFF from the time you apply power to the system, till you 
actually have your operating system and its switch driver running and 
issue "ip link set dev sw0p0 up" requires a lot more coordination.

I am sensitive to the power management aspect that getting the PHY and 
Ethernet link negotiated and then (re)negotiated several times through a 
products' boot cycle is a waste of energy and too many times do we break 
and make the link. The security aspect, I am less sensitive since the 
PHY is not how it should be enforced.

> 
> The things get worse when a user performs
> "ip link set dev lan1 up; ip link set dev lan1 down", because now
> the LEDs go OFF.

That seems to be exactly the behavior you want based upon the previous 
paragraph as it indicates that the PHY has been powered down.

> 
> Is this behaviour intended? Shall I try to develop patches moving
> phylink_.*phy_connect to dsa_slave_open() and something similar
> for CPU port?

Yes this was intentional since the beginning to speed up 
auto-negotiation, and it dates back to when DSA was brought into the 
kernel circa 2008.

We are almost guaranteed to be breaking someone's behavior if we 
postpone the connection to the PHY, however we could introduce a flag 
and make the deferring of connecting to the PHY to ndo_open() a driver 
by driver decision when proven this has no ill effect.
-- 
Florian

Powered by blists - more mailing lists