linux-kernel - Re: [PATCHv2] 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>] [day] [month] [year] [list]

Date:	Thu, 7 Apr 2016 19:03:26 +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: [PATCHv2] x86/mce: Look in genpool instead of mcelog.entry[] for
 pending error records

On Thu, Apr 07, 2016 at 09:34:06AM -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>
> ---
> v2: Better names and code layout (Boris)
>     Revised commments on mce record comparisons (Ashok)
> 
>  arch/x86/kernel/cpu/mcheck/mce-genpool.c  | 46 +++++++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/mcheck/mce-internal.h | 15 ++++++++++
>  arch/x86/kernel/cpu/mcheck/mce.c          | 21 ++++++--------
>  3 files changed, 70 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..c43050b91d6d 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce-genpool.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce-genpool.c
> @@ -26,6 +26,52 @@ static struct gen_pool *mce_evt_pool;
>  static LLIST_HEAD(mce_event_llist);
>  static char gen_pool_buf[MCE_POOLSZ];
>  
> +/*
> + * Compare the record "t" with each of the records on list "l" to see if
> + * a functionally equivalent one is present in the list.

functionally?

> + */
> +static bool is_duplicate_mce_record(struct mce_evt_llist *t, struct mce_evt_llist *l)
> +{
> +	struct mce_evt_llist *node;
> +	struct mce *m1, *m2;
> +
> +	m1 = &t->mce;
> +
> +	llist_for_each_entry(node, &l->llnode, llnode) {
> +		m2 = &node->mce;
> +
> +		if (mce_cmp(m1, m2))

Sorry for nitpicking but isn't it usually the case that a
_cmp()-something function should return 0 when both things are equal?

I.e., you have:

	if (!strcmp(s1, s2))
		...

I think if we do it this way here too, it'll be very natural. mce_cmp()
would then have to do:

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

simply.

Hmmm?

Rest looks ok.

-- 
Regards/Gruss,
    Boris.

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