linux-kernel - Re: [PATCH 3/3] drivers/net/ftgmac100: fix DHCP potential failure with systemd

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

Message-ID: <1675a52d-a270-d768-5ccc-35b1e82e56d2@gmail.com>
Date:   Tue, 22 Feb 2022 21:00:54 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Heyi Guo <guoheyi@...ux.alibaba.com>, linux-kernel@...r.kernel.org
Cc:     Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Joel Stanley <joel@....id.au>,
        Guangbin Huang <huangguangbin2@...wei.com>,
        Hao Chen <chenhao288@...ilicon.com>,
        Arnd Bergmann <arnd@...db.de>,
        Dylan Hung <dylan_hung@...eedtech.com>, netdev@...r.kernel.org
Subject: Re: [PATCH 3/3] drivers/net/ftgmac100: fix DHCP potential failure
 with systemd



On 2/22/2022 7:14 PM, Heyi Guo wrote:
> DHCP failures were observed with systemd 247.6. The issue could be
> reproduced by rebooting Aspeed 2600 and then running ifconfig ethX
> down/up.
> 
> It is caused by below procedures in the driver:
> 
> 1. ftgmac100_open() enables net interface and call phy_start()
> 2. When PHY is link up, it calls netif_carrier_on() and then
> adjust_link callback
> 3. ftgmac100_adjust_link() will schedule the reset task
> 4. ftgmac100_reset_task() will then reset the MAC in another schedule
> 
> After step 2, systemd will be notified to send DHCP discover packet,
> while the packet might be corrupted by MAC reset operation in step 4.
> 
> Call ftgmac100_reset() directly instead of scheduling task to fix the
> issue.
> 
> Signed-off-by: Heyi Guo <guoheyi@...ux.alibaba.com>
> ---
> Cc: Andrew Lunn <andrew@...n.ch>
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: Joel Stanley <joel@....id.au>
> Cc: Guangbin Huang <huangguangbin2@...wei.com>
> Cc: Hao Chen <chenhao288@...ilicon.com>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Dylan Hung <dylan_hung@...eedtech.com>
> Cc: netdev@...r.kernel.org
> 
> 
> ---
>   drivers/net/ethernet/faraday/ftgmac100.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index c1deb6e5d26c5..d5356db7539a4 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -1402,8 +1402,17 @@ static void ftgmac100_adjust_link(struct net_device *netdev)
>   	/* Disable all interrupts */
>   	iowrite32(0, priv->base + FTGMAC100_OFFSET_IER);
>   
> -	/* Reset the adapter asynchronously */
> -	schedule_work(&priv->reset_task);
> +	/* Release phy lock to allow ftgmac100_reset to aquire it, keeping lock

typo: acquire

> +	 * order consistent to prevent dead lock.
> +	 */
> +	if (netdev->phydev)
> +		mutex_unlock(&netdev->phydev->lock);
> +
> +	ftgmac100_reset(priv);
> +
> +	if (netdev->phydev)
> +		mutex_lock(&netdev->phydev->lock);

Do you really need to perform a full MAC reset whenever the link goes up 
or down? Instead cannot you just extract the maccr configuration which 
adjusts the speed and be done with it?

What kind of Ethernet MAC design is this seriously.
-- 
Florian