[Xen-devel] [PATCH 0/3] x86: fixes/improvements for scratch cpumask

[Xen-devel] [PATCH 0/3] x86: fixes/improvements for scratch cpumask

[Roger Pau Monne]

Hello,

Commit:

5500d265a2a8fa63d60c08beb549de8ec82ff7a5
x86/smp: use APIC ALLBUT destination shorthand when possible

Introduced a bogus usage of the scratch cpumask: it was used in a
function that could be called from interrupt context, and hence using
the scratch cpumask there is not safe. Patch #2 is a fix for that usage.

Patch #3 adds some debug infrastructure to make sure the scratch cpumask
is used in the right context, and hence should prevent further missuses.

Thanks, Roger.

Roger Pau Monne (3):
  x86/smp: unify header includes in smp.h
  x86/smp: use a dedicated scratch cpumask in send_IPI_mask
  x86: add accessors for scratch cpu mask

 xen/arch/x86/io_apic.c    |  6 ++++--
 xen/arch/x86/irq.c        | 13 ++++++++++---
 xen/arch/x86/mm.c         | 30 +++++++++++++++++++++---------
 xen/arch/x86/msi.c        |  4 +++-
 xen/arch/x86/smp.c        | 14 +++++++++++++-
 xen/arch/x86/smpboot.c    | 33 ++++++++++++++++++++++++++++++++-
 xen/include/asm-x86/smp.h | 15 +++++++++++----
 7 files changed, 94 insertions(+), 21 deletions(-)

-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/3] x86/smp: unify header includes in smp.h

[Roger Pau Monne]

Unify the two adjacent header includes that are both gated with ifndef
__ASSEMBLY__.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/include/asm-x86/smp.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/include/asm-x86/smp.h b/xen/include/asm-x86/smp.h
index 1aa55d41e1..92d69a5ea0 100644
--- a/xen/include/asm-x86/smp.h
+++ b/xen/include/asm-x86/smp.h
@@ -5,13 +5,10 @@
  * We need the APIC definitions automatically as part of 'smp.h'
  */
 #ifndef __ASSEMBLY__
+#include <xen/bitops.h>
 #include <xen/kernel.h>
 #include <xen/cpumask.h>
 #include <asm/current.h>
-#endif
-
-#ifndef __ASSEMBLY__
-#include <xen/bitops.h>
 #include <asm/mpspec.h>
 #endif
 
-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] x86/smp: unify header includes in smp.h

[Wei Liu]

On Wed, Feb 12, 2020 at 05:49:47PM +0100, Roger Pau Monne wrote:
> Unify the two adjacent header includes that are both gated with ifndef
> __ASSEMBLY__.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Wei Liu <wl@xen.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] x86/smp: unify header includes in smp.h

[Jan Beulich]

On 13.02.2020 00:05, Wei Liu wrote:
> On Wed, Feb 12, 2020 at 05:49:47PM +0100, Roger Pau Monne wrote:
>> Unify the two adjacent header includes that are both gated with ifndef
>> __ASSEMBLY__.
>>
>> No functional change intended.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Wei Liu <wl@xen.org>

Acked-by: Jan Beulich <jbeulich@suse.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/3] x86/smp: use a dedicated scratch cpumask in send_IPI_mask

[Roger Pau Monne]

Using scratch_cpumask in send_IPI_mak is not safe because it can be
called from interrupt context, and hence Xen would have to make sure
all the users of the scratch cpumask disable interrupts while using
it.

Instead introduce a new cpumask to be used by send_IPI_mask, and
disable interrupts while using.

Fixes: 5500d265a2a8 ('x86/smp: use APIC ALLBUT destination shorthand when possible')
Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/smp.c     | 14 +++++++++++++-
 xen/arch/x86/smpboot.c |  9 ++++++++-
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
index 9bc925616a..384c3ba924 100644
--- a/xen/arch/x86/smp.c
+++ b/xen/arch/x86/smp.c
@@ -59,6 +59,7 @@ static void send_IPI_shortcut(unsigned int shortcut, int vector,
     apic_write(APIC_ICR, cfg);
 }
 
+DECLARE_PER_CPU(cpumask_var_t, send_ipi_cpumask);
 /*
  * send_IPI_mask(cpumask, vector): sends @vector IPI to CPUs in @cpumask,
  * excluding the local CPU. @cpumask may be empty.
@@ -67,7 +68,8 @@ static void send_IPI_shortcut(unsigned int shortcut, int vector,
 void send_IPI_mask(const cpumask_t *mask, int vector)
 {
     bool cpus_locked = false;
-    cpumask_t *scratch = this_cpu(scratch_cpumask);
+    cpumask_t *scratch = this_cpu(send_ipi_cpumask);
+    unsigned long flags;
 
     /*
      * This can only be safely used when no CPU hotplug or unplug operations
@@ -81,7 +83,15 @@ void send_IPI_mask(const cpumask_t *mask, int vector)
          local_irq_is_enabled() && (cpus_locked = get_cpu_maps()) &&
          (park_offline_cpus ||
           cpumask_equal(&cpu_online_map, &cpu_present_map)) )
+    {
+        /*
+         * send_IPI_mask can be called from interrupt context, and hence we
+         * need to disable interrupts in order to protect the per-cpu
+         * send_ipi_cpumask while being used.
+         */
+        local_irq_save(flags);
         cpumask_or(scratch, mask, cpumask_of(smp_processor_id()));
