linux-kernel - Re: [PATCH] x86/mce: Look in genpool instead of mcelog.entry[] for pending error records

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] [day] [month] [year] [list]

Message-ID: <20160331084620.GB25191@pd.tnic>
Date:	Thu, 31 Mar 2016 10:46:20 +0200
From:	Borislav Petkov <bp@...en8.de>
To:	Tony Luck <tony.luck@...el.com>
Cc:	linux-kernel@...r.kernel.org, Ashok Raj <ashok.raj@...el.com>
Subject: Re: [PATCH] x86/mce: Look in genpool instead of mcelog.entry[] for
 pending error records

On Fri, Mar 25, 2016 at 11:35:17AM -0700, Tony Luck wrote:
> Couple of issues here:
> 1) MCE_LOG_LEN is only 32 - so we may have more pending records than will
>    fit in the buffer on high core count cpus
> 2) During a panic we may have a lot of duplicate records because multiple
>    logical cpus may have seen and logged the same error because some
>    banks are shared.
> 
> Switch to using the genpool to look for the pending records. Squeeze
> out duplicated records.
> 
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
> 
> New solution for an old problem. See
>   https://lkml.kernel.org/r/1443073720-3940-1-git-send-email-ashok.raj@intel.com
> for the previous take where Ashok said "bump MCE_LOG_LEN from 32 to 128" and
> Boris replied "switch to the gen_pool".
> 
> I think this clears the way to pull all of the /dev/mcelog driver
> code out of mce.c and make it just another driver that registers
> a notifier on x86_mce_decoder_chain.

Good, finally!

Once we have all the code around RAS daemon and things, we could finally
mark it deprecated and make it a default off config option for distros
which still need mcelog.

>  arch/x86/kernel/cpu/mcheck/mce-genpool.c  | 42 +++++++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/mcheck/mce-internal.h | 10 ++++++++
>  arch/x86/kernel/cpu/mcheck/mce.c          | 21 +++++++---------
>  3 files changed, 61 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce-genpool.c b/arch/x86/kernel/cpu/mcheck/mce-genpool.c
> index 0a850100c594..2dc276260fea 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce-genpool.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce-genpool.c
> @@ -26,6 +26,48 @@ static struct gen_pool *mce_evt_pool;
>  static LLIST_HEAD(mce_event_llist);
>  static char gen_pool_buf[MCE_POOLSZ];
>  
> +static bool duplicate_mce_record(struct mce_evt_llist *t, struct mce_evt_llist *l)

Hmm, having the verb at the beginning of the name makes me think this
function duplicates MCE records :)

Maybe

	is_mce_record_duplicate()

or
	mce_record_seen()

...

Bah, all ideas I have right now are meh.

> +{
> +	struct mce_evt_llist *node;
> +	struct mce *m1, *m2;
> +
> +	m1 = &t->mce;
> +
> +	llist_for_each_entry(node, &l->llnode, llnode) {
> +		m2 = &node->mce;
> +
> +		if (dup_mce_record(m1, m2))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +/*
> + * The system has paniced - we'd like to peruse the list of mce records

		     panicked

> + * that have been queued, but not seen by anyone yet.  The list is in
> + * reverse time order, so we need to reverse it. While doing that we can
> + * also drop duplicate records (these were logged because some banks are
> + * shared between cores or by all threads on a socket).
> + */
> +struct llist_node *mce_gen_pool_panic(void)

I'd call it mce_gen_pool_prepare_records() or so.

> +{
> +	struct llist_node *head;
> +	LLIST_HEAD(new_head);
> +	struct mce_evt_llist *node, *t;
> +
> +	head = llist_del_all(&mce_event_llist);
> +	if (!head)
> +		return NULL;
> +
> +	/* squeeze out duplicates while reversing order */
> +	llist_for_each_entry_safe(node, t, head, llnode) {
> +		if (!duplicate_mce_record(node, t))
> +			llist_add(&node->llnode, &new_head);
> +	}
> +
> +	return new_head.first;
> +}
> +
>  void mce_gen_pool_process(void)
>  {
>  	struct llist_node *head;
> diff --git a/arch/x86/kernel/cpu/mcheck/mce-internal.h b/arch/x86/kernel/cpu/mcheck/mce-internal.h
> index 547720efd923..492e38e4d02e 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce-internal.h
> +++ b/arch/x86/kernel/cpu/mcheck/mce-internal.h
> @@ -35,6 +35,7 @@ void mce_gen_pool_process(void);
>  bool mce_gen_pool_empty(void);
>  int mce_gen_pool_add(struct mce *mce);
>  int mce_gen_pool_init(void);
> +struct llist_node *mce_gen_pool_panic(void);
>  
>  extern int (*mce_severity)(struct mce *a, int tolerant, char **msg, bool is_excp);
>  struct dentry *mce_get_debugfs_dir(void);
> @@ -81,3 +82,12 @@ static inline int apei_clear_mce(u64 record_id)
>  #endif
>  
>  void mce_inject_log(struct mce *m);
> +
> +/*
> + * We consider records to be duplicate if bank+status+addr+misc all match
> + */
> +static inline bool dup_mce_record(struct mce *m1, struct mce *m2)

Since this is a "comparison" of sorts, I'd prefer something like:

	mce_records_equal(m1, m2);

or even better:

	mce_cmp(m1, m2);

:-)

> +{
> +	return m1->bank == m2->bank && m1->status == m2->status &&
> +		m1->addr == m2->addr && m1->misc == m2->misc;

Should be more readable like this:

        return  m1->bank   == m2->bank &&
                m1->status == m2->status &&
                m1->addr   == m2->addr &&
                m1->misc   == m2->misc;


-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.