linux-kernel - Re: [PATCH] Revert "scripts/coccinelle: Find PTR

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: <aQNw5NqZSZk5JNxn@hovoldconsulting.com>
Date: Thu, 30 Oct 2025 15:06:28 +0100
From: Johan Hovold <johan@...nel.org>
To: Gal Pressman <gal@...dia.com>
Cc: Julia Lawall <Julia.Lawall@...ia.fr>,
	Nicolas Palix <nicolas.palix@...g.fr>,
	Jakub Kicinski <kuba@...nel.org>, Alexei Lazar <alazar@...dia.com>,
	Simon Horman <horms@...nel.org>, cocci@...ia.fr,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Revert "scripts/coccinelle: Find PTR_ERR() to %pe
 candidates"

On Wed, Oct 29, 2025 at 07:35:25PM +0200, Gal Pressman wrote:
> On 29/10/2025 18:38, Johan Hovold wrote:
> > On Wed, Oct 29, 2025 at 03:59:50PM +0200, Gal Pressman wrote:

> > Unlike the rest of the misc cocci scripts I skimmed, this one does not
> > guard against any bugs. Instead it's pushing for a subjective style
> > preference, which is just going to result in churn when the clean up
> > crew starts sending mindless conversions of individual printks.
> 
> Not all cocci scripts are used for fixing bugs:
> err_cast.cocci
> memdup.cocci
> minmax.cocci
> string_choices.cocci
> 
> They are used to improve the code.

I only skimmed the misc ones, but I still things there's a difference
here since "%pe" is changing the output (e.g. unlike err_cast.cocci) and
is essentially a subjective preference.

> We can probably agree that for new code %pe is preferable, but I
> understand your concerns about the churn of conversions.

I'm not even convinced about new drivers. For consistency you'd need to
add *ERR_PTR()* to more or less every dev_err() in the driver (which
becomes ugly):

	if (ret)
		dev_err(dev, "failed to ...: %pe\n", ERR_PTR(ret));

And by now many of us recognise (or can easily lookup) the errnos used
in the occasional error message if at all needed.

Note that in most cases you have ret variable that holds the errno,
which would not be caught by this cocci script either:

	ret = PTR_ERR(p);
	dev_err(dev, "failed to ...: %d\n", ret);
	return ret; // or goto out;

> >> If the issue is with automatic build bots, then maybe this test should
> >> be excluded from them, rather than deleted?
> > 
> > It's both; it's the noise the new warnings generate but also the coming
> > flood up patches to "fix" them. There are already some 40 commits or so
> > in linux-next referencing this script.
> 
> It's OK to not like these conversion patches, it's up to the maintainer
> to accept/reject them.
> 
> For example, I know Jakub dislikes the string_choices.cocci script and
> rejects conversion patches. But the script still exists and can be used
> in other places in the kernel who might have a different opinion.

It still generates noise and extra work for already overworked
maintainers that would need to explain over and over again why they are
rejecting patches that appears to fix "warnings". Some will just take
the patches, which leads to inconsistencies (as only a handful of
printks will be converted) and a push for a style which again only some
people prefer.

So I still think this script should be dropped. And you still need to
review drivers manually if you really want to use %pe consistently (e.g.
for all the cases where there is no error pointer to begin with).

Johan