linux-kernel - Re: [PATCH 1/2] keys: Remove unnecessary local variable from ca_keys

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: <aP3n0Rn0UMW3_rj9@wunner.de>
Date: Sun, 26 Oct 2025 10:20:17 +0100
From: Lukas Wunner <lukas@...ner.de>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: David Howells <dhowells@...hat.com>,
	Ignat Korchagin <ignat@...udflare.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	"David S. Miller" <davem@...emloft.net>, keyrings@...r.kernel.org,
	linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] keys: Remove unnecessary local variable from
 ca_keys_setup

On Thu, Oct 23, 2025 at 04:32:31PM +0200, Thorsten Blum wrote:
> The variable 'ret', whose name implies a return variable, is only used
> to temporarily store the result of __asymmetric_key_hex_to_key_id().
> Use the result directly and remove the local variable.
[...]
> +++ b/crypto/asymmetric_keys/restrict.c
> @@ -29,15 +29,13 @@ static int __init ca_keys_setup(char *str)
>  	if (strncmp(str, "id:", 3) == 0) {
>  		struct asymmetric_key_id *p = &cakey.id;
>  		size_t hexlen = (strlen(str) - 3) / 2;
> -		int ret;
>  
>  		if (hexlen == 0 || hexlen > sizeof(cakey.data)) {
>  			pr_err("Missing or invalid ca_keys id\n");
>  			return 1;
>  		}
>  
> -		ret = __asymmetric_key_hex_to_key_id(str + 3, p, hexlen);
> -		if (ret < 0)
> +		if (__asymmetric_key_hex_to_key_id(str + 3, p, hexlen) < 0)
>  			pr_err("Unparsable ca_keys id hex string\n");
>  		else
>  			ca_keyid = p;	/* owner key 'id:xxxxxx' */

Quite honestly I don't think this change constitutes a worthwhile
improvement.

Those "if (ret)" checks are everywhere in the kernel, it's a pattern
that developers have grown accustomed to and immediately understand
when reading the code.  If it takes an extra variable declaration,
so be it.

For people (like me) who frequently have to dig in the git history
with recursive "git blame", changes like this one make life harder
because it introduces an extra step when trying to understand from
which commit a particular line of code originated.

And so changes like this one which are merely motivated by personal
stylistic preferences become a net negative.

Thanks,

Lukas