public inbox for [email protected]
 help / color / mirror / Atom feed
From: Ammar Faizi <[email protected]>
To: Borislav Petkov <[email protected]>
Cc: Thomas Gleixner <[email protected]>,
	Alviro Iskandar Setiawan <[email protected]>,
	Alviro Iskandar Setiawan <[email protected]>,
	Dave Hansen <[email protected]>,
	Greg Kroah-Hartman <[email protected]>,
	"H. Peter Anvin" <[email protected]>, Ingo Molnar <[email protected]>,
	Tony Luck <[email protected]>,
	Yazen Ghannam <[email protected]>,
	[email protected], [email protected],
	[email protected], [email protected], [email protected]
Subject: Re: [PATCH v5 2/2] x86/MCE/AMD: Fix memory leak when `threshold_create_bank()` fails
Date: Mon, 28 Mar 2022 11:12:53 +0700	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

On 3/28/22 5:52 AM, Borislav Petkov wrote:
[...]
>> Fixes: 6458de97fc15 ("x86/mce/amd: Straighten CPU hotplug path")
> 
> How did you decide this is the commit that this is fixing?

I examined the history in those lines by git blame. Will recheck after the below
doubt is cleared.

>> Link: https://lore.kernel.org/lkml/[email protected]
> 
> That Link tag is not needed.
> 
>> Co-authored-by: Alviro Iskandar Setiawan <[email protected]>
>> Signed-off-by: Alviro Iskandar Setiawan <[email protected]>
>> Co-authored-by: Yazen Ghannam <[email protected]>
> 
> There's no "Co-authored-by".
> 
> The correct tag is described in
> 
> Documentation/process/submitting-patches.rst

Will fix them in the v6.

> ...
> 
>> @@ -1350,15 +1357,14 @@ int mce_threshold_create_device(unsigned int cpu)
>>   		if (!(this_cpu_read(bank_map) & (1 << bank)))
>>   			continue;
>>   		err = threshold_create_bank(bp, cpu, bank);
>> -		if (err)
>> -			goto out_err;
>> +		if (err) {
>> +			_mce_threshold_remove_device(bp, numbanks);
>> +			return err;
>> +		}
>>   	}
>>   	this_cpu_write(threshold_banks, bp);
> 
> Do I see it correctly that the publishing of the @bp pointer - i.e.,
> this line - should be moved right above the for loop?
> 
> Then mce_threshold_remove_device() would properly free it in the error
> case and your patch turns into a oneliner?

Previously, in v4 I did that too. But after discussion with Yazen, we got a
conclusion that placing `this_cpu_write(threshold_banks, bp);` before the for loop
is not the right thing to do.

> And then your Fixes: tag would be correct too...
The reason is based on the discussion with Yazen, the full discussion can be read in
the Link tag above.

==================
The point is:

On Wed, 2 Mar 2022 17:26:32 +0000, Yazen Ghannam <[email protected]> wrote:
> The threshold interrupt handler uses this pointer. I think the goal here is to
> set this pointer when the list is fully formed and clear this pointer before
> making any changes to the list. Otherwise, the interrupt handler will operate
> on incomplete data if an interrupt comes in the middle of these updates.
==================

Also, looking at the comment in mce_threshold_remove_device() function:

	/*
	 * Clear the pointer before cleaning up, so that the interrupt won't
	 * touch anything of this.
	 */
	this_cpu_write(threshold_banks, NULL);

I think it's reasonable to place `this_cpu_write(threshold_banks, bp);` after
the "for loop" on the creation process for the similar reason. In short, don't
let the interrupt sees incomplete data.

Although, I am not sure if that 100% guarantees mce_threshold_remove_device()
will not mess up with the interrupt (e.g. freeing the data while the interrupt
reading it), unless we're using RCU stuff.

What do you think?

-- 
Ammar Faizi

  reply	other threads:[~2022-03-28  4:12 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-10  1:53 [PATCH v5 0/2] Two x86 fixes Ammar Faizi
2022-03-10  1:53 ` [PATCH v5 1/2] x86/delay: Fix the wrong asm constraint in `delay_loop()` Ammar Faizi
2022-03-27 21:38   ` Borislav Petkov
2022-03-28  4:16     ` Ammar Faizi
2022-03-28  4:29     ` Ammar Faizi
2022-03-28  7:56       ` Borislav Petkov
2022-03-10  1:53 ` [PATCH v5 2/2] x86/MCE/AMD: Fix memory leak when `threshold_create_bank()` fails Ammar Faizi
2022-03-27 22:52   ` Borislav Petkov
2022-03-28  4:12     ` Ammar Faizi [this message]
2022-03-28  8:05       ` Borislav Petkov
2022-03-17  8:19 ` [PATCH v5 0/2] Two x86 fixes Ammar Faizi
2022-03-17  9:27   ` Borislav Petkov
2022-03-17  9:50     ` Ammar Faizi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox