linux-kernel - Re: [PATCH V2] perf/x86/intel/uncore: Fix iounmap() leak on global

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: <32386e0a-09d9-4f13-a5e1-c6f9dd3afdfd@intel.com>
Date: Wed, 14 Jan 2026 16:57:24 -0800
From: "Chen, Zide" <zide.chen@...el.com>
To: Markus Elfring <Markus.Elfring@....de>, linux-perf-users@...r.kernel.org,
 Adrian Hunter <adrian.hunter@...el.com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Andi Kleen <ak@...ux.intel.com>, Arnaldo Carvalho de Melo <acme@...nel.org>,
 Dapeng Mi <dapeng1.mi@...ux.intel.com>, Ian Rogers <irogers@...gle.com>,
 Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
 Namhyung Kim <namhyung@...nel.org>, Stephane Eranian <eranian@...gle.com>
Cc: lkp@...el.com, LKML <linux-kernel@...r.kernel.org>,
 kernel-janitors@...r.kernel.org, Thomas Falcon <thomas.falcon@...el.com>,
 Xudong Hao <xudong.hao@...el.com>
Subject: Re: [PATCH V2] perf/x86/intel/uncore: Fix iounmap() leak on
 global_init failure



On 1/14/2026 12:57 PM, Markus Elfring wrote:
>> Kernel test robot reported:
> 
> Is this duplicate information according to a known tag?

I was trying to follow some examples of how test robot reports are shown
in commit messages, for example:

https://lore.kernel.org/all/20240626151131.550535-2-torvalds@linux-foundation.org/

https://lore.kernel.org/all/173058261037.3137.8690137124888546964.tip-bot2@tip-bot2/


>> Unverified Error/Warning (likely false positive, kindly check if
>> interested):
>>     arch/x86/events/intel/uncore_discovery.c:293:2-8:
>>     ERROR: missing iounmap; ioremap on line 288 and execution via
>>     conditional on line 292
> 
> See also:
> https://elixir.bootlin.com/linux/v6.19-rc5/source/scripts/coccinelle/free/iounmap.cocci#L2-L8
> 
> 
>> If domain->global_init() fails in __parse_discovery_table(), the
>> ioremap'ed MMIO region is not released before returning, resulting
>> in an MMIO mapping leak.
>>
>> Reported-by: kernel test robot <lkp@...el.com>
> 
> See also once more:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.19-rc5#n94
> 
> Will another imperative wording approach become helpful for an improved change description?

My apologies — I did try to address the feedback, but I may still be
missing something.  Could you please point out what specifically could
be improved?


> …
>> ---
>>  arch/x86/events/intel/uncore_discovery.c | 15 ++++++++++-----
> …
> 
> Some contributors would appreciate patch version descriptions.
> https://lore.kernel.org/all/?q=%22This+looks+like+a+new+version+of+a+previously+submitted+patch%22
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.19-rc5#n310

Yes, that was the intention for v2.

V2:
- As suggested by Markus, add an `out` label and use goto-based error
  handling to reduce duplicated iounmap() code.
- Add the original warning from the kernel test robot to the commit message.
- Trivial rewording of the commit message.

> 
> Is there a need to perform desirable changes by a small patch series?
> 
> * Specific fix
> * Related refinements

It seems to me that the changes in this patch are small and closely
related, so I wondered whether splitting it might be unnecessary.

> 
>> +++ b/arch/x86/events/intel/uncore_discovery.c
>> @@ -264,6 +264,7 @@ static int __parse_discovery_table(struct uncore_discovery_domain *domain,
>>  	struct uncore_unit_discovery unit;
>>  	void __iomem *io_addr;
>>  	unsigned long size;
>> +	int ret = 0;
>>  	int i;
> 
> Would scope adjustments become helpful for any of these local variables?

As mentioned in my earlier reply, my suggestion was to avoid doing other
unrelated optimizations in this patch.

https://lore.kernel.org/all/e7d74d9d-cb45-4f5f-8e44-502dd7c4bcff@intel.com/T/#t



>> @@ -273,21 +274,23 @@ static int __parse_discovery_table(struct uncore_discovery_domain *domain,
>>  
>>  	/* Read Global Discovery State */
>>  	memcpy_fromio(&global, io_addr, sizeof(struct uncore_global_discovery));
>> +	iounmap(io_addr);
>> +
>>  	if (uncore_discovery_invalid_unit(global)) {
> …
>>  	}
>> -	iounmap(io_addr);
> 
> Can this modification part be interpreted as an optimisation?

Yes, this is technically an optimization. Since the patch is already
refactoring the iounmap() handling, my thinking was that it would be
reasonable to include it in the same patch.

> …
>> -	if (domain->global_init && domain->global_init(global.ctl))
>> -		return -ENODEV;
>> +	if (domain->global_init && domain->global_init(global.ctl)) {
>> +		ret = -ENODEV;
>> +		goto out;
>> +	}
> …
>>  	*parsed = true;
>> +
>> +out:
> 
> Would an other label be a bit clearer here?

I’d suggest keeping the label name out for style consistency, as
mentioned in my earlier reply.

https://lore.kernel.org/all/e7d74d9d-cb45-4f5f-8e44-502dd7c4bcff@intel.com/T/#t

> 
> unmap_io:
> 
>>  	iounmap(io_addr);
>> -	return 0;
>> +	return ret;
>>  }
> …
> 
> 
> Regards,
> Markus