linux-kernel - Re: [PATCH v3] 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: <aFwBYRF0wJwVDdeX@hovoldconsulting.com>
Date: Wed, 25 Jun 2025 16:02:09 +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 v3] interconnect: avoid memory allocation when
 'icc_bw_lock' is held

On Wed, Jun 25, 2025 at 03:15:53PM +0200, Gabor Juhos wrote:
> 2025. 06. 25. 14:41 keltezéssel, Johan Hovold írta:
> > On Wed, Jun 25, 2025 at 02:30:15PM +0200, Johan Hovold wrote:
> >> On Wed, Jun 25, 2025 at 01:25:04PM +0200, Gabor Juhos wrote:
> > 
> >>> @@ -276,13 +276,17 @@ int qcom_icc_rpmh_probe(struct platform_device *pdev)
> >>>  		qcom_icc_bcm_init(qp->bcms[i], dev);
> >>>  
> >>>  	for (i = 0; i < num_nodes; i++) {
> >>> +		bool is_dyn_node = false;
> >>> +
> >>>  		qn = qnodes[i];
> >>>  		if (!qn)
> >>>  			continue;
> >>>  
> >>>  		if (desc->alloc_dyn_id) {
> >>> -			if (!qn->node)
> >>> +			if (!qn->node) {
> >>
> >> AFAICS, qn->node will currently never be set here and I'm not sure why
> >> commit 7f9560a3bebe ("interconnect: qcom: icc-rpmh: Add dynamic icc node
> >> id support") added this check, or even the node field to struct
> >> qcom_icc_desc for that matter.
> >>
> >> But if there's some future use case for this, then you may or may not
> >> need to make sure that a name is allocated also in that case.
> > 
> > Ok, I see what's going on. The qn->node may have been (pre-) allocated
> > in icc_link_nodes() dynamically, which means you need to make sure to
> > generate a name also in that case.
> > 
> >> And that could be done by simply checking if node->id >=
> >> ICC_DYN_ID_START instead of using a boolean flag below. That may be
> >> preferred either way.
> > 
> > So you should probably use node->id to determine this.
> 
> You are right. The problem is that ICC_DYN_ID_START is only visible from the
> core code. Either we have to move that into the 'interconnect-provider.h' header
> or we have to add an icc_node_is_dynamic() helper or something similar.
> 
> Which is the preferred solution?

I think adding a helper like icc_node_is_dynamic() in a separate
preparatory patch is best here.

If it wasn't for nodes now being created also in icc_link_nodes() we
could otherwise perhaps just as well have moved the name generation into
icc_node_create_dyn(). Now it seems we'd need a new helper to set the
name (or add error handling for every icc_node_add()), but we've already
spent way too much time trying to clean up this mess...

Johan