+    }
     else
     {
         if ( cpus_locked )
@@ -89,6 +99,7 @@ void send_IPI_mask(const cpumask_t *mask, int vector)
             put_cpu_maps();
             cpus_locked = false;
         }
+        local_irq_save(flags);
         cpumask_clear(scratch);
     }
 
@@ -97,6 +108,7 @@ void send_IPI_mask(const cpumask_t *mask, int vector)
     else
         alternative_vcall(genapic.send_IPI_mask, mask, vector);
 
+    local_irq_restore(flags);
     if ( cpus_locked )
         put_cpu_maps();
 }
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index e83e4564a4..82e89201b3 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -57,6 +57,9 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask);
 DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, scratch_cpumask);
 static cpumask_t scratch_cpu0mask;
 
+DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, send_ipi_cpumask);
+static cpumask_t send_ipi_cpu0mask;
+
 cpumask_t cpu_online_map __read_mostly;
 EXPORT_SYMBOL(cpu_online_map);
 
@@ -930,6 +933,8 @@ static void cpu_smpboot_free(unsigned int cpu, bool remove)
         FREE_CPUMASK_VAR(per_cpu(cpu_core_mask, cpu));
         if ( per_cpu(scratch_cpumask, cpu) != &scratch_cpu0mask )
             FREE_CPUMASK_VAR(per_cpu(scratch_cpumask, cpu));
+        if ( per_cpu(send_ipi_cpumask, cpu) != &send_ipi_cpu0mask )
+            FREE_CPUMASK_VAR(per_cpu(send_ipi_cpumask, cpu));
     }
 
     cleanup_cpu_root_pgt(cpu);
@@ -1034,7 +1039,8 @@ static int cpu_smpboot_alloc(unsigned int cpu)
 
     if ( !(cond_zalloc_cpumask_var(&per_cpu(cpu_sibling_mask, cpu)) &&
            cond_zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) &&
-           cond_alloc_cpumask_var(&per_cpu(scratch_cpumask, cpu))) )
+           cond_alloc_cpumask_var(&per_cpu(scratch_cpumask, cpu)) &&
+           cond_alloc_cpumask_var(&per_cpu(send_ipi_cpumask, cpu))) )
         goto out;
 
     rc = 0;
@@ -1175,6 +1181,7 @@ void __init smp_prepare_boot_cpu(void)
     cpumask_set_cpu(cpu, &cpu_present_map);
 #if NR_CPUS > 2 * BITS_PER_LONG
     per_cpu(scratch_cpumask, cpu) = &scratch_cpu0mask;
+    per_cpu(send_ipi_cpumask, cpu) = &send_ipi_cpu0mask;
 #endif
 
     get_cpu_info()->use_pv_cr3 = false;
-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] x86/smp: use a dedicated scratch cpumask in send_IPI_mask

[Jan Beulich]

On 12.02.2020 17:49, Roger Pau Monne wrote:
> Using scratch_cpumask in send_IPI_mak is not safe because it can be
> called from interrupt context, and hence Xen would have to make sure
> all the users of the scratch cpumask disable interrupts while using
> it.
> 
> Instead introduce a new cpumask to be used by send_IPI_mask, and
> disable interrupts while using.

My first thought here was: What about NMI or #MC context? Even if
not using the function today (which I didn't check), there shouldn't
be a latent issue introduced here preventing them from doing so in
the future. Instead I think you want to allocate the scratch mask
dynamically (at least if caller context doesn't allow use of the
generic one), and simply refrain from coalescing IPIs if this
allocations fails.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] x86/smp: use a dedicated scratch cpumask in send_IPI_mask

[Roger Pau Monné]

On Thu, Feb 13, 2020 at 10:59:29AM +0100, Jan Beulich wrote:
> On 12.02.2020 17:49, Roger Pau Monne wrote:
> > Using scratch_cpumask in send_IPI_mak is not safe because it can be
> > called from interrupt context, and hence Xen would have to make sure
> > all the users of the scratch cpumask disable interrupts while using
> > it.
> > 
> > Instead introduce a new cpumask to be used by send_IPI_mask, and
> > disable interrupts while using.
> 
> My first thought here was: What about NMI or #MC context? Even if
> not using the function today (which I didn't check), there shouldn't
> be a latent issue introduced here preventing them from doing so in
> the future. Instead I think you want to allocate the scratch mask
> dynamically (at least if caller context doesn't allow use of the
> generic one), and simply refrain from coalescing IPIs if this
> allocations fails.

Hm, isn't this going to be quite expensive, and hence render the
benefit of using the shorthand moot?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] x86/smp: use a dedicated scratch cpumask in send_IPI_mask

[Jan Beulich]

On 13.02.2020 11:03, Roger Pau Monné wrote:
> On Thu, Feb 13, 2020 at 10:59:29AM +0100, Jan Beulich wrote:
>> On 12.02.2020 17:49, Roger Pau Monne wrote:
>>> Using scratch_cpumask in send_IPI_mak is not safe because it can be
>>> called from interrupt context, and hence Xen would have to make sure
>>> all the users of the scratch cpumask disable interrupts while using
>>> it.
>>>
>>> Instead introduce a new cpumask to be used by send_IPI_mask, and
>>> disable interrupts while using.
>>
>> My first thought here was: What about NMI or #MC context? Even if
>> not using the function today (which I didn't check), there shouldn't
>> be a latent issue introduced here preventing them from doing so in
>> the future. Instead I think you want to allocate the scratch mask
>> dynamically (at least if caller context doesn't allow use of the
>> generic one), and simply refrain from coalescing IPIs if this
>> allocations fails.
> 
> Hm, isn't this going to be quite expensive, and hence render the
> benefit of using the shorthand moot?

Depends on how many CPUs there are, i.e. how long the loop otherwise
would be. When xmalloc() doesn't need to turn to the page allocator,
I think it won't be overly slow. Another option would be to avoid
coalescing in a slightly different way (without having to fiddle
with the scratch mask) when called in interrupt context.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] x86/smp: use a dedicated scratch cpumask in send_IPI_mask

