linux-kernel - Re: [PATCH] checkpatch: allow an assignment in if condition for ACQUIRE

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] [day] [month] [year] [list]

Message-ID: <aJ6BEuNueCNuOOfQ@aschofie-mobl2.lan>
Date: Thu, 14 Aug 2025 17:36:34 -0700
From: Alison Schofield <alison.schofield@...el.com>
To: Joe Perches <joe@...ches.com>
CC: Andy Whitcroft <apw@...onical.com>, Dwaipayan Ray
	<dwaipayanray1@...il.com>, Lukas Bulwahn <lukas.bulwahn@...il.com>, "Peter
 Zijlstra" <peterz@...radead.org>, Dan Williams <dan.j.williams@...el.com>,
	<linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] checkpatch: allow an assignment in if condition for
 ACQUIRE_ERR()

On Wed, Aug 13, 2025 at 12:17:51AM -0700, Joe Perches wrote:
> On Tue, 2025-08-12 at 17:38 -0700, alison.schofield@...el.com wrote:
> > From: Alison Schofield <alison.schofield@...el.com>
> > 
> > New helpers, ACQUIRE() and ACQUIRE_ERR(), were recently introduced to
> > clean up conditional locking paths [1].
> []
> > That compact format was a deliberate choice by the authors. By making
> > this a checkpatch exception, existing ERRORs are quieted, and future
> > users of the macro will not be dissuaded by checkpatch from using the
> > preferred compact format.
> 
> not stylish IMO.

I knew when suggesting this patch, that the 'no assignment in if
condition' hasn't had any previous exceptions. I appreciate your
helping in working out the implementation details here.

more below -

> 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -5696,7 +5696,14 @@ sub process {
> >  			my ($s, $c) = ($stat, $cond);
> >  			my $fixed_assign_in_if = 0;
> >  
> > -			if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s) {
> > +			if ($c =~ /\bif\s*\((.*[^<>!=]=[^=].*)\)/s) {
> > +				my $expr = $1;
> > +
> > +				# Allow ACQUIRE_ERR() macro syntax
> > +				if ($expr =~ /\w+\s*=\s*ACQUIRE_ERR\s*\(/) {
> > +					next;
> > +				}
> 
> nak.
> 
> Using next would not do any additional checks on this line.
I see. I'll get rid of the next and use conditional blocks instead.

> Likely use basic block or a constuct like
> 
> 			if ($c =~ /\bif\s*\((.*[^<>!=]=[^=].*)/s &&
> 			    $1 !~ /\bACQUIRE_ERR\b/) {
> 
> 
> and perhaps the \w+ should be $Lval

Thanks for that. The v2 approach uses the $Lval and captures the full
if condition expression and iterates thru each assignment.

I did more diligence in v2 wrt testing and will post my test cases
along with that so you can see what the patch intends to allow and
what triggers ERRORs, including ERRORs that the v1 usage of 'next'
got ignored.

-- Alison


> 
> > +
> >  				if (ERROR("ASSIGN_IN_IF",
> >  					  "do not use assignment in if condition\n" . $herecurr) &&
> >  				    $fix && $perl_version_ok) {
>