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

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: <20231030142355.GB5885@unreal>
Date: Mon, 30 Oct 2023 16:23:55 +0200
From: Leon Romanovsky <leon@...nel.org>
To: Sven Auhagen <sven.auhagen@...eatech.de>
Cc: Ilias Apalodimas <ilias.apalodimas@...aro.org>,
	Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
	thomas.petazzoni@...tlin.com, hawk@...nel.org, lorenzo@...nel.org,
	Paulo.DaSilva@...erna.com, mcroce@...ux.microsoft.com
Subject: Re: [PATCH v2 1/2] net: page_pool: check page pool ethtool stats

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