linux-kernel - Re: [PATCH net-next] net: stmmac: Add support to Ethtool get/set ring parameters

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

Date:   Wed, 16 Sep 2020 15:25:50 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     vee.khee.wong@...el.com
Cc:     peppe.cavallaro@...com, alexandre.torgue@...com,
        joabreu@...opsys.com, kuba@...nel.org, mcoquelin.stm32@...il.com,
        linux@...linux.org.uk, netdev@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        boon.leong.ong@...el.com, weifeng.voon@...el.com,
        yoong.siang.song@...el.com, sadhishkhanna.vijaya.balan@...el.com,
        chen.yong.seow@...el.com
Subject: Re: [PATCH net-next] net: stmmac: Add support to Ethtool get/set
 ring parameters

From: Wong Vee Khee <vee.khee.wong@...el.com>
Date: Wed, 16 Sep 2020 15:40:20 +0800

> +int stmmac_reinit_ringparam(struct net_device *dev, u32 rx_size, u32 tx_size)
> +{
> +	struct stmmac_priv *priv = netdev_priv(dev);
> +	int ret = 0;
> +
> +	if (netif_running(dev))
> +		stmmac_release(dev);
 ...
> +	if (netif_running(dev))
> +		ret = stmmac_open(dev);
> +

I've applied this patch but this approach is so fragile, but everyone
does it initially because it is so easy.

The problem here is that for so many reasons the stmmac_open() can
fail, and instead of just the ringparam() operation failing, the
interface becomes down and unusable.

Can you please eventually implement this properly?  Allocate the new
ring resources, and only commit the new configuration if it is
guaranteed to succeed.  Otherwise, backout the ringparam change
and keep the old configuration.

This way the device stays up regardless of whether the resources
(memory, DMA mappings, etc.) can be allocated fully.

Right now, if you do a ringparam under hard memory pressure, this will
take the inteface down as stmmac_open() fails.