[Roger Pau Monné]

On Thu, Feb 13, 2020 at 11:19:02AM +0100, Jan Beulich wrote:
> On 13.02.2020 11:03, Roger Pau Monné wrote:
> > On Thu, Feb 13, 2020 at 10:59:29AM +0100, Jan Beulich wrote:
> >> On 12.02.2020 17:49, Roger Pau Monne wrote:
> >>> Using scratch_cpumask in send_IPI_mak is not safe because it can be
> >>> called from interrupt context, and hence Xen would have to make sure
> >>> all the users of the scratch cpumask disable interrupts while using
> >>> it.
> >>>
> >>> Instead introduce a new cpumask to be used by send_IPI_mask, and
> >>> disable interrupts while using.
> >>
> >> My first thought here was: What about NMI or #MC context? Even if
> >> not using the function today (which I didn't check), there shouldn't
> >> be a latent issue introduced here preventing them from doing so in
> >> the future. Instead I think you want to allocate the scratch mask
> >> dynamically (at least if caller context doesn't allow use of the
> >> generic one), and simply refrain from coalescing IPIs if this
> >> allocations fails.
> > 
> > Hm, isn't this going to be quite expensive, and hence render the
> > benefit of using the shorthand moot?
> 
> Depends on how many CPUs there are, i.e. how long the loop otherwise
> would be. When xmalloc() doesn't need to turn to the page allocator,
> I think it won't be overly slow. Another option would be to avoid
> coalescing in a slightly different way (without having to fiddle
> with the scratch mask) when called in interrupt context.

What about preventing the mask usage when in nmi context?

I could introduce something like in_nmi and just avoid the scratch
mask usage in that case (and the shorthand).

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] x86/smp: use a dedicated scratch cpumask in send_IPI_mask

[Jan Beulich]

On 13.02.2020 12:41, Roger Pau Monné wrote:
> On Thu, Feb 13, 2020 at 11:19:02AM +0100, Jan Beulich wrote:
>> On 13.02.2020 11:03, Roger Pau Monné wrote:
>>> On Thu, Feb 13, 2020 at 10:59:29AM +0100, Jan Beulich wrote:
>>>> On 12.02.2020 17:49, Roger Pau Monne wrote:
>>>>> Using scratch_cpumask in send_IPI_mak is not safe because it can be
>>>>> called from interrupt context, and hence Xen would have to make sure
>>>>> all the users of the scratch cpumask disable interrupts while using
>>>>> it.
>>>>>
>>>>> Instead introduce a new cpumask to be used by send_IPI_mask, and
>>>>> disable interrupts while using.
>>>>
>>>> My first thought here was: What about NMI or #MC context? Even if
>>>> not using the function today (which I didn't check), there shouldn't
>>>> be a latent issue introduced here preventing them from doing so in
>>>> the future. Instead I think you want to allocate the scratch mask
>>>> dynamically (at least if caller context doesn't allow use of the
>>>> generic one), and simply refrain from coalescing IPIs if this
>>>> allocations fails.
>>>
>>> Hm, isn't this going to be quite expensive, and hence render the
>>> benefit of using the shorthand moot?
>>
>> Depends on how many CPUs there are, i.e. how long the loop otherwise
>> would be. When xmalloc() doesn't need to turn to the page allocator,
>> I think it won't be overly slow. Another option would be to avoid
>> coalescing in a slightly different way (without having to fiddle
>> with the scratch mask) when called in interrupt context.
> 
> What about preventing the mask usage when in nmi context?
> 
> I could introduce something like in_nmi and just avoid the scratch
> mask usage in that case (and the shorthand).

Right, allocation isn't permitted in NMI context anyway. As to
#MC context - I'm afraid we don't have any indicator of this so
far, which is a problem (here and maybe also elsewhere).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 3/3] x86: add accessors for scratch cpu mask

[Roger Pau Monne]

Current usage of the per-CPU scratch cpumask is dangerous since
there's no way to figure out if the mask is already being used except
for manual code inspection of all the callers and possible call paths.

