RE: net: fec: memory corruption caused by err_enet_mii_probe error path

From: Joakim Zhang <hidden>
Date: 2021-12-24 02:29:36

Hi Kegl,

-----Original Message-----
From: Kegl Rohit <redacted>
Sent: 2021年12月23日 21:35
To: Joakim Zhang <redacted>
Cc: netdev <redacted>; Andrew Lunn <andrew@lunn.ch>
Subject: Re: net: fec: memory corruption caused by err_enet_mii_probe
error path

Thanks for the hint to use of_mdiobus_register instead of the old way with
fec { phy-reset-gpios = ....}

This change could solve another issue with the smsc phys.
I think phy_reset_after_clk_enable does only something if the phy was
specified in the DT with reset-gpio = < Coming from an older kernel I was
using the fecs phy-reset-gpios and mdio auto probing and therefore
phy_reset_after_clk_enable did nothing.
And my smsc phys need this reset after enet_out enable
(PHY_RST_AFTER_CLK_EN).

int phy_reset_after_clk_enable(struct phy_device *phydev) { if (!phydev
|| !phydev->drv) return -ENODEV;

if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN)
{ phy_device_reset(phydev, 1); phy_device_reset(phydev, 0); return 1; }
return 0; }

Yes, please use mdio or phy reset, instead of mac reset, which marked as deprecated in DT-binding.

So I adapted the DT to specify the reset gpios.
BUT this solved not my issue with the not detected phy.
So i changed the generic
compatible = "ethernet-phy-ieee802.3-c22"; to the exact phy id compatible =
"ethernet-phy-id0007.c0f0";

=> Now the phy is detected on every boot

if (!is_c45 && !of_get_phy_id(child, &phy_id)) phy =
phy_device_create(mdio, addr, phy_id, 0, NULL); else phy =
get_phy_device(mdio, addr, is_c45); if (IS_ERR(phy)) { if (mii_ts)
unregister_mii_timestamper(mii_ts);
return PTR_ERR(phy);
}
rc = of_mdiobus_phy_device_register(mdio, phy, child, addr);

With a given phy id mdio probe will issue phy_device_create; otherwise
get_phy_device to read the id via mdio.

ethernet-phy@0 { reset-gpio = ....} gpio does not get asserted/deasserted
before the get_phy_device call.
Only the global mdio { reset-gpio = .... } gpio is asserted/deasserted before
the get_phy_device call.
So the phy does not get its reset after enet_out clock enable => phy is in bad
state => get_phy_device does not find the phy => no
of_mdiobus_phy_device_register => missing phy

It sounds like that phylib would play a mdio reset very earlier, before we accessing PHY registers.

Yes, generic compatible and exact phy-id compatible indeed different at this point, you caught the root case, also
let me learn more :)

I would need something like this in addition to ethernet-phy@0 { reset-gpio
= ....} mdio { reset-gpio = <&gpio4 13 GPIO_ACTIVE_LOW &gpio2 7
GPIO_ACTIVE_LOW} But mdio { reset-gpio } does only support a single gpio.

For now I am ok with the fixed phy ids.


&mac0 {
phy-mode = "rmii";
phy-handle = <&mac0_phy>;
phy-supply = <&reg_etn_3v3>;
pinctrl-names = "default";
pinctrl-0 = <&mac0_pins_a>, <&mac0_phy_reset_pin>,
<&mac1_phy_reset_pin>; status = "okay";


mdio {
#address-cells = <1>;
#size-cells = <0>;

mac0_phy: ethernet-phy@0 {
reg = <0>;
compatible = "ethernet-phy-id0007.c0f0"; reset-gpios = <&gpio4 13
GPIO_ACTIVE_LOW>; reset-assert-us = <10000>; reset-deassert-us =
<30000>; smsc,disable-energy-detect; };

mac1_phy: ethernet-phy@1 {
reg = <1>;
compatible = "ethernet-phy-id0007.c0f0"; reset-gpios = <&gpio2 7
GPIO_ACTIVE_LOW>; reset-assert-us = <10000>; reset-deassert-us =
<30000>; smsc,disable-energy-detect; }; }; };

Best Regards,
Joakim Zhang

On Thu, Dec 23, 2021 at 2:53 AM Joakim Zhang [off-list ref]
wrote:

quoted


Hi Kegl,

quoted

-----Original Message-----
From: Kegl Rohit <redacted>
Sent: 2021年12月22日 19:06
To: Joakim Zhang <redacted>
Cc: netdev <redacted>; Andrew Lunn <andrew@lunn.ch>
Subject: Re: net: fec: memory corruption caused by
err_enet_mii_probe error path

On Wed, Dec 22, 2021 at 9:01 AM Joakim Zhang
[off-list ref]
wrote:

quoted

This error path frees the DMA buffers, BUT as far I could see it
does not stop the DMA engines.
=> open() fails => frees buffers => DMA still active => MAC
receives network packet => DMA starts => random memory
corruption (use after
free) => random kernel panics

A question here, why receive path still active? MAC has not
connected to

PHY when this failure happened, should not see network activities.

