linux-kernel - Re: [PATCH] perf/x86/intel/uncore: Initialize with correct logical package ID

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]

Date:   Wed, 11 Jan 2017 11:29:25 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Prarit Bhargava <prarit@...hat.com>
cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Kan Liang <kan.liang@...el.com>, Borislav Petkov <bp@...e.de>,
        Harish Chegondi <harish.chegondi@...el.com>
Subject: Re: [PATCH] perf/x86/intel/uncore: Initialize with correct logical
 package ID

On Tue, 3 Jan 2017, Prarit Bhargava wrote:

> On multi-socket Intel v3 processor systems (aka Haswell) kdump can fail with:
> 
> BUG: unable to handle kernel paging request at 00000000006563a1
> IP: [<ffffffff8101b582>] hswep_uncore_cpu_init+0x52/0xa0
> PGD 0 [    2.313897]
> Oops: 0000 [#1] SMP
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0 #1
> Hardware name: NEC Express5800/T120f [N8100-2285Y]/GA-7WESV-NJ, BIOS 5.0.4009 08/01/2016
> task: ffff88002bdb8000 task.stack: ffffc90000014000
> RIP: 0010:[<ffffffff8101b582>]  [<ffffffff8101b582>] hswep_uncore_cpu_init+0x52/0xa0
> RSP: 0000:ffffc90000017db8  EFLAGS: 00010206
> RAX: 0000000000656369 RBX: 0000000000000000 RCX: 0000000000001e03
> RDX: ffff88002b224780 RSI: 0000000000000000 RDI: 0000000000000000
> RBP: ffffc90000017dc8 R08: 000000000001c880 R09: ffffffff813667e1
> R10: ffff880030c1c880 R11: 0000000000000000 R12: 0000000000000000
> R13: ffffffff81c1c090 R14: afafafafafafafaf R15: afafafafafafafaf
> FS:  0000000000000000(0000) GS:ffff880030c00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000006563a1 CR3: 000000002fc07000 CR4: 00000000001406b0
> Stack:
>  ffffc90000017dc8 00000000352bd002 ffffc90000017e00 ffffffff81da17f8
>  0000000000000000 ffffffff81da16f9 00000000000000f0 afafafafafafafaf
>  afafafafafafafaf ffffc90000017e78 ffffffff81002190 ffffc90000017e00
> Call Trace:
>  [<ffffffff81da17f8>] intel_uncore_init+0xff/0x2e6
>  [<ffffffff81da16f9>] ? uncore_type_init+0x158/0x158
>  [<ffffffff81002190>] do_one_initcall+0x50/0x190
>  [<ffffffff810af27b>] ? parse_args+0x27b/0x460
>  [<ffffffff81d9c357>] kernel_init_freeable+0x1a5/0x249
>  [<ffffffff81d9ba27>] ? set_debug_rodata+0x12/0x12
>  [<ffffffff81702010>] ? rest_init+0x80/0x80
>  [<ffffffff8170201e>] kernel_init+0xe/0x110
>  [<ffffffff8170f715>] ret_from_fork+0x25/0x30
> Code: 1a d5 00 39 15 cc 1c c0 00 7e 06 89 15 c4 1c c0 00 48 98 48 8b 15 d7 c3 f7 00 48 8d 04 40 48 8d 04 c2 48 8b 40 10 48 85 c0 74 1b <8b> 70 38 48 8b 78 10 48 8d 4d f4 ba 94 00 00 00 e8 b9 db 38 00
> RIP  [<ffffffff8101b582>] hswep_uncore_cpu_init+0x52/0xa0

And that back trace is useful because it looks good and occupies a lot of
space in the changelog? There is no value as we already know the call
chain. Back traces are only useful if they show a particular call path of
many possible ones and help to explain the problem.

> This is now occuring because 9d85eb9119f4 ("x86/smpboot: Make logical package
> management more robust") corrected the physical ID to logical ID mapping of the
> threads.

That's nonsense. This has absolutely nothing to do with that particular
commit. Using the hardcoded 0 physical id is simply wrong since:

commit cf6d445f6897 ("perf/x86/uncore: Track packages, not per CPU data")

> hswep_uncore_cpu_init() is hard coded for physical socket 0 and if
> the system is kdump'ing on any other socket the logical package value will be
> incorrect.  The code should not use 0 as the physical ID, and should use
> the boot cpu's physical package ID in this calculation.

Should?

No, it MUST use that. Documentation/process/submitting-patches.rst:
  
  Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
  ... as if you are giving orders to the codebase to change its
  behaviour.

I really like your patches, but you might finally start to write real
change logs instead of fairy tales.

Thanks,

	tglx