This is unsafe and not reliable, so introduce a minimal get/put
infrastructure to prevent nested usage of the scratch mask and usage
in interrupt context.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/io_apic.c    |  6 ++++--
 xen/arch/x86/irq.c        | 13 ++++++++++---
 xen/arch/x86/mm.c         | 30 +++++++++++++++++++++---------
 xen/arch/x86/msi.c        |  4 +++-
 xen/arch/x86/smpboot.c    | 24 ++++++++++++++++++++++++
 xen/include/asm-x86/smp.h | 10 ++++++++++
 6 files changed, 72 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index e98e08e9c8..4ee261b632 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2236,10 +2236,11 @@ int io_apic_set_pci_routing (int ioapic, int pin, int irq, int edge_level, int a
     entry.vector = vector;
 
     if (cpumask_intersects(desc->arch.cpu_mask, TARGET_CPUS)) {
-        cpumask_t *mask = this_cpu(scratch_cpumask);
+        cpumask_t *mask = get_scratch_cpumask();
 
         cpumask_and(mask, desc->arch.cpu_mask, TARGET_CPUS);
         SET_DEST(entry, logical, cpu_mask_to_apicid(mask));
+        put_scratch_cpumask();
     } else {
         printk(XENLOG_ERR "IRQ%d: no target CPU (%*pb vs %*pb)\n",
                irq, CPUMASK_PR(desc->arch.cpu_mask), CPUMASK_PR(TARGET_CPUS));
@@ -2433,10 +2434,11 @@ int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val)
 
     if ( cpumask_intersects(desc->arch.cpu_mask, TARGET_CPUS) )
     {
-        cpumask_t *mask = this_cpu(scratch_cpumask);
+        cpumask_t *mask = get_scratch_cpumask();
 
         cpumask_and(mask, desc->arch.cpu_mask, TARGET_CPUS);
         SET_DEST(rte, logical, cpu_mask_to_apicid(mask));
+        put_scratch_cpumask();
     }
     else
     {
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index cc2eb8e925..7ecf5376e3 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -196,7 +196,7 @@ static void _clear_irq_vector(struct irq_desc *desc)
 {
     unsigned int cpu, old_vector, irq = desc->irq;
     unsigned int vector = desc->arch.vector;
-    cpumask_t *tmp_mask = this_cpu(scratch_cpumask);
+    cpumask_t *tmp_mask = get_scratch_cpumask();
 
     BUG_ON(!valid_irq_vector(vector));
 
@@ -223,7 +223,10 @@ static void _clear_irq_vector(struct irq_desc *desc)
     trace_irq_mask(TRC_HW_IRQ_CLEAR_VECTOR, irq, vector, tmp_mask);
 
     if ( likely(!desc->arch.move_in_progress) )
+    {
+        put_scratch_cpumask();
         return;
+    }
 
     /* If we were in motion, also clear desc->arch.old_vector */
     old_vector = desc->arch.old_vector;
@@ -236,6 +239,7 @@ static void _clear_irq_vector(struct irq_desc *desc)
         per_cpu(vector_irq, cpu)[old_vector] = ~irq;
     }
 
+    put_scratch_cpumask();
     release_old_vec(desc);
 
     desc->arch.move_in_progress = 0;
@@ -1152,10 +1156,11 @@ static void irq_guest_eoi_timer_fn(void *data)
         break;
 
     case ACKTYPE_EOI:
-        cpu_eoi_map = this_cpu(scratch_cpumask);
+        cpu_eoi_map = get_scratch_cpumask();
         cpumask_copy(cpu_eoi_map, action->cpu_eoi_map);
         spin_unlock_irq(&desc->lock);
         on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0);
+        put_scratch_cpumask();
         return;
     }
 
@@ -2531,12 +2536,12 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
     unsigned int irq;
     static int warned;
     struct irq_desc *desc;
+    cpumask_t *affinity = get_scratch_cpumask();
 
     for ( irq = 0; irq < nr_irqs; irq++ )
     {
         bool break_affinity = false, set_affinity = true;
         unsigned int vector;
-        cpumask_t *affinity = this_cpu(scratch_cpumask);
 
         if ( irq == 2 )
             continue;
@@ -2640,6 +2645,8 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
                    irq, CPUMASK_PR(affinity));
     }
 
+    put_scratch_cpumask();
+
     /* That doesn't seem sufficient.  Give it 1ms. */
     local_irq_enable();
     mdelay(1);
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 9b33829084..bded19717b 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1271,7 +1271,7 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner)
              (l1e_owner == pg_owner) )
         {
             struct vcpu *v;
-            cpumask_t *mask = this_cpu(scratch_cpumask);
+            cpumask_t *mask = get_scratch_cpumask();
 
             cpumask_clear(mask);
 
@@ -1288,6 +1288,7 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner)
 
             if ( !cpumask_empty(mask) )
                 flush_tlb_mask(mask);
+            put_scratch_cpumask();
         }
 #endif /* CONFIG_PV_LDT_PAGING */
         put_page(page);
@@ -2912,7 +2913,7 @@ static int _get_page_type(struct page_info *page, unsigned long type,
                  * vital that no other CPUs are left with mappings of a frame
                  * which is about to become writeable to the guest.
                  */
-                cpumask_t *mask = this_cpu(scratch_cpumask);
+                cpumask_t *mask = get_scratch_cpumask();
 
                 BUG_ON(in_irq());
                 cpumask_copy(mask, d->dirty_cpumask);
@@ -2928,6 +2929,7 @@ static int _get_page_type(struct page_info *page, unsigned long type,
                     perfc_incr(need_flush_tlb_flush);
                     flush_tlb_mask(mask);
                 }
+                put_scratch_cpumask();
 
                 /* We lose existing type and validity. */
                 nx &= ~(PGT_type_mask | PGT_validated);
@@ -3644,7 +3646,7 @@ long do_mmuext_op(
         case MMUEXT_TLB_FLUSH_MULTI:
         case MMUEXT_INVLPG_MULTI:
         {
-            cpumask_t *mask = this_cpu(scratch_cpumask);
+            cpumask_t *mask = get_scratch_cpumask();
 
             if ( unlikely(currd != pg_owner) )
                 rc = -EPERM;
@@ -3654,12 +3656,17 @@ long do_mmuext_op(
                                    mask)) )
                 rc = -EINVAL;
             if ( unlikely(rc) )
+            {
+                put_scratch_cpumask();
                 break;
+            }
 
             if ( op.cmd == MMUEXT_TLB_FLUSH_MULTI )
                 flush_tlb_mask(mask);
             else if ( __addr_ok(op.arg1.linear_addr) )
                 flush_tlb_one_mask(mask, op.arg1.linear_addr);
+            put_scratch_cpumask();
+
             break;
         }
 
