| 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
| ||
|
Message-ID: <20200718112102.GJ23489@earth.li>
Date: Sat, 18 Jul 2020 12:21:02 +0100
From: Jonathan McDowell <noodles@...th.li>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
David Miller <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Matthew Hagan <mnhagan88@...il.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: dsa: qca8k: implement the port MTU callbacks
On Sat, Jul 18, 2020 at 01:38:08PM +0300, Vladimir Oltean wrote:
> On Sat, Jul 18, 2020 at 10:35:55AM +0100, Jonathan McDowell wrote:
> > This switch has a single max frame size configuration register, so we
> > track the requested MTU for each port and apply the largest.
> >
> > Signed-off-by: Jonathan McDowell <noodles@...th.li>
> > ---
> > drivers/net/dsa/qca8k.c | 38 ++++++++++++++++++++++++++++++++++++++
> > drivers/net/dsa/qca8k.h | 3 +++
> > 2 files changed, 41 insertions(+)
> >
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index 4acad5fa0c84..3690f02aea3a 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -670,6 +670,12 @@ qca8k_setup(struct dsa_switch *ds)
> > }
> > }
> >
> > + /* Setup our port MTUs to match power on defaults */
> > + for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> > + priv->port_mtu[i] = ETH_FRAME_LEN + ETH_FCS_LEN;
> > + }
>
> I am not quite sure the curly brackets are needed. And nowhere else in
> qca8k.c is this convention being used.
Good point; force of habit from coding standards elsewhere. Fixed (and
also the instance below).
> > + qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, ETH_FRAME_LEN + ETH_FCS_LEN);
> > +
> > /* Flush the FDB table */
> > qca8k_fdb_flush(priv);
> >
> > @@ -1098,6 +1104,36 @@ qca8k_port_disable(struct dsa_switch *ds, int port)
> > priv->port_sts[port].enabled = 0;
> > }
> >
> > +static int
> > +qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> > +{
> > + struct qca8k_priv *priv = ds->priv;
> > + int i, mtu;
> > +
> > + if ((new_mtu < ETH_MIN_MTU) || (new_mtu > QCA8K_MAX_MTU)) {
> > + return -EINVAL;
> > + }
>
> I'm pretty sure this check should not be needed.
> The only reason why slave_dev->min_mtu is 0 seems to be:
>
> commit 8b1efc0f83f1f75b8f85c70d2211007de8fd7633
> Author: Jarod Wilson <jarod@...hat.com>
> Date: Thu Oct 20 23:25:27 2016 -0400
>
> net: remove MTU limits on a few ether_setup callers
>
> These few drivers call ether_setup(), but have no ndo_change_mtu, and thus
> were overlooked for changes to MTU range checking behavior. They
> previously had no range checks, so for feature-parity, set their min_mtu
> to 0 and max_mtu to ETH_MAX_MTU (65535), instead of the 68 and 1500
> inherited from the ether_setup() changes. Fine-tuning can come after we get
> back to full feature-parity here.
>
> CC: netdev@...r.kernel.org
> Reported-by: Asbjoern Sloth Toennesen <asbjorn@...jorn.st>
> CC: Asbjoern Sloth Toennesen <asbjorn@...jorn.st>
> CC: R Parameswaran <parameswaran.r7@...il.com>
> Signed-off-by: Jarod Wilson <jarod@...hat.com>
> Signed-off-by: David S. Miller <davem@...emloft.net>
>
> which is an oversight on my part. Since now DSA supports
> ndo_change_mtu(), the "slave_dev->min_mtu = 0;" line in net/dsa/slave.c
> can be removed and so can this check.
Ok.
> > +
> > + priv->port_mtu[port] = new_mtu;
> > +
> > + mtu = 0;
>
> I think it's more typical to initialize mtu to 0 at declaration time.
Sure, fixed.
> > + for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> > + if (priv->port_mtu[port] > mtu)
> > + mtu = priv->port_mtu[port];
> > + }
>
> Again, curly brackets are not needed here, although some might feel it
> aids readability.
>
> > +
> > + /* Include L2 header / FCS length */
> > + qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +qca8k_port_max_mtu(struct dsa_switch *ds, int port)
> > +{
> > + return QCA8K_MAX_MTU;
>
> So what is the maximum value that you can write into
> QCA8K_MAX_FRAME_SIZE? 9000 or 9018? If it's 9000, you should report a
> max MTU of 8982, and let the network stack do the range check for you,
> that's why this callback exists in the first place.
9018 (including header + fcs), hence returning QCA8K_MAX_MTU here.
> Do you know how are VLAN tags accounted for (i.e. does iperf3 TCP work
> over a VLAN sub-interface after your patch)? There are 2 options:
> - The ports automatically increase the maximum accepted frame size by 4
> (or 8, in case of double tag) bytes if they see VLAN tagged traffic.
> Case in which you don't need to do anything.
> - You need to manually account for the possibility that VLAN-tagged
> traffic will be received, since the 802.1Q header is not part of the
> SDU whose max length is measured by the MTU. So you might want to
> write a value to QCA8K_MAX_FRAME_SIZE that is either "mtu +
> VLAN_ETH_HLEN + ETH_FCS_LEN", or "mtu + ETH_HLEN + 2 * VLAN_HLEN +
> ETH_FCS_LEN", depending on whether you foresee double-tagging being
> used.
The value is for a normal packet; the switch accounts for VLAN / double
VLAN / the switch tagging header size itself.
> > +}
> > +
> > static int
> > qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
> > u16 port_mask, u16 vid)
> > @@ -1174,6 +1210,8 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> > .set_mac_eee = qca8k_set_mac_eee,
> > .port_enable = qca8k_port_enable,
> > .port_disable = qca8k_port_disable,
> > + .port_change_mtu = qca8k_port_change_mtu,
> > + .port_max_mtu = qca8k_port_max_mtu,
> > .port_stp_state_set = qca8k_port_stp_state_set,
> > .port_bridge_join = qca8k_port_bridge_join,
> > .port_bridge_leave = qca8k_port_bridge_leave,
> > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> > index 10ef2bca2cde..31439396401c 100644
> > --- a/drivers/net/dsa/qca8k.h
> > +++ b/drivers/net/dsa/qca8k.h
> > @@ -13,6 +13,7 @@
> > #include <linux/gpio.h>
> >
> > #define QCA8K_NUM_PORTS 7
> > +#define QCA8K_MAX_MTU 9000
> >
> > #define PHY_ID_QCA8337 0x004dd036
> > #define QCA8K_ID_QCA8337 0x13
> > @@ -58,6 +59,7 @@
> > #define QCA8K_MDIO_MASTER_MAX_REG 32
> > #define QCA8K_GOL_MAC_ADDR0 0x60
> > #define QCA8K_GOL_MAC_ADDR1 0x64
> > +#define QCA8K_MAX_FRAME_SIZE 0x78
> > #define QCA8K_REG_PORT_STATUS(_i) (0x07c + (_i) * 4)
> > #define QCA8K_PORT_STATUS_SPEED GENMASK(1, 0)
> > #define QCA8K_PORT_STATUS_SPEED_10 0
> > @@ -189,6 +191,7 @@ struct qca8k_priv {
> > struct device *dev;
> > struct dsa_switch_ops ops;
> > struct gpio_desc *reset_gpio;
> > + unsigned int port_mtu[QCA8K_NUM_PORTS];
> > };
> >
> > struct qca8k_mib_desc {
> > --
> > 2.27.0
J.
--
/-\ | 101 things you can't have too much
|@/ Debian GNU/Linux Developer | of : 19 - A Good Thing.
\- |
Powered by blists - more mailing lists