netdev - Re: [PATCH net-next 2/2] net: dsa: realtek: Rewrite RTL8366RB MTU handling

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: <20231211153054.vpgbx7oufazujtzf@skbuf>
Date: Mon, 11 Dec 2023 17:30:54 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
	Alvin Šipraga <alsi@...g-olufsen.dk>,
	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] net: dsa: realtek: Rewrite RTL8366RB MTU
 handling

On Mon, Dec 11, 2023 at 12:14:39AM -0300, Luiz Angelo Daros de Luca wrote:
> > The MTU callbacks are in layer 1 size, so for example 1500
> > bytes is a normal setting. Cache this size, and only add
> > the layer 2 framing right before choosing the setting. On
> > the CPU port this will however include the DSA tag since
> > this is transmitted from the parent ethernet interface!
> >
> > Add the layer 2 overhead such as ethernet and VLAN framing
> > and FCS before selecting the size in the register.
> >
> > This will make the code easier to understand.
> >
> > The rtl8366rb_max_mtu() callback returns a bogus MTU
> > just subtracting the CPU tag, which is the only thing
> > we should NOT subtract. Return the correct layer 1
> > max MTU after removing headers and checksum.
> >
> > Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> 
> Don't we need a Fixes tag?

If there's nothing observably broken, no.

> I'm not sure you need this old code. Whenever you change the MTU in a
> user port, it will also call rtl8366rb_change_mtu() for the CPU port
> if the max MTU changes. A call to change both the port and the CPU
> port makes sense when you need to update something inside the switch
> to set the MTU per port. However, these realtek switches only have a
> global MTU size for all ports. What I did in rtl8365mb is to ignore
> any MTU change except it is related to the CPU port. I hope this is a
> "core feature" that you can rely on.

Ha, "core feature" :-/

It is a valid way to simplify the programming of a register that is
global to the switch, when the DSA methods are per port. The largest_mtu
is programmed via DSA_NOTIFIER_MTU to all cascade and CPU ports. So it
makes sense to want to use it. But with a single CPU port, the driver
would program the largest_mtu to hardware once. With 2 CPU ports (not
the case here), twice (although it would still be the same value).

To do as you recommend would still not make it a "core feature".
That would be if DSA were to call a new ds->ops->set_global_mtu() with a
clear contract and expectation about being called once (not once per CPU
port), and with the maximum value only.

Searching through the code to see how widespread the pattern is, I
noticed mv88e6xxx_change_mtu() and I think I found a bug.

static int mv88e6xxx_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
{
	struct mv88e6xxx_chip *chip = ds->priv;
	int ret = 0;

	/* For families where we don't know how to alter the MTU,
	 * just accept any value up to ETH_DATA_LEN
	 */
	if (!chip->info->ops->port_set_jumbo_size &&
	    !chip->info->ops->set_max_frame_size) {
		if (new_mtu > ETH_DATA_LEN)
			return -EINVAL;

		return 0;
	}

	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
		new_mtu += EDSA_HLEN;

	mv88e6xxx_reg_lock(chip);
	if (chip->info->ops->port_set_jumbo_size)
		ret = chip->info->ops->port_set_jumbo_size(chip, port, new_mtu);
	else if (chip->info->ops->set_max_frame_size)
		ret = chip->info->ops->set_max_frame_size(chip, new_mtu);
	mv88e6xxx_reg_unlock(chip);

	return ret;
}

If the chip uses set_max_frame_size() - which is not per port - then it
will accept any latest value, and not look just at the largest_mtu.

b53_change_mtu() also looks like it suffers from a similar problem, it
always programs the latest per-port value to a global register.

So I guess there is ample opportunity to get this wrong, and maybe
making the global MTU "core functionality" is worth considering.
As "net-next" material - I think the bugs are sufficiently artificial,
and workarounds exist, to not bother the stable kernels with fixes over
the existing API.

Would you volunteer to do that?