linux-kernel - Re: [PATCH v2 08/10] x86/mce: Remove the redundant zeroing assignments

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: <f5d4d763-0fa2-4d84-8501-28d8cd8a1dde@intel.com>
Date: Fri, 18 Oct 2024 13:25:29 -0700
From: Sohil Mehta <sohil.mehta@...el.com>
To: Qiuxu Zhuo <qiuxu.zhuo@...el.com>, <tony.luck@...el.com>, <bp@...en8.de>
CC: <tglx@...utronix.de>, <dave.hansen@...ux.intel.com>, <mingo@...hat.com>,
	<hpa@...or.com>, <x86@...nel.org>, <linux-edac@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 08/10] x86/mce: Remove the redundant zeroing
 assignments

On 10/16/2024 5:30 AM, Qiuxu Zhuo wrote:
> As the entire mce structure is initialized to zero using memset(0)
> within mce_gather_info(), remove the redundant zeroing assignments to
> mce->misc and mce->addr.
> 

...

> 
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index e718b9bbe8e5..844a6f8d6f39 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -706,8 +706,6 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
>  		if (!mce_banks[i].ctl || !test_bit(i, *b))
>  			continue;
>  
> -		m.misc = 0;
> -		m.addr = 0;
>  		m.bank = i;
>  

This makes sense since mce_gather_info() happens in the same function.

>  		barrier();
> @@ -1284,8 +1282,6 @@ __mc_scan_banks(struct mce *m, struct pt_regs *regs, struct mce *final,
>  		if (!mce_banks[i].ctl)
>  			continue;
>  
> -		m->misc = 0;
> -		m->addr = 0;
>  		m->bank = i;
>  

However, in this case, I am not fully convinced if the misc and addr
would already be 0 when we reach here.

There are potentially a lot of things that happen in do_machine_check()
between mce_gather_info() and __mc_scan_banks(). Especially,
mce_no_way_out() which could theoretically call mce_read_aux() in some
cases.

Maybe it doesn't matter, misc and addr would be overwritten anyway. But
I feel some more details in the commit message would be useful. It
doesn't seem as simple as the brief description makes it sound (at least
to me).

>  		m->status = mce_rdmsrl(mca_msr_reg(i, MCA_STATUS));