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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <r3kkmihygodvoo5pa6mvdtwqrzik3dh6su32lerkficokzt4s7@w23lpg2edugf>
Date: Mon, 30 Oct 2023 15:42:52 +0100
From: Sven Auhagen <sven.auhagen@...eatech.de>
To: Leon Romanovsky <leon@...nel.org>
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 04:23:55PM +0200, Leon Romanovsky wrote:
> 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

Understood, thanks.
I will resubmit with the initial patch that has only
the driver fix in it.

Best
Sven

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