linux-kernel - Re: [PATCH v2 6/6] x86/mce: Dynamically register default MCE handler

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: <20200103214229.GA10677@agluck-desk2.amr.corp.intel.com>
Date:   Fri, 3 Jan 2020 13:42:29 -0800
From:   "Luck, Tony" <tony.luck@...el.com>
To:     Jan H. Schönherr <jschoenh@...zon.de>
Cc:     Borislav Petkov <bp@...en8.de>,
        Yazen Ghannam <yazen.ghannam@....com>,
        linux-kernel@...r.kernel.org, linux-edac@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org
Subject: Re: [PATCH v2 6/6] x86/mce: Dynamically register default MCE handler

On Fri, Jan 03, 2020 at 04:07:22PM +0100, Jan H. Schönherr wrote:
> The default MCE handler takes action, when no external MCE handler is
> registered. Instead of checking for external handlers within the default
> MCE handler, only register the default MCE handler when there are no
> external handlers in the first place.
> 
> Signed-off-by: Jan H. Schönherr <jschoenh@...zon.de>
> ---
> New in v2. This is something that became possible due to patch 4.
> I'm not entirely happy with it.
> 
> One the one hand, I'm wondering whether there's a nicer way to handle
> (de-)registration races.
> 

Instead of unregistering/registering the default notifier depending
on whether there are other notifiers, couldn't you just make the
default notifier check to see if it should print. E.g.

static int mce_default_notifier(struct notifier_block *nb, unsigned long val,
			void *data)
{
	struct mce *m = (struct mce *)data;

	if (m && !atomic_read(&num_notifiers))
		__print_mce(m);

	return NOTIFY_DONE;
}

> On the other hand, I'm starting to question the whole logic to "only print
> the MCE if nothing else in the kernel has a handler registered". Is that
> really how it should be? For example, there are handlers that filter for a
> specific subset of MCEs. If one of those is registered, we're losing all
> information for MCEs that don't match.

Maybe put control of this into the hands of the notifiers ... if
a notifier thinks that it does something useful with the log
that makes the console log redundant, then it could call a function
to bump the count to suppress the __print_mce(). Simple filter functions
on the chain wouldn't do that.

If we go this path the variable should be named something like
"suppress_console_mce" rather than num_notifiers.

Might also be useful to have some command line option or debugfs knob
to force printing for those cases where bad stuff is happening and we
don't see what was logged before a crash drops all the bits on the floor.

-Tony