linux-kernel - Re: [PATCH v4] x86: mce: kexec: switch MCE handler for kexec/kdump

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: <20150305012447.GA16001@hori1.linux.bs1.fc.nec.co.jp>
Date:	Thu, 5 Mar 2015 01:24:47 +0000
From:	Naoya Horiguchi <n-horiguchi@...jp.nec.com>
To:	"Luck, Tony" <tony.luck@...el.com>
CC:	Borislav Petkov <bp@...en8.de>,
	Prarit Bhargava <prarit@...hat.com>,
	"Vivek Goyal" <vgoyal@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Junichi Nomura <j-nomura@...jp.nec.com>,
	Kiyoshi Ueda <k-ueda@...jp.nec.com>
Subject: Re: [PATCH v4] x86: mce: kexec: switch MCE handler for kexec/kdump

On Wed, Mar 04, 2015 at 11:12:33PM +0000, Luck, Tony wrote:
> > - fixed AR and UC order in enum severity_level because UC is severer than AR
> >  by definition. Current code is not affected by this wrong order by chance.
> 
> AR and AO are both UC errors - that happen also to be recoverable.

Maybe just saying "UC" might be confusing, meaning "UC bit is set" or "type of
error (defined in Table SDM's Table 15-6 in vol.3B) is 'Uncorrected Error'
(clearly separate from SRAR/SRAO)". You seem to mean the former, and I meant
the latter. So I should write the description more accurately like "UC=1,PCC=0"
error and "UC=1,PCC=1" error.

>  Are you really sure
> about this re-order not affecting existing code?

Sorry, I thought I checked that but I missed one, so let me check again now.
I checked all referencing site of MCE_*_SEVERITY. Most of them are using '=='
to compare the severity, where the re-order doesn't affect them.
Some are using inequalities:

- around ./arch/x86/kernel/cpu/mcheck/mce.c:720, 

          if (mce_severity(m, mca_cfg.tolerant, msg, true) >=
              MCE_PANIC_SEVERITY)                            

  , the re-order doesn't affect.

- ./arch/x86/kernel/cpu/mcheck/mce.c:819:

          if (m && global_worst >= MCE_PANIC_SEVERITY && mca_cfg.tolerant < 3)

  , the re-order doesn't affect.

- ./arch/x86/kernel/cpu/mcheck/mce.c:832:

          if (global_worst <= MCE_KEEP_SEVERITY && mca_cfg.tolerant < 3) 

  , ditto.

- ./arch/x86/kernel/cpu/mcheck/mce.c:1196:

                no_way_out = worst >= MCE_PANIC_SEVERITY;

  , ditto.

- ./arch/x86/kernel/cpu/mcheck/mce-severity.c:211:

                if (s->sev >= MCE_UC_SEVERITY && ctx == IN_KERNEL) {

  , the re-order should change to s->sev >= MCE_AR_SEVERITY to keep the
  kernel behavior.

So I fixed the last part to be included in the re-order patch.

>  You might well be right, but as every one
> else has pointed out mce_severity() is full of odd subtleties that catch people out.

I agree that this one big table is hard to maintain and bug-prone. One problem
is that it has too many fields to check so the parameter space is huge. I think
some field are checked only once, so separating it out makes table more simple
and readable.

> Is the "UC" entry at the end of the severities[] table just a catch-all for things that made it
> past all the other entries? Does it ever really get used?

I read through the severity check table and it seems that all UC=1 case
are already considered by the above entries, so it seems not used.

> What was the test case that made you promote UC above AR?

I thought of "Action required but unaffected thread is continuable" case
on kexec kernel, but I'm not sure that such a case really happens.
My motivation on the promotion was mainly from what SDM says rather than
the real testcase.

> This absolutely should not be buried in the middle of your other patch - it needs to
> be separate with a much more than two lines of commit description.

OK, I might not include this part in this series in later post, but if I do,
I'll separete it out.

Thanks,
Naoya Horiguchi--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/