Re: [PATCH v2 1/2] net: page_pool: check page pool ethtool stats

[Leon Romanovsky <leon@kernel.org>]

On Mon, Oct 30, 2023 at 10:39:18AM +0100, Sven Auhagen wrote:
> On Wed, Oct 25, 2023 at 10:53:41AM +0300, Leon Romanovsky wrote:
> > On Wed, Oct 25, 2023 at 08:59:44AM +0300, Ilias Apalodimas wrote:
> > > Hi Jakub,
> > > 
> > > On Mon, Oct 02, 2023 at 12:46:50PM -0700, Jakub Kicinski wrote:
> > > > On Sun, 1 Oct 2023 13:41:15 +0200 Sven Auhagen wrote:
> > > > > If the page_pool variable is null while passing it to
> > > > > the page_pool_get_stats function we receive a kernel error.
> > > > >
> > > > > Check if the page_pool variable is at least valid.
> > > >
> > > > IMHO this seems insufficient, the driver still has to check if PP
> > > > was instantiated when the strings are queried. My weak preference
> > > > would be to stick to v1 and have the driver check all the conditions.
> > > > But if nobody else feels this way, it's fine :)
> > > 
> > > I don't disagree, but OTOH it would be sane for the API not to crash if
> > > something invalid is passed. 
> > 
> > In-kernel API assumes that in-kernel callers know how to use it.
> > 
> > > Maybe the best approach would be to keep the
> > > driver checks, which are saner, but add the page pool code as well with a
> > > printable error indicating a driver bug?
> > 
> > It is no different from adding extra checks to prevent drivers from random calls
> > with random parameters to well defined API.
> > 
> > Thanks
> 
> I can see the point for both arguments so I think we should definitely
> keep the driver check.
> 
> Is there a consensus on what to do on the page pool side?
> Do you want me to keep the additional page pool check to prevent
> a kernel crash?

I don't want to see bloat of checks in kernel API. They hide issues and
not prevent them.

> I mean the mvneta change was also implemented with this problem
> and it leads to serious side effects without an additional check.
> Especially if the page pool ethtool stats is implemented in more
> drivers in the future and the implementations are not 100% correct,
> it will crash the kernel.

Like many other driver mistakes.

Thanks

> 
> Best
> Sven
> 
> > 
> > > 
> > > Thanks
> > > /Ilias
> > >