dsa: phy started on dsa_register_switch()

dsa: phy started on dsa_register_switch()

[Sverdlin, Alexander]

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.

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.

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.

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.

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

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: dsa: phy started on dsa_register_switch()

[Florian Fainelli]

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

Re: dsa: phy started on dsa_register_switch()

[Sverdlin, Alexander]

Thank you Florian for the quick reply!

On Mon, 2023-01-23 at 10:57 -0800, Florian Fainelli wrote:
> > 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.
> 

Absolutely! This was merely an indication problem, people want to see
that disabled ports are visibly disabled from the outside, and we
want to bring the netdev state in sync with LED indication.

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

You are right, this behavior is up to the individual drivers, there
is no guarantee for this in core.

[...]

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

Maybe you are right and we should just toggle IFF_UP once on boot
to sync the state, as this behavour is not unified...
Thanks for looking into this!

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

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: dsa: phy started on dsa_register_switch()

[Andrew Lunn]

On Mon, Jan 23, 2023 at 10:57:10AM -0800, Florian Fainelli wrote:
> 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.

It depends on the switch vendor, but Marvell Switches have a NO_CPU
strapping. If strapped this way, they power up in dumb switch mode,
all ports active, one big bridge. If NO_CPU is not strapped, depending
on the generation of device, the PHYs may come up, but the switch core
keeps the ports isolated, so no traffic will flow. So if security is a
design consideration, check if the switch you are using has such a
strapping. And do consider an EEPROM to explicitly shut everything
down. Also, watch out for the bootloader. Some switches have basic
support in u-boot, and barebox, enough to get the switch into a state
your can tftpboot over one of its ports.

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

There are a number of MACs which do the connect in the probe. Both are
considered correct, and doing it in probe has advantages, such as
being able to return -EPROBE_DEFER because the PHY has not appeared
yet.

It should also be noted that phylib will not explicitly down a PHY
during the PHYs probe. So if the PHY is strapped to come up on power
up, or something else like the bootloader to brought it up, it will
remain up.

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

There are also people who as sensitive to the time it takes to
establish link. Auto-neg takes around 1.5 seconds. In some embedded
uses cases, that can be a long time, they want to avoid waiting for
that by strapping the PHY to start autoneg on power on, so it might be
ready to go by the time userspace ifup's the interface.

In general, PHY LEDs are a wild west in Linux, no well defined
behaviour. So i expect there are traditional NICs whos LEDs are on
when the link is admin down.

     Andrew

Re: dsa: phy started on dsa_register_switch()

[Sverdlin, Alexander]

Hi Andrew,

On Mon, 2023-01-23 at 20:49 +0100, Andrew Lunn wrote:
> There are a number of MACs which do the connect in the probe. Both
> are
> considered correct, and doing it in probe has advantages, such as
> being able to return -EPROBE_DEFER because the PHY has not appeared
> yet.
> 
> It should also be noted that phylib will not explicitly down a PHY
> during the PHYs probe. So if the PHY is strapped to come up on power
> up, or something else like the bootloader to brought it up, it will
> remain up.
> 
> > 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.
> 
> There are also people who as sensitive to the time it takes to
> establish link. Auto-neg takes around 1.5 seconds. In some embedded
> uses cases, that can be a long time, they want to avoid waiting for
> that by strapping the PHY to start autoneg on power on, so it might
> be
> ready to go by the time userspace ifup's the interface.
> 
> In general, PHY LEDs are a wild west in Linux, no well defined
> behaviour. So i expect there are traditional NICs whos LEDs are on
> when the link is admin down.

thank you for the explanation!

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com