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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 16 Dec 2022 09:24:35 -0800 From: Alexander Duyck <alexander.duyck@...il.com> To: Lukasz Majewski <lukma@...x.de> Cc: Andrew Lunn <andrew@...n.ch>, Vladimir Oltean <olteanv@...il.com>, Vivien Didelot <vivien.didelot@...il.com>, Florian Fainelli <f.fainelli@...il.com>, "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH v2 1/3] dsa: marvell: Provide per device information about max frame size On Fri, Dec 16, 2022 at 5:05 AM Lukasz Majewski <lukma@...x.de> wrote: > > Hi Alexander, > > > On Thu, 2022-12-15 at 15:45 +0100, Lukasz Majewski wrote: > > > Different Marvell DSA switches support different size of max frame > > > bytes to be sent. > > > > > > For example mv88e6185 supports max 1632 bytes, which is now > > > in-driver standard value. On the other hand - mv88e6250 supports > > > 2048 bytes. > > > > > > As this value is internal and may be different for each switch IC, > > > new entry in struct mv88e6xxx_info has been added to store it. > > > > > > Signed-off-by: Lukasz Majewski <lukma@...x.de> > > > --- > > > Changes for v2: > > > - Define max_frame_size with default value of 1632 bytes, > > > - Set proper value for the mv88e6250 switch SoC (linkstreet) family > > > --- > > > drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++- > > > drivers/net/dsa/mv88e6xxx/chip.h | 1 + > > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c > > > b/drivers/net/dsa/mv88e6xxx/chip.c index 2ca3cbba5764..7ae4c389ce50 > > > 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c > > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > > > @@ -3093,7 +3093,9 @@ static int mv88e6xxx_get_max_mtu(struct > > > dsa_switch *ds, int port) if (chip->info->ops->port_set_jumbo_size) > > > return 10240 - VLAN_ETH_HLEN - EDSA_HLEN - > > > ETH_FCS_LEN; else if (chip->info->ops->set_max_frame_size) > > > - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN - > > > ETH_FCS_LEN; > > > + return (chip->info->max_frame_size - VLAN_ETH_HLEN > > > + - EDSA_HLEN - ETH_FCS_LEN); > > > + > > > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > > > } > > > > > > > > > > Is there any specific reason for triggering this based on the > > existance of the function call? > > This was the original code in this driver. > > This value (1632 or 2048 bytes) is SoC (family) specific. > > By checking which device defines set_max_frame_size callback, I could > fill the chip->info->max_frame_size with 1632 value. > > > Why not just replace: > > else if (chip->info->ops->set_max_frame_size) > > with: > > else if (chip->info->max_frame_size) > > > > I think that the callback check is a bit "defensive" approach -> 1522B > is the default value and 1632 (or 10240 - jumbo) can be set only when > proper callback is defined. > > > Otherwise my concern is one gets defined without the other leading to > > a future issue as 0 - extra headers will likely wrap and while the > > return value may be a signed int, it is usually stored in an unsigned > > int so it would effectively uncap the MTU. > > Please correct me if I misunderstood something: > > The problem is with new mv88eXXXX devices, which will not provide > max_frame_size information to their chip->info struct? > > Or is there any other issue? That was mostly my concern. I was adding a bit of my own defensive programming in the event that somebody forgot to fill out the chip->info. If nothing else it might make sense to add a check to verify that the max_frame_size is populated before blindly using it. So perhaps you could do something similar to the max_t approach I had called out earlier but instead of applying it on the last case you could apply it for the "set_max_frame_size" case with 1632 being the minimum and being overwritten by 2048 if it is set in max_frame_size.
Powered by blists - more mailing lists