@@ -3692,7 +3699,7 @@ long do_mmuext_op(
             else if ( likely(cache_flush_permitted(currd)) )
             {
                 unsigned int cpu;
-                cpumask_t *mask = this_cpu(scratch_cpumask);
+                cpumask_t *mask = get_scratch_cpumask();
 
                 cpumask_clear(mask);
                 for_each_online_cpu(cpu)
@@ -3700,6 +3707,7 @@ long do_mmuext_op(
                                              per_cpu(cpu_sibling_mask, cpu)) )
                         __cpumask_set_cpu(cpu, mask);
                 flush_mask(mask, FLUSH_CACHE);
+                put_scratch_cpumask();
             }
             else
                 rc = -EINVAL;
@@ -4165,12 +4173,13 @@ long do_mmu_update(
          * Force other vCPU-s of the affected guest to pick up L4 entry
          * changes (if any).
          */
-        unsigned int cpu = smp_processor_id();
-        cpumask_t *mask = per_cpu(scratch_cpumask, cpu);
+        cpumask_t *mask = get_scratch_cpumask();
 
-        cpumask_andnot(mask, pt_owner->dirty_cpumask, cpumask_of(cpu));
+        cpumask_andnot(mask, pt_owner->dirty_cpumask,
+                       cpumask_of(smp_processor_id()));
         if ( !cpumask_empty(mask) )
             flush_mask(mask, FLUSH_TLB_GLOBAL | FLUSH_ROOT_PGTBL);
+        put_scratch_cpumask();
     }
 
     perfc_add(num_page_updates, i);
@@ -4361,7 +4370,7 @@ static int __do_update_va_mapping(
             mask = d->dirty_cpumask;
             break;
         default:
-            mask = this_cpu(scratch_cpumask);
+            mask = get_scratch_cpumask();
             rc = vcpumask_to_pcpumask(d, const_guest_handle_from_ptr(bmap_ptr,
                                                                      void),
                                       mask);
@@ -4381,7 +4390,7 @@ static int __do_update_va_mapping(
             mask = d->dirty_cpumask;
             break;
         default:
-            mask = this_cpu(scratch_cpumask);
+            mask = get_scratch_cpumask();
             rc = vcpumask_to_pcpumask(d, const_guest_handle_from_ptr(bmap_ptr,
                                                                      void),
                                       mask);
@@ -4392,6 +4401,9 @@ static int __do_update_va_mapping(
         break;
     }
 
+    if ( mask && mask != d->dirty_cpumask )
+        put_scratch_cpumask();
+
     return rc;
 }
 
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index c85cf9f85a..1ec1cc51d3 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -159,13 +159,15 @@ void msi_compose_msg(unsigned vector, const cpumask_t *cpu_mask, struct msi_msg
 
     if ( cpu_mask )
     {
-        cpumask_t *mask = this_cpu(scratch_cpumask);
+        cpumask_t *mask;
 
         if ( !cpumask_intersects(cpu_mask, &cpu_online_map) )
             return;
 
+        mask = get_scratch_cpumask();
         cpumask_and(mask, cpu_mask, &cpu_online_map);
         msg->dest32 = cpu_mask_to_apicid(mask);
+        put_scratch_cpumask();
     }
 
     msg->address_hi = MSI_ADDR_BASE_HI;
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 82e89201b3..67ee490f94 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -57,6 +57,30 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask);
 DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, scratch_cpumask);
 static cpumask_t scratch_cpu0mask;
 
+#ifndef NDEBUG
+cpumask_t *scratch_cpumask(const char *fn)
+{
+    static DEFINE_PER_CPU(const char *, scratch_cpumask_use);
+
+    /*
+     * Scratch cpumask cannot be used in IRQ context, or else we would have to
+     * make sure all users have interrupts disabled while using the scratch
+     * mask.
+     */
+    BUG_ON(in_irq());
+
+    if ( fn && unlikely(this_cpu(scratch_cpumask_use)) )
+    {
+        printk("%s: scratch CPU mask already in use by %s\n",
+              fn, this_cpu(scratch_cpumask_use));
+        BUG();
+    }
+    this_cpu(scratch_cpumask_use) = fn;
+
+    return fn ? this_cpu(scratch_cpumask) : NULL;
+}
+#endif
+
 DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, send_ipi_cpumask);
 static cpumask_t send_ipi_cpu0mask;
 
diff --git a/xen/include/asm-x86/smp.h b/xen/include/asm-x86/smp.h
index 92d69a5ea0..28f2044bb7 100644
--- a/xen/include/asm-x86/smp.h
+++ b/xen/include/asm-x86/smp.h
@@ -23,6 +23,16 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_sibling_mask);
 DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask);
 DECLARE_PER_CPU(cpumask_var_t, scratch_cpumask);
 