It is a imx.28 platform using the fec for dual ethernet eth0 & eth1.

One of our devices (out of 10) eth1 did not detect a phy for eth1 on
ifup ( fec_open() ) => mdio error path => random system crashes But
the phy is there and the link is good, only MDIO access failed.
phys have autoneg activated after reset. So the RX path is active,
even if the fec driver says that there is no phy.

Without attached ethernet cable the phy was also not detected, BUT
the system did not crash. => Because no packets will arrive without
attached cable.

So the main issue on our side is the not detected phy. And the other
issue is the use after free in the error path.

Thanks for you detailed explanation :)

quoted

I think the main issue has something to do with phy reset handling.
On a cold boot the eth1 phy is detected successfully. A warm restart
( reboot -f ) will always lead to a not detected eth1 phy. The eth0
phy is always detected.
From past experience the dual ethernet implementation in the driver
was not the most stable one. Maybe because of a smaller user base.
In our setup both phy reset lines are connected to gpios with the
correct entry in the mac0 & mac1 DT entry.
Revert of
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
thu

b.com%2Ftorvalds%2Flinux%2Fcommit%2F7705b5ed8adccd921423019e870df

quoted

672fa423279%23diff-655f306656e7bccbec8fe6ebff2adf466bb8133f5bcb55111

quoted

2d3fe37e50b1a15&amp;data=04%7C01%7Cqiangqing.zhang%40nxp.com%7C

quoted

2f10c3ff43de4ccccc8708d9c53b037e%7C686ea1d3bc2b4c6fa92cd99c5c301635

quoted

%7C0%7C1%7C637757679518224181%7CUnknown%7CTWFpbGZsb3d8eyJ

WIj

quoted

oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1

quoted

000&amp;sdata=yIQBX3v6PRvPerVBhdDcRJpjt1aIiq%2BtL8RXfNmKs%2FA%3

quoted

D&amp;reserved=0
seems to get the phys in a correct state before the fec driver is
probed and both phys are detected.

You mean it always can detect both eth0 and eth1 PHY on a code boot,
but eth1 PHY failed when do warm reset, it may deserve a further analyze

the different for these two scenarios.

quoted

I am not sure when you will hardware reset PHY on you platforms. For
FEC driver, it seems only hardware reset PHY when probe MAC (please
note PHY reset has moved to phylib, not recommend to do it in MAC
driver),

fec_probe->of_mdiobus_register->of_mdiobus_register_phy->of_mdiobus
_ph

quoted

y_device_register->phy_device_register

Generally, a hardware reset will trigger an auto-nego together, so I
think PHY should in a correct state when you ifup the device,
although, it will also trigger auto-nego when connected PHY. What I want to

say is that you may need to check if the hardware reset at you side is suitable?
Not sure you use single GPIO to control two PHYs reset?

quoted

Just some analysis, hope this can give you some hints.

quoted

So maybe fec_stop() as counterpart to fec_restart() is missing
before freeing the buffers?
err_enet_mii_probe:
fec_stop(ndev);
fec_enet_free_buffers(ndev);

Issue happend with 5.10.83 and should also also happen with
current

master.

quoted

It's fine for me, please see if anyone else has some comments. If
not,

please cook a formal patch, thanks.
So fec_stop is the right guess to stop the rx/tx dma rings.

Other paths use netif_tx_lock_bh before calling fec_stop().
I don't think this is necessary because netif_tx_start_all_queues()
was not called before in this error path case?
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
thu

b.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fnet%2Fethern

quoted

et%2Ffreescale%2Ffec_main.c%23L3207&amp;data=04%7C01%7Cqiangqing.z

quoted

hang%40nxp.com%7C2f10c3ff43de4ccccc8708d9c53b037e%7C686ea1d3bc2b4

quoted

c6fa92cd99c5c301635%7C0%7C1%7C637757679518224181%7CUnknown%7CT

quoted

WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC

quoted

JXVCI6Mn0%3D%7C1000&amp;sdata=KfDdWkJ76Tvsd7nmXpNhdrY%2Fz9x24

quoted

UYJl6fxjwvqtYA%3D&amp;reserved=0

ACK, please submit a patch if you test there is no regressions.

Best Regards,
Joakim Zhang

quoted

index 1b1f7f2a6..8f208b4a9 100644

--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c

@@ -3211,8 +3211,9 @@ fec_enet_open(struct net_device *ndev)

        return 0;

 err_enet_mii_probe:
+       fec_stop(ndev);
        fec_enet_free_buffers(ndev);
 err_enet_alloc:
        fec_enet_clk_enable(ndev, false);
 clk_enable:

On Thu, Dec 23, 2021 at 2:53 AM Joakim Zhang [off-list ref]
wrote:

quoted


Hi Kegl,

quoted

-----Original Message-----
From: Kegl Rohit <redacted>
Sent: 2021年12月22日 19:06
To: Joakim Zhang <redacted>
Cc: netdev <redacted>; Andrew Lunn <andrew@lunn.ch>
Subject: Re: net: fec: memory corruption caused by
err_enet_mii_probe error path

