diff mbox series

[3/3] x86: add accessors for scratch cpu mask

Message ID 20200212164949.56434-4-roger.pau@citrix.com (mailing list archive)
State Superseded
Headers show
Series x86: fixes/improvements for scratch cpumask | expand

Commit Message

Roger Pau Monné Feb. 12, 2020, 4:49 p.m. UTC
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(-)

Comments

Jan Beulich Feb. 13, 2020, 10:12 a.m. UTC | #1
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
Roger Pau Monné Feb. 13, 2020, 3:15 p.m. UTC | #2
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.
Jan Beulich Feb. 13, 2020, 3:43 p.m. UTC | #3
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
diff mbox series

Patch

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?