+#ifndef NDEBUG
+/* Not to be called directly, use {get/put}_scratch_cpumask(). */
+cpumask_t *scratch_cpumask(const char *fn);
+#define get_scratch_cpumask() scratch_cpumask(__func__)
+#define put_scratch_cpumask() ((void)scratch_cpumask(NULL))
+#else
+#define get_scratch_cpumask() this_cpu(scratch_cpumask)
+#define put_scratch_cpumask()
+#endif
+
 /*
  * Do we, for platform reasons, need to actually keep CPUs online when we
  * would otherwise prefer them to be off?
-- 
2.25.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] x86: add accessors for scratch cpu mask

[Jan Beulich]

On 12.02.2020 17:49, Roger Pau Monne wrote:
> @@ -223,7 +223,10 @@ static void _clear_irq_vector(struct irq_desc *desc)
>      trace_irq_mask(TRC_HW_IRQ_CLEAR_VECTOR, irq, vector, tmp_mask);
>  
>      if ( likely(!desc->arch.move_in_progress) )
> +    {
> +        put_scratch_cpumask();
>          return;
> +    }

I'm not overly happy to see a need introduced to do cleanup like
this one, but at least missing a path is a debug-build problem
only.

> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -57,6 +57,30 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask);
>  DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, scratch_cpumask);
>  static cpumask_t scratch_cpu0mask;
>  
> +#ifndef NDEBUG
> +cpumask_t *scratch_cpumask(const char *fn)

Please don't pass an argument that you can deduce, and then
provide even more meaningful data:

> +{
> +    static DEFINE_PER_CPU(const char *, scratch_cpumask_use);
> +
> +    /*
> +     * Scratch cpumask cannot be used in IRQ context, or else we would have to
> +     * make sure all users have interrupts disabled while using the scratch
> +     * mask.
> +     */
> +    BUG_ON(in_irq());
> +
> +    if ( fn && unlikely(this_cpu(scratch_cpumask_use)) )
> +    {
> +        printk("%s: scratch CPU mask already in use by %s\n",
> +              fn, this_cpu(scratch_cpumask_use));

Use __builtin_return_address(0) here, which will allow
identifying which of perhaps multiple uses in a function is
the offending one.

Also, why in smpboot.c instead of in smp.c? This isn't a
boot or CPU-hot-online related function afaict.

Finally, it would seem nice if multiple uses by the same caller
could be permitted:

    for ( ... )
    {
        if ( ... )
        {
            mask = get_scratch_cpumask();
            ...
        }
        else
        {
            /* no use of get_scratch_cpumask() */
            ...
        }
    }

    put_scratch_cpumask();

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] x86: add accessors for scratch cpu mask

[Roger Pau Monné]

On Thu, Feb 13, 2020 at 11:12:12AM +0100, Jan Beulich wrote:
> On 12.02.2020 17:49, Roger Pau Monne wrote:
> > @@ -223,7 +223,10 @@ static void _clear_irq_vector(struct irq_desc *desc)
> >      trace_irq_mask(TRC_HW_IRQ_CLEAR_VECTOR, irq, vector, tmp_mask);
> >  
> >      if ( likely(!desc->arch.move_in_progress) )
> > +    {
> > +        put_scratch_cpumask();
> >          return;
> > +    }
> 
> I'm not overly happy to see a need introduced to do cleanup like
> this one, but at least missing a path is a debug-build problem
> only.
> 
> > --- a/xen/arch/x86/smpboot.c
> > +++ b/xen/arch/x86/smpboot.c
> > @@ -57,6 +57,30 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask);
> >  DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, scratch_cpumask);
> >  static cpumask_t scratch_cpu0mask;
> >  
> > +#ifndef NDEBUG
> > +cpumask_t *scratch_cpumask(const char *fn)
> 
> Please don't pass an argument that you can deduce, and then
> provide even more meaningful data:
> 
> > +{
> > +    static DEFINE_PER_CPU(const char *, scratch_cpumask_use);
> > +
> > +    /*
> > +     * Scratch cpumask cannot be used in IRQ context, or else we would have to
> > +     * make sure all users have interrupts disabled while using the scratch
> > +     * mask.
> > +     */
> > +    BUG_ON(in_irq());
> > +
> > +    if ( fn && unlikely(this_cpu(scratch_cpumask_use)) )
> > +    {
> > +        printk("%s: scratch CPU mask already in use by %s\n",
> > +              fn, this_cpu(scratch_cpumask_use));
> 
> Use __builtin_return_address(0) here, which will allow
> identifying which of perhaps multiple uses in a function is
> the offending one.

Will change.

> 
> Also, why in smpboot.c instead of in smp.c? This isn't a
> boot or CPU-hot-online related function afaict.

I've added it to smpboot.c because that's where scratch_cpumask is
defined. I could move it to smp.c, but I would prefer to keep the
accessor as close as possible to the declaration.

> 
> Finally, it would seem nice if multiple uses by the same caller
> could be permitted:
> 
>     for ( ... )
>     {
>         if ( ... )
>         {
>             mask = get_scratch_cpumask();
>             ...
>         }
>         else
>         {
>             /* no use of get_scratch_cpumask() */
>             ...
>         }
>     }
> 
>     put_scratch_cpumask();

I have to admit I don't really like this kinds of asymmetric
constructions, what you suggest for example won't be valid if
get_scratch_cpumask took some kind of lock.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] x86: add accessors for scratch cpu mask

[Jan Beulich]

On 13.02.2020 16:15, Roger Pau Monné wrote:
> On Thu, Feb 13, 2020 at 11:12:12AM +0100, Jan Beulich wrote:
>> On 12.02.2020 17:49, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/smpboot.c
>>> +++ b/xen/arch/x86/smpboot.c
>>> @@ -57,6 +57,30 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask);
>>>  DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, scratch_cpumask);
>>>  static cpumask_t scratch_cpu0mask;
>>>  
>>> +#ifndef NDEBUG
>>> +cpumask_t *scratch_cpumask(const char *fn)
>>
>> Please don't pass an argument that you can deduce, and then
>> provide even more meaningful data:
>>
>>> +{
>>> +    static DEFINE_PER_CPU(const char *, scratch_cpumask_use);
>>> +
>>> +    /*
>>> +     * Scratch cpumask cannot be used in IRQ context, or else we would have to
>>> +     * make sure all users have interrupts disabled while using the scratch
>>> +     * mask.
>>> +     */
>>> +    BUG_ON(in_irq());
>>> +
>>> +    if ( fn && unlikely(this_cpu(scratch_cpumask_use)) )
>>> +    {
>>> +        printk("%s: scratch CPU mask already in use by %s\n",
>>> +              fn, this_cpu(scratch_cpumask_use));
>>
>> Use __builtin_return_address(0) here, which will allow
>> identifying which of perhaps multiple uses in a function is
>> the offending one.
> 
> Will change.
> 
>>
>> Also, why in smpboot.c instead of in smp.c? This isn't a
>> boot or CPU-hot-online related function afaict.
> 
> I've added it to smpboot.c because that's where scratch_cpumask is
> defined. I could move it to smp.c, but I would prefer to keep the
> accessor as close as possible to the declaration.

May I suggest then to move the definition of the symbol?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/3] x86: fixes/improvements for scratch cpumask

[Sander Eikelenboom]

On 12/02/2020 17:49, Roger Pau Monne wrote:
> Hello,
> 
> Commit:
> 
> 5500d265a2a8fa63d60c08beb549de8ec82ff7a5
> x86/smp: use APIC ALLBUT destination shorthand when possible
> 
> Introduced a bogus usage of the scratch cpumask: it was used in a
> function that could be called from interrupt context, and hence using
> the scratch cpumask there is not safe. Patch #2 is a fix for that usage.
> 
> Patch #3 adds some debug infrastructure to make sure the scratch cpumask
> is used in the right context, and hence should prevent further missuses.
> 
> Thanks, Roger.

Hi Roger,

Do you still want me to test the "panic" patch ?
Or test this series instead ?

--
Sander


> Roger Pau Monne (3):
>   x86/smp: unify header includes in smp.h
>   x86/smp: use a dedicated scratch cpumask in send_IPI_mask
>   x86: add accessors for scratch cpu mask
> 
>  xen/arch/x86/io_apic.c    |  6 ++++--
>  xen/arch/x86/irq.c        | 13 ++++++++++---
>  xen/arch/x86/mm.c         | 30 +++++++++++++++++++++---------
>  xen/arch/x86/msi.c        |  4 +++-
>  xen/arch/x86/smp.c        | 14 +++++++++++++-
>  xen/arch/x86/smpboot.c    | 33 ++++++++++++++++++++++++++++++++-
>  xen/include/asm-x86/smp.h | 15 +++++++++++----
>  7 files changed, 94 insertions(+), 21 deletions(-)
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/3] x86: fixes/improvements for scratch cpumask

