netdev - Re: [PATCH net-next] net: stmmac: publish actual MTU restriction

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:   Mon, 3 Apr 2023 20:17:23 +0200
From:   Corinna Vinschen <vinschen@...hat.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu <joabreu@...opsys.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: stmmac: publish actual MTU restriction

On Apr  3 09:30, Jakub Kicinski wrote:
> On Mon, 3 Apr 2023 11:12:12 +0200 Corinna Vinschen wrote:
> > > Are any users depending on the advertised values being exactly right?  
> > 
> > The max MTU is advertised per interface:
> > 
> > p -d link show dev enp0s29f1
> > 2: enp0s29f1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
> >     link/ether [...] promiscuity 0 minmtu 46 maxmtu 9000 [...]
> > 
> > So the idea is surely that the user can check it and then set the MTU
> > accordingly.  If the interface claims a max MTU of 9000, the expectation
> > is that setting the MTU to this value just works, right?
> > 
> > So isn't it better if the interface only claims what it actually supports,
> > i. .e, 
> > 
> >   # ip -d link show dev enp0s29f1
> >   2: enp0s29f1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
> >       link/ether [...] promiscuity 0 minmtu 46 maxmtu 4096 [...]
> > 
> > ?
> 
> No doubt that it's better to be more precise.
> 
> The question is what about drivers which can't support full MTU with
> certain features enabled. So far nobody has been updating the max MTU
> dynamically, to my knowledge, so the max MTU value is the static max
> under best conditions.

Yeah, I agree, but what this code does *is* to set the max MTU to the
best possible value.

In all(*) drivers using the stmmac core, the max MTU is restricted by
the size of a single TX queue per the code in stmmac_change_mtu().

You can change the number of queues within the limits of the HW
dynamically, but the size of the queues is not configurable.  The size
of the queues is determined at probe time, typically as tx_fifo_size,
a HW feature, divided by the max number of queues.  Or, in dwmac-intel.c,
simply set to the constant value 4096.

(*) Noticable exception is dwmac-mediatek, which doesn't support jumbo
    frames and sets max mtu to ETH_DATA_LEN.

> > > > +	/* stmmac_change_mtu restricts MTU to queue size.
> > > > +	 * Set maxmtu accordingly, if it hasn't been set from DT.
> > > > +	 */
> > > > +	if (priv->plat->maxmtu == 0) {
> > > > +		priv->plat->maxmtu = priv->plat->tx_fifo_size ?:
> > > > +				     priv->dma_cap.tx_fifo_size;
> > > > +		priv->plat->maxmtu /= priv->plat->tx_queues_to_use;  
> > > 
> > > tx_queues_to_use may change due to reconfiguration, no?
> > > What will happen then?  
> > 
> > Nothing.  tx_fifo_size is tx_queues_to_use multiplied by the size of the
> > queue.  All the above code does is to compute the size of the queues,
> > which is a fixed value limiting the size of the MTU.  It's the same
> > check the stmmac_change_mtu() function performs to allow or deny the MTU
> > change, basically:
> > 
> >   txfifosz = priv->plat->tx_fifo_size;
> >   if (txfifosz == 0)
> >     txfifosz = priv->dma_cap.tx_fifo_size;
> >   txfifosz /= priv->plat->tx_queues_to_use;
> >   if (txfifosz < new_mtu)
> >     return -EINVAL;
> 
> I haven't looked at the code in detail but if we start with
> tx_queues_to_use = 4 and lower it via ethtool -L, won't that
> make the core prevent setting higher MTU even tho the driver
> would have supported it previously?

No.  See the code snippet from stmmac_change_mtu() above.  Max MTU is
*not* restricted by the number of queues, but by the non-configurable
size of a single queue.

After probe, you can potentially change the number of queues, but not
the size of the queues.


Corinna