linux-kernel - RE: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid range

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] [day] [month] [year] [list]
Message-ID: <F54AEECA5E2B9541821D670476DAE19C5A918CE1@PGSMSX102.gar.corp.intel.com>
Date:   Sat, 7 Jan 2017 00:54:29 +0000
From:   "Kweh, Hock Leong" <hock.leong.kweh@...el.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     "David S. Miller" <davem@...emloft.net>,
        Joao Pinto <Joao.Pinto@...opsys.com>,
        Giuseppe CAVALLARO <peppe.cavallaro@...com>,
        "seraphin.bonnaffe@...com" <seraphin.bonnaffe@...com>,
        Jarod Wilson <jarod@...hat.com>,
        Alexandre TORGUE <alexandre.torgue@...il.com>,
        "Joachim Eastwood" <manabian@...il.com>,
        Niklas Cassel <niklas.cassel@...s.com>,
        "Johan Hovold" <johan@...nel.org>, Pavel Machek <pavel@....cz>,
        "lars.persson@...s.com" <lars.persson@...s.com>,
        netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v3] net: stmmac: fix maxmtu assignment to be within
 valid range

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@...il.com]
> Sent: Saturday, January 07, 2017 8:07 AM
> To: Kweh, Hock Leong <hock.leong.kweh@...el.com>
> Cc: David S. Miller <davem@...emloft.net>; Joao Pinto
> <Joao.Pinto@...opsys.com>; Giuseppe CAVALLARO <peppe.cavallaro@...com>;
> seraphin.bonnaffe@...com; Jarod Wilson <jarod@...hat.com>; Alexandre
> TORGUE <alexandre.torgue@...il.com>; Joachim Eastwood
> <manabian@...il.com>; Niklas Cassel <niklas.cassel@...s.com>; Johan Hovold
> <johan@...nel.org>; Pavel Machek <pavel@....cz>; lars.persson@...s.com;
> netdev <netdev@...r.kernel.org>; LKML <linux-kernel@...r.kernel.org>
> Subject: Re: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid
> range
> 
> On Sat, Jan 7, 2017 at 1:47 AM, Kweh, Hock Leong
> <hock.leong.kweh@...el.com> wrote:
> 
> >> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> >> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> >> > @@ -3345,8 +3345,14 @@ int stmmac_dvr_probe(struct device *device,
> >> >                 ndev->max_mtu = JUMBO_LEN;
> >> >         else
> >> >                 ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD +
> NET_IP_ALIGN);
> >> > -       if (priv->plat->maxmtu < ndev->max_mtu)
> >>
> >> > +       if ((priv->plat->maxmtu < ndev->max_mtu) &&
> >> > +           (priv->plat->maxmtu >= ndev->min_mtu))
> >>
> >> >                 ndev->max_mtu = priv->plat->maxmtu;
> >>
> >> > +       else if (priv->plat->maxmtu < ndev->min_mtu)
> >>
> >> And if it > ndev->max_mtu?..
> >>
> >
> > Base on my understanding to the original code, the "maxmtu >= ndev-
> >max_mtu"
> > is meant for products that would want to use the value from logic which is just
> above
> > this statement where you just ask me not to add new line. That the reason the
> > stmmac_platform.c put in "plat->maxmtu = JUMBO_LEN;" as generic and I
> also
> > follow it in stmmac_pci.c.
> >
> > Or do you mean only take maxmtu = JUMBO_LEN for the option to use driver
> itself
> > assignment statement above and all the > max_mtu consider invalid?
> 
> So, just answer to the simple question: is it a valid case to have
> plat->maxmtu > ndev->max_mtu? If it so, how is it used?
> Otherwise we need a warning in such case. What did I miss?
> 

it is a valid case for priv->plat->maxmtu > ndev->max_mtu if referring
to the statement above it:

	/* MTU range: 46 - hw-specific max */
	ndev->min_mtu = ETH_ZLEN - ETH_HLEN;
	if ((priv->plat->enh_desc) || (priv->synopsys_id >= DWMAC_CORE_4_00))
		ndev->max_mtu = JUMBO_LEN;
	else
		ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN);

When the ndev->max_mtu go into the else statement, then the assignment in
stmmac_platform.c & stammac_pci.c plat->maxmtu = JUMBO_LEN is actually 
greater than ndev->max_mtu. That is what I understanding that maxmtu > max_mtu
is an option trick to allow driver assign value through the logic above instead of getting
it from of_property_read_u32(np, "max-frame-size", &plat->maxmtu); or *_default_data().

I need to revert back the V4 and submit V5.

> >
> >> > +               netdev_warn(priv->dev,
> >> > +                           "%s: warning: maxmtu having invalid value (%d)\n",
> >> > +                           __func__, priv->plat->maxmtu);
> 
> 
> >> > +       /* Set the maxmtu to a default of JUMBO_LEN in case the
> >> > +        * parameter is not defined for the device.
> >> > +        */
> >> > +       plat->maxmtu = JUMBO_LEN;
> >>
> >> Please, use *_default_data() hooks for that.
> >>
> >> At some point it might make sense to extract
> >> static int common_default_data() {...}
> >> and call it at the beginning of the rest of *_defautl_data() hooks.
> >>
> >
> > Just try to understand, are you referring changing the code something
> > like this:
> >
> >         stmmac_default_data(plat);
> >         if (info) {
> >                 info->pdev = pdev;
> >                 if (info->setup) {
> >                         ret = info->setup(plat, info);
> >                         if (ret)
> >                                 return ret;
> >                 }
> >         }
> >
> > Where all the common code is inside the stmmac_default_data()?
> 
> No.
> 
> common_default_data()
> {
>  ... common defaults among *_default_data() ...
> }
> 
> *_default_data()
> {
> ...
>  common_default_data();
>  ...
> }
> 

Ok noted. Will be a separate patch. Thanks.

Regards,
Wilson

> --
> With Best Regards,
> Andy Shevchenko