[PATCH 1/5] net: Introduce DSA class for Ethernet switches

[Tom Rini]

On Tue, Jan 19, 2021 at 05:15:26PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 19 Jan 2021 at 14:00, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Tue, Jan 19, 2021 at 11:06:10AM -0700, Simon Glass wrote:
> > > Hi Claudiu,
> > >
> > > On Fri, 15 Jan 2021 at 09:47, Claudiu Manoil <claudiu.manoil at nxp.com> wrote:
> > > >
> > > > >-----Original Message-----
> > > > >From: Simon Glass <sjg at chromium.org>
> > > > >Sent: Thursday, January 14, 2021 5:42 PM
> > > > >To: Claudiu Manoil <claudiu.manoil at nxp.com>
> > > > >Cc: Joe Hershberger <joe.hershberger at ni.com>; Bin Meng
> > > > ><bmeng.cn at gmail.com>; Michael Walle <michael at walle.cc>; U-Boot Mailing
> > > > >List <u-boot at lists.denx.de>; Vladimir Oltean <vladimir.oltean at nxp.com>;
> > > > >Alexandru Marginean <alexandru.marginean at nxp.com>
> > > > >Subject: Re: [PATCH 1/5] net: Introduce DSA class for Ethernet switches
> > > > >
> > > > [...]
> > > > >
> > > > >Reviewed-by: Simon Glass <sjg at chromium.org>
> > > > >
> > > > >I don't think it is necessary to have the 'if (!pdev)' checks around
> > > > >the place. We need a way in U-Boot to have checks like that to catch
> > > > >programming errors  but to be able to turn them off in production code
> > > > >to reduce size.
> > > > >
> > > > >I suppose a Kconfig would do it, with:
> > > > >
> > > > >if (CONFIG_IS_ENABLED(SAFETY) && !pdev)
> > > > >    return log_,msg_ref("safety", -ENODEV)
> > > > >
> > > > >Also note that -ENODEV is used by drive rmodel so it generally isn't
> > > > >safe to return it as a logic error. I think in this case because it
> > > > >never happens, it should be OK.
> > > > >
> > > >
> > > > Thanks for the review, Simon.
> > > > I thought about using assert(pdev) checks, but during development the
> > > > simple "if (!pdev)..." proved more friendly.  I like your idea about enabling
> > > > the checks at compile time and disabling them in production.
> > > > For now, since this SAFETY flag is not implemented, my understanding is
> > > > that you’re ok with leaving the pdev checks in the code as they are right now
> > > > and sometime in the future these will be converted to the "SAFETY" construct
> > > > you mention.
> > > >
> > >
> > > Yes that's fine, you have my review tag.
> > >
> > > +Tom Rini what do you think about CONFIG_SAFETY or similar to allow
> > > these bug checks to be disabled for code-size reasons?
> >
> > I don't know.  Setting aside the name, my first concern is "so we
> > disable certain forms of sanity checks, now assuming a malicious entity
> > somewhere, what's now able to be exploited?"
> 
> Well if you are able to inject code then I suppose you can do
> anything. You don't need to worry about existing parameter checking.
> 
> The difference in my mind is whether there is user/input data
> involved. If so, then we need lots of checks. If it is just telling
> the clock to go a 2GHz, then the checks are probably just bloating the
> code. So rather than leave this to individual preference, I am
> wondering whether a Kconfig option might be best.

Thinking about this more, yes, if we can macro the check, then we can
hide it (and potential other checks) under some SANITY_CHECKS option.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210125/269becb6/attachment.sig>