All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: [PATCH] x86/SMP: guard socket_cpumask[] access in cpu_smpboot_free()
Date: Mon, 30 Jun 2025 13:42:31 +0200	[thread overview]
Message-ID: <7fa05d3f-2f85-4a42-9549-b68a486e30ea@suse.com> (raw)

Before CPU identification has run (and it may not have run at all e.g.
when AP bringup failed altogether), cpu_data[].phys_proc_id (which is
what cpu_to_socket() resolves to) can't really be used. The use of
cpu_to_socket()'s result as an array index cpu_smpboot_free() therefore
needs guarding, as the function will also be invoked upon AP bringup
failure, in which case CPU identification may not have run.

Without "x86/CPU: re-work populating of cpu_data[]" [1] the issue is
less pronounced: The field starts out as zero, then has the BSP value
(likely again zero) copied into it, and it is properly invalidated only
in cpu_smpboot_free(). Still it is clearly wrong to use the BSP's socket
number here.

Making the guard work with and without the above patch applied turns out
interesting: Prior to that patch, the sole invalidation done is that in
cpu_smpboot_free(). Upon a later bringup attempt, the fields invalidated
are overwritten by the BSP values again, though. Hence compare APIC IDs,
as they cannot validly be the same once CPU identification has run.

[1] https://lists.xen.org/archives/html/xen-devel/2024-02/msg00727.html

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Sadly there was no feedback at all yet for the referenced patch.

--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -958,7 +958,13 @@ static void cpu_smpboot_free(unsigned in
     unsigned int socket = cpu_to_socket(cpu);
     struct cpuinfo_x86 *c = cpu_data;
 
-    if ( cpumask_empty(socket_cpumask[socket]) )
+    /*
+     * We may come here without the CPU having run through CPU identification.
+     * In that case the socket number cannot be relied upon, but the respective
+     * socket_cpumask[] slot also wouldn't have been set.
+     */
+    if ( c[cpu].apicid != boot_cpu_data.apicid &&
+         cpumask_empty(socket_cpumask[socket]) )
     {
         xfree(socket_cpumask[socket]);
         socket_cpumask[socket] = NULL;


             reply	other threads:[~2025-06-30 11:43 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-06-30 11:42 Jan Beulich [this message]
2025-06-30 11:45 ` [PATCH] x86/SMP: guard socket_cpumask[] access in cpu_smpboot_free() Jan Beulich
2025-07-17  9:34 ` Roger Pau Monné

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7fa05d3f-2f85-4a42-9549-b68a486e30ea@suse.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=roger.pau@citrix.com \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.