Re: [PATCH] s390/cert_store: Hold the keyring->sem while invalidating keys

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] s390/cert_store: scoped_guard() mixed with goto in invalidate_keyring_keys()
--

commit e2f24110a6a571c7bef25566a83aee22fb61ad5a
Author: Jarkko Sakkinen <jarkko@kernel.org>

s390/cert_store: Hold the keyring->sem while invalidating keys

This commit acquires keyring->sem for reading via scoped_guard() in
invalidate_keyring_keys() while querying the keyring payload length and
reading the key serial numbers.

> diff --git a/arch/s390/kernel/cert_store.c b/arch/s390/kernel/cert_store.c
> index 998f2da561750..d35b1ea22507d 100644
> --- a/arch/s390/kernel/cert_store.c
> +++ b/arch/s390/kernel/cert_store.c
[ ... ]
> @@ -320,13 +321,17 @@ static int invalidate_keyring_keys(struct key *keyring)
>  	struct key *current_key;
>  	int rc;
>  
> -	keyring_payload_len = key_type_keyring.read(keyring, NULL, 0);
> -	num_keys = keyring_payload_len / sizeof(key_serial_t);
> -	key_array = kzalloc_objs(key_serial_t, num_keys);
> -	if (!key_array)
> -		return -ENOMEM;
> +	scoped_guard(rwsem_read, &keyring->sem) {
> +		keyring_payload_len = key_type_keyring.read(keyring, NULL, 0);
> +		num_keys = keyring_payload_len / sizeof(key_serial_t);
> +
> +		key_array = kzalloc_objs(key_serial_t, num_keys);
> +		if (!key_array)
> +			return -ENOMEM;
> +
> +		rc = key_type_keyring.read(keyring, (char *)key_array, keyring_payload_len);
> +	}
>  
> -	rc = key_type_keyring.read(keyring, (char *)key_array, keyring_payload_len);
>  	if (rc != keyring_payload_len) {
>  		pr_dbf_msg("Couldn't read keyring payload");
>  		goto out;

[Severity: Low]
In invalidate_keyring_keys() in arch/s390/kernel/cert_store.c, this patch
introduces scoped_guard(rwsem_read, &keyring->sem) while the function still
uses goto out to free key_array when key_type_keyring.read() returns a length
mismatch:

	if (rc != keyring_payload_len) {
		pr_dbf_msg("Couldn't read keyring payload");
		goto out;
	}
	...
out:
	kfree(key_array);
	return rc;

include/linux/cleanup.h notes that goto-based cleanup and scope-based cleanup
helpers should not be mixed within the same function. Could key_array also be
declared with __free(kfree) = NULL so the out label and goto can be dropped,
or should explicit down_read() and up_read() calls be used instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024144229.1546151-1-jarkko@kernel.org?part=1