linux-kernel - Re: [PATCH v2 1/3] dsa: marvell: Provide per device information about max frame size

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: <20221219130005.6e995cb0@wsk>
Date:   Mon, 19 Dec 2022 13:00:05 +0100
From:   Lukasz Majewski <lukma@...x.de>
To:     Alexander Duyck <alexander.duyck@...il.com>
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

Hi Alexander,

> 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.

I think that I shall add:

else if (chip->info->ops->set_max_frame_size)
	return max_t(int, chip->info->max_frame_size, 1632) - (headers)

So then the "default" value of 1632 will be overwritten by 2048 bytes.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@...x.de

Content of type "application/pgp-signature" skipped