linux-kernel - Re: [PATCH v2] interconnect: avoid memory allocation when 'icc_bw

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: <aFPhdWoZDOrdrbQz@hovoldconsulting.com>
Date: Thu, 19 Jun 2025 12:07:49 +0200
From: Johan Hovold <johan@...nel.org>
To: Gabor Juhos <j4g8y7@...il.com>
Cc: Georgi Djakov <djakov@...nel.org>,
	Raviteja Laggyshetty <quic_rlaggysh@...cinc.com>,
	Johan Hovold <johan+linaro@...nel.org>,
	Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
	linux-pm@...r.kernel.org, linux-arm-msm@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] interconnect: avoid memory allocation when
 'icc_bw_lock' is held

On Wed, Jun 18, 2025 at 09:58:31PM +0200, Gabor Juhos wrote:
> The 'icc_bw_lock' mutex is introduced in commit af42269c3523
> ("interconnect: Fix locking for runpm vs reclaim") in order
> to decouple serialization of bw aggregation from codepaths
> that require memory allocation.
> 
> However commit d30f83d278a9 ("interconnect: core: Add dynamic
> id allocation support") added a devm_kasprintf() call into a
> path protected by the 'icc_bw_lock' which causes this lockdep
> warning (at least on the IPQ9574 platform):
> 
>     ======================================================
>     WARNING: possible circular locking dependency detected
>     6.15.0-next-20250529 #0 Not tainted

> Move the memory allocation part of the code outside of the protected
> path to eliminate the warning, and add a note about why it is moved
> to there. Also add memory allocation failure handling, while we are
> at it.
> 
> Fixes: d30f83d278a9 ("interconnect: core: Add dynamic id allocation support")
> Signed-off-by: Gabor Juhos <j4g8y7@...il.com>
> ---
> Changes in v2:
>   - move memory allocation outside of icc_lock
>   - issue a warning and return without modifying the node name in case of
>     memory allocation failure, and adjust the commit description
>   - remove offered tags from Johan and Bryan
>     Note: since I was not sure that that the added WARN_ON() is a substantial
>     change or not, I have removed the offered tags intentionally to be on the
>     safe side

Bah, what a mess (thanks for dropping the tags).

This dynamic id feature looks like a very ad-hoc and badly designed
interface.

icc_node_add() should not be allocating memory in the first place as it
is not designed to ever fail (e.g. does not return errors).

Generating the name could have been done as part of of
icc_node_create_dyn() or yet another helper for the caller could have
been added for that. In any case, it should be done before calling
icc_node_add().

Perhaps the best minimal fix of the regression is to move the allocation
into the two users of this interface. They already handle both dynamic
and non-dynamic node allocation explicitly.

Then whoever cares about this code can come up with a common interface
for allocating the name (e.g. move it into icc_node_create_dyn() or add
a new icc_node_init() helper or similar).

> ---
>  drivers/interconnect/core.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 1a41e59c77f85a811f78986e98401625f4cadfa3..32d969c349093bc356dc66234c62484aa9b9e872 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -1022,6 +1022,21 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider)
>  	if (WARN_ON(node->provider))
>  		return;
>  
> +	if (node->id >= ICC_DYN_ID_START) {
> +		char *name;
> +
> +		/*
> +		 * Memory allocation must be done outside of codepaths
> +		 * protected by icc_bw_lock.
> +		 */
> +		name = devm_kasprintf(provider->dev, GFP_KERNEL, "%s@%s",
> +				      node->name, dev_name(provider->dev));
> +		if (WARN_ON(!name))
> +			return;

But this won't do. We'd need to return an error to the caller (even if
this small allocation will never fail in practice).

> +
> +		node->name = name;
> +	}

Johan