On Wed, Dec 22, 2021 at 9:01 AM Joakim Zhang
[off-list ref]
wrote:

quoted

This error path frees the DMA buffers, BUT as far I could see it
does not stop the DMA engines.
=> open() fails => frees buffers => DMA still active => MAC
receives network packet => DMA starts => random memory
corruption (use after
free) => random kernel panics

A question here, why receive path still active? MAC has not
connected to

PHY when this failure happened, should not see network activities.

It is a imx.28 platform using the fec for dual ethernet eth0 & eth1.

One of our devices (out of 10) eth1 did not detect a phy for eth1 on
ifup ( fec_open() ) => mdio error path => random system crashes But
the phy is there and the link is good, only MDIO access failed.
phys have autoneg activated after reset. So the RX path is active,
even if the fec driver says that there is no phy.

Without attached ethernet cable the phy was also not detected, BUT
the system did not crash. => Because no packets will arrive without
attached cable.

So the main issue on our side is the not detected phy. And the other
issue is the use after free in the error path.

Thanks for you detailed explanation :)

quoted

I think the main issue has something to do with phy reset handling.
On a cold boot the eth1 phy is detected successfully. A warm restart
( reboot -f ) will always lead to a not detected eth1 phy. The eth0
phy is always detected.
From past experience the dual ethernet implementation in the driver
was not the most stable one. Maybe because of a smaller user base.
In our setup both phy reset lines are connected to gpios with the
correct entry in the mac0 & mac1 DT entry.
Revert of
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
thu

b.com%2Ftorvalds%2Flinux%2Fcommit%2F7705b5ed8adccd921423019e870df

quoted

672fa423279%23diff-655f306656e7bccbec8fe6ebff2adf466bb8133f5bcb55111

quoted

2d3fe37e50b1a15&amp;data=04%7C01%7Cqiangqing.zhang%40nxp.com%7C

quoted

2f10c3ff43de4ccccc8708d9c53b037e%7C686ea1d3bc2b4c6fa92cd99c5c301635

quoted

%7C0%7C1%7C637757679518224181%7CUnknown%7CTWFpbGZsb3d8eyJ

WIj

quoted

oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1

quoted

000&amp;sdata=yIQBX3v6PRvPerVBhdDcRJpjt1aIiq%2BtL8RXfNmKs%2FA%3

quoted

D&amp;reserved=0
seems to get the phys in a correct state before the fec driver is
probed and both phys are detected.

You mean it always can detect both eth0 and eth1 PHY on a code boot,
but eth1 PHY failed when do warm reset, it may deserve a further analyze

the different for these two scenarios.

quoted

I am not sure when you will hardware reset PHY on you platforms. For
FEC driver, it seems only hardware reset PHY when probe MAC (please
note PHY reset has moved to phylib, not recommend to do it in MAC
driver),

fec_probe->of_mdiobus_register->of_mdiobus_register_phy->of_mdiobus
_ph

quoted

y_device_register->phy_device_register

Generally, a hardware reset will trigger an auto-nego together, so I
think PHY should in a correct state when you ifup the device,
although, it will also trigger auto-nego when connected PHY. What I want to

say is that you may need to check if the hardware reset at you side is suitable?
Not sure you use single GPIO to control two PHYs reset?

quoted

Just some analysis, hope this can give you some hints.

quoted

So maybe fec_stop() as counterpart to fec_restart() is missing
before freeing the buffers?
err_enet_mii_probe:
fec_stop(ndev);
fec_enet_free_buffers(ndev);

Issue happend with 5.10.83 and should also also happen with
current

master.

quoted

It's fine for me, please see if anyone else has some comments. If
not,

please cook a formal patch, thanks.
So fec_stop is the right guess to stop the rx/tx dma rings.

Other paths use netif_tx_lock_bh before calling fec_stop().
I don't think this is necessary because netif_tx_start_all_queues()
was not called before in this error path case?
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
thu

b.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fnet%2Fethern

quoted

et%2Ffreescale%2Ffec_main.c%23L3207&amp;data=04%7C01%7Cqiangqing.z

quoted

hang%40nxp.com%7C2f10c3ff43de4ccccc8708d9c53b037e%7C686ea1d3bc2b4

quoted

c6fa92cd99c5c301635%7C0%7C1%7C637757679518224181%7CUnknown%7CT

quoted

WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC

quoted

JXVCI6Mn0%3D%7C1000&amp;sdata=KfDdWkJ76Tvsd7nmXpNhdrY%2Fz9x24

quoted

UYJl6fxjwvqtYA%3D&amp;reserved=0

ACK, please submit a patch if you test there is no regressions.

Best Regards,
Joakim Zhang

quoted

index 1b1f7f2a6..8f208b4a9 100644

--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c

@@ -3211,8 +3211,9 @@ fec_enet_open(struct net_device *ndev)

        return 0;

 err_enet_mii_probe:
+       fec_stop(ndev);
        fec_enet_free_buffers(ndev);
 err_enet_alloc:
        fec_enet_clk_enable(ndev, false);
 clk_enable:

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help