linux-kernel - Re: [PATCH v2] 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: <aJ6JJ8H3qrVd9Hhs@aschofie-mobl2.lan>
Date: Thu, 14 Aug 2025 18:11:03 -0700
From: Alison Schofield <alison.schofield@...el.com>
To: Joe Perches <joe@...ches.com>, Andy Whitcroft <apw@...onical.com>,
	Dwaipayan Ray <dwaipayanray1@...il.com>, Lukas Bulwahn
	<lukas.bulwahn@...il.com>
CC: 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 v2] checkpatch: allow an assignment in if condition for
 ACQUIRE_ERR()

On Thu, Aug 14, 2025 at 06:06:43PM -0700, alison.schofield@...el.com wrote:
> From: Alison Schofield <alison.schofield@...el.com>

Appended is the file.c used in testing. It's annotated with what is
expected to pass thru vs trigger.


/*
 * Test file for checkpatch.pl ACQUIRE_ERR exception
 * This file contains various assignment-in-if patterns to verify
 * that the checkpatch modification works correctly.
 *
 * Usage: ./scripts/checkpatch.pl --file this_filename.c --strict 
 *
 * Expect: total: 16 errors, 3 warnings, 1 checks
 */

/*
 * Valid ACQUIRE_ERR() usage patterns that should be allowed
 * Should all pass,  no checkpatch errors
 */
int test_cases_that_should_pass(void)
{
	/* Basic ACQUIRE_ERR usage - should be allowed */
	if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem_ptr)))
		return rc;

	/* ACQUIRE_ERR with different variable names - should be allowed */
	if ((ret = ACQUIRE_ERR(mutex_lock, &obj->ptr)))
		goto error_exit;

	/* ACQUIRE_ERR with longer variable names - should be allowed */
	if ((return_code = ACQUIRE_ERR(spinlock_acquire, rwsem_ptr)))
		return return_code;

	/* ACQUIRE_ERR with struct member assignment - should be allowed */
	if ((obj->value = ACQUIRE_ERR(lock_type, &rwsem_ptr)))
		return -EBUSY;

	/* Different whitespace patterns - should all be allowed */
	if ((rc = ACQUIRE_ERR(lock_type, &ptr)))
		return rc;

	if ((rc = ACQUIRE_ERR(lock_type, &ptr)))
		return rc;

	if ((rc = ACQUIRE_ERR(lock_type, &ptr)))
		return rc;

	/* Multi-line ACQUIRE_ERR - should be allowed */
	if ((rc = ACQUIRE_ERR(very_long_lock_type_name,
			      &ptr)))
		return rc;

	/* Multiple ACQUIRE_ERR assignments - should be allowed */
	if ((rc = ACQUIRE_ERR(lock_type, &ptr)) || (ret = ACQUIRE_ERR(other_lock, &rwsem_ptr)))
		return rc;

	/* ACQUIRE_ERR in complex but all-ACQUIRE_ERR expression - should be allowed */
	if ((obj->value = ACQUIRE_ERR(lock1, &ptr)) && (rc = ACQUIRE_ERR(lock2, &rwsem_ptr)))
		return rc;

	/* Comparison operators - should be allowed */
	if (rc == ACQUIRE_ERR(lock_type, &rwsem_ptr))
		return rc;

	if (rc != ACQUIRE_ERR(lock_type, &rwsem_ptr))
		return rc;

	if (rc < ACQUIRE_ERR(lock_type, &rwsem_ptr))
		return rc;

	/* Function calls without assignment - should be allowed */
	if (ACQUIRE_ERR(lock_type, &rwsem_ptr))
		return -EBUSY;

	/* Regular conditionals - should be allowed */
	if (rc)
		return rc;

	if (obj && obj->value)
		return obj->value;

	return 0;
}

/*
 * Should all trigger ERROR or WARNING or CHECK
 *
 * Non-ACQUIRE_ERR assignments that should still be caught
 * And, cases that pass ASSIGN_IN_IF, but fail other rules
 */
int test_cases_that_should_fail(void)
{
	/* ERROR: do not use assignment in if condition */
	if ((rc = regular_function()))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((value = obj->value))
		return value;

	/* ERROR: do not use assignment in if condition */
	if ((rc = value + 1))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((rc = ACQUIRE(rwsem_write_kill, &rwsem_ptr)))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((rc = some_ACQUIRE_ERR_helper_function()))
		return rc;

	/* ERROR: do not use assignment in if condition */
	ret = ACQUIRE_ERR(lock_type, &rwsem_ptr);
	if ((rc = ret))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((rc = regular_function())) /* ACQUIRE_ERR pattern in comment */
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((rc = regular_function() + ACQUIRE_ERR(lock_type, &rwsem_ptr)))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((rc = regular_function()) || (ret = ACQUIRE_ERR(lock_type, &rwsem_ptr)))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((ret = ACQUIRE_ERR(lock_type, &rwsem_ptr)) || (rc = regular_function()))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((rc = regular_function()) || (value = obj->value))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((rc = regular_function()) && (value = 22))
		return rc;

	/* Next set should pass the ASSIGN_IN_IF check but fail other rules */

	/* ERROR: trailing whitespace */
	/* WARNING: please, no space before tabs */
	if ((rc = ACQUIRE_ERR(lock_type, &ptr)))   	
		return rc;

	/* ERROR: space required before the open brace '{' */
	/* WARNING: braces {} are not necessary for single statement blocks */
	if ((rc = ACQUIRE_ERR(lock_type, &ptr))){
		return rc;
	}

	/* CHECK: line length of 150 exceeds 100 columns */
	if ((rc = ACQUIRE_ERR(very_very_very_very_very_very_very_very_very_very_very_very_long_lock_type_name_that_exceeds_typical_line_limit, &ptr)))
		return rc;

	/* ERROR: space required after that ',' (ctx:VxO) */
	/* ERROR: space required before that '&' (ctx:OxV) */
	if ((rc = ACQUIRE_ERR(lock_type,&ptr)))
		return rc;
}