netdev - Re: [PATCH net-nex 2/2] net: mvneta: Don't speed down the PHY when changing mtu

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]

Date:   Tue, 28 Jul 2020 17:52:02 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     Jisheng.Zhang@...aptics.com
Cc:     thomas.petazzoni@...tlin.com, kuba@...nel.org,
        linux@...linux.org.uk, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-nex 2/2] net: mvneta: Don't speed down the PHY when
 changing mtu

From: Jisheng Zhang <Jisheng.Zhang@...aptics.com>
Date: Mon, 27 Jul 2020 19:53:14 +0800

> @@ -3651,7 +3651,8 @@ static void mvneta_stop_dev(struct mvneta_port *pp)
>  
>  	set_bit(__MVNETA_DOWN, &pp->state);
>  
> -	if (device_may_wakeup(&pp->dev->dev))
> +	if (device_may_wakeup(&pp->dev->dev) &&
> +	    pp->pkt_size == MVNETA_RX_PKT_SIZE(pp->dev->mtu))
>  		phylink_speed_down(pp->phylink, false);
>  

This is too much for me.

You shouldn't have to shut down the entire device and take it back up
again just to change the MTU.

Unfortunately, this is a common pattern in many drivers and it is very
dangerous to take this lazy path of just doing "stop/start" around
the MTU change.

It means you can't recover from partial failures properly,
f.e. recovering from an inability to allocate queue resources for the
new MTU.

To solve this properly, you must restructure the MTU change such that
is specifically stops the necessary and only the units of the chip
necessary to change the MTU.

It should next try to allocate the necessary resources to satisfy the
MTU change, keeping the existing resources allocated in case of
failure.

Then, only is all resources are successfully allocated, it should
commit the MTU change fully and without errors.

Then none of these link flapping issues are even possible.