[Roger Pau Monné]

On Wed, Feb 12, 2020 at 05:53:39PM +0100, Sander Eikelenboom wrote:
> On 12/02/2020 17:49, Roger Pau Monne wrote:
> > Hello,
> > 
> > Commit:
> > 
> > 5500d265a2a8fa63d60c08beb549de8ec82ff7a5
> > x86/smp: use APIC ALLBUT destination shorthand when possible
> > 
> > Introduced a bogus usage of the scratch cpumask: it was used in a
> > function that could be called from interrupt context, and hence using
> > the scratch cpumask there is not safe. Patch #2 is a fix for that usage.
> > 
> > Patch #3 adds some debug infrastructure to make sure the scratch cpumask
> > is used in the right context, and hence should prevent further missuses.
> > 
> > Thanks, Roger.
> 
> Hi Roger,
> 
> Do you still want me to test the "panic" patch ?
> Or test this series instead ?

I've been able to trigger this myself, so if you can give a try to the
series in order to assert it fixes your issue that would be great.

Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/3] x86: fixes/improvements for scratch cpumask

[Sander Eikelenboom]

On 12/02/2020 18:01, Roger Pau Monné wrote:
> On Wed, Feb 12, 2020 at 05:53:39PM +0100, Sander Eikelenboom wrote:
>> On 12/02/2020 17:49, Roger Pau Monne wrote:
>>> Hello,
>>>
>>> Commit:
>>>
>>> 5500d265a2a8fa63d60c08beb549de8ec82ff7a5
>>> x86/smp: use APIC ALLBUT destination shorthand when possible
>>>
>>> Introduced a bogus usage of the scratch cpumask: it was used in a
>>> function that could be called from interrupt context, and hence using
>>> the scratch cpumask there is not safe. Patch #2 is a fix for that usage.
>>>
>>> Patch #3 adds some debug infrastructure to make sure the scratch cpumask
>>> is used in the right context, and hence should prevent further missuses.
>>>
>>> Thanks, Roger.
>>
>> Hi Roger,
>>
>> Do you still want me to test the "panic" patch ?
>> Or test this series instead ?
> 
> I've been able to trigger this myself, so if you can give a try to the
> series in order to assert it fixes your issue that would be great.
> 
> Thanks.
> 

Sure, compiling now, will report back tomorrow morning.
--
Sander

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/3] x86: fixes/improvements for scratch cpumask

[Sander Eikelenboom]

On 12/02/2020 18:13, Sander Eikelenboom wrote:
> On 12/02/2020 18:01, Roger Pau Monné wrote:
>> On Wed, Feb 12, 2020 at 05:53:39PM +0100, Sander Eikelenboom wrote:
>>> On 12/02/2020 17:49, Roger Pau Monne wrote:
>>>> Hello,
>>>>
>>>> Commit:
>>>>
>>>> 5500d265a2a8fa63d60c08beb549de8ec82ff7a5
>>>> x86/smp: use APIC ALLBUT destination shorthand when possible
>>>>
>>>> Introduced a bogus usage of the scratch cpumask: it was used in a
>>>> function that could be called from interrupt context, and hence using
>>>> the scratch cpumask there is not safe. Patch #2 is a fix for that usage.
>>>>
>>>> Patch #3 adds some debug infrastructure to make sure the scratch cpumask
>>>> is used in the right context, and hence should prevent further missuses.
>>>>
>>>> Thanks, Roger.
>>>
>>> Hi Roger,
>>>
>>> Do you still want me to test the "panic" patch ?
>>> Or test this series instead ?
>>
>> I've been able to trigger this myself, so if you can give a try to the
>> series in order to assert it fixes your issue that would be great.
>>
>> Thanks.
>>
> 
> Sure, compiling now, will report back tomorrow morning.
> --
> Sander
> 

Haven't seen the issue yet, so it seems fixed.
Thanks !
--
Sander

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/3] x86: fixes/improvements for scratch cpumask

[Julien Grall]

On 12/02/2020 17:49, Roger Pau Monne wrote:
> Hello,

Hi Roger,

> Commit:
> 
> 5500d265a2a8fa63d60c08beb549de8ec82ff7a5
> x86/smp: use APIC ALLBUT destination shorthand when possible

There is a more subtle problem introduced by this patch. I thought I 
would mention it here just in case this affect the approach you have 
chosen in this series.

get_cpu_maps() is used by stop_machine_run() to serialize the callers. 
If the latter fails to acquire the lock, it will bail out. 
Unfortunately, rcu_barrier() is implemented using stop_machine_run() and 
will be turned to pretty much a NOP if the latter fails (e.g the lock 
cannot be acquired).

This means that the rcu_barrier() will not do the expected job and 
potentially introduce unknown issues (e.g use-after-free...).

Before your patch, it would have been pretty hard to hit the problem 
above. After, you can race more easily with rcu_barrier() as sending IPI 
is pretty common.

Sadly, I don't have a suggestion yet how to fix this problem.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/3] x86: fixes/improvements for scratch cpumask

[Jan Beulich]

On 12.02.2020 22:05, Julien Grall wrote:
> On 12/02/2020 17:49, Roger Pau Monne wrote:
>> Commit:
>>
>> 5500d265a2a8fa63d60c08beb549de8ec82ff7a5
>> x86/smp: use APIC ALLBUT destination shorthand when possible
> 
> There is a more subtle problem introduced by this patch. I thought I 
> would mention it here just in case this affect the approach you have 
> chosen in this series.
> 
> get_cpu_maps() is used by stop_machine_run() to serialize the callers. 
> If the latter fails to acquire the lock, it will bail out. 
> Unfortunately, rcu_barrier() is implemented using stop_machine_run() and 
> will be turned to pretty much a NOP if the latter fails (e.g the lock 
> cannot be acquired).
> 
> This means that the rcu_barrier() will not do the expected job and 
> potentially introduce unknown issues (e.g use-after-free...).
> 
> Before your patch, it would have been pretty hard to hit the problem 
> above. After, you can race more easily with rcu_barrier() as sending IPI 
> is pretty common.
> 
> Sadly, I don't have a suggestion yet how to fix this problem.

A frequent use like on the IPI sending path suggests the lock may
want to become an r/w one, where both parties you mention want to
acquire it in read mode.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/3] x86: fixes/improvements for scratch cpumask

[Roger Pau Monné]

On Thu, Feb 13, 2020 at 10:53:43AM +0100, Jan Beulich wrote:
> On 12.02.2020 22:05, Julien Grall wrote:
> > On 12/02/2020 17:49, Roger Pau Monne wrote:
> >> Commit:
> >>
> >> 5500d265a2a8fa63d60c08beb549de8ec82ff7a5
> >> x86/smp: use APIC ALLBUT destination shorthand when possible
> > 
> > There is a more subtle problem introduced by this patch. I thought I 
> > would mention it here just in case this affect the approach you have 
> > chosen in this series.
> > 
> > get_cpu_maps() is used by stop_machine_run() to serialize the callers. 
> > If the latter fails to acquire the lock, it will bail out. 
> > Unfortunately, rcu_barrier() is implemented using stop_machine_run() and 
> > will be turned to pretty much a NOP if the latter fails (e.g the lock 
> > cannot be acquired).
> > 
> > This means that the rcu_barrier() will not do the expected job and 
> > potentially introduce unknown issues (e.g use-after-free...).
> > 
> > Before your patch, it would have been pretty hard to hit the problem 
> > above. After, you can race more easily with rcu_barrier() as sending IPI 
> > is pretty common.
> > 
> > Sadly, I don't have a suggestion yet how to fix this problem.
> 
> A frequent use like on the IPI sending path suggests the lock may
> want to become an r/w one, where both parties you mention want to
> acquire it in read mode.

I'm already preparing a patch in this direction.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel