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

[Jan Beulich <jbeulich@suse.com>]

On 24.02.2020 11:46, Roger Pau Monne wrote:
> 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.

While I can see the reasoning (especially in light of the change
which did violate to assumption), I'm still uncertain if this isn't
"over-engineering". Andrew, do you have a clear opinion one way or
the other here?

> Move the declaration of scratch_cpumask to smp.c in order to place the
> declaration and the accessors as close as possible.

s/declaration/definition/g

> --- 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;
> +    }

I think if possible such error path adjustments would better be
avoided. And this seems feasible here: There are two entirely
independent used of the scratch mask in this function. You could
therefore put the mask above from here, and get it again further
down, or you could leverage a property of the current
implementation, plus the fact that the 2nd use doesn't involved
any "real" function calls, and avoid a 2nd get/put altogether.

Of course another question then is whether it is a good property
of the current model, i.e. whether it wouldn't be better for
"put" to actually zap the pointer, to prevent subsequent use.

> @@ -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();

Just as a remark, not necessarily as a request to change the code: I
wonder if down the road this pretty wide scope of "holding" the mask
isn't going to bite us, when a function called from here (in a range
of code not actively needing the mask) also may want to use the mask.
But of course we can make this finer grained at the point where it
might actually start mattering.

> @@ -3645,12 +3647,17 @@ long do_mmuext_op(
>                                     mask)) )
>                  rc = -EINVAL;
>              if ( unlikely(rc) )
> +            {
> +                put_scratch_cpumask();
>                  break;
> +            }

Again, instead of adjusting an error path, how about making this
have an empty statement (i.e. dropping the break) and ...

>              if ( op.cmd == MMUEXT_TLB_FLUSH_MULTI )

... having this become "else if()"?

> @@ -4384,6 +4393,9 @@ static int __do_update_va_mapping(
>          break;
>      }
>  
> +    if ( mask && mask != d->dirty_cpumask )
> +        put_scratch_cpumask();

The right side of the && here makes things feel a little fragile for
me.

> --- 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();
>      }

This, I think, could do with a little more changing:

    if ( cpu_mask )
    {
        cpumask_t *mask = get_scratch_cpumask();

        cpumask_and(mask, cpu_mask, &cpu_online_map);
        if ( !cpumask_empty(mask) )
            msg->dest32 = cpu_mask_to_apicid(mask);
        put_scratch_cpumask();
    }

This way instead of looking twice at two cpumask_t instances, the
2nd one involves just one. Thoughts?

> --- a/xen/arch/x86/smp.c
> +++ b/xen/arch/x86/smp.c
> @@ -25,6 +25,31 @@
>  #include <irq_vectors.h>
>  #include <mach_apic.h>
>  
> +DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, scratch_cpumask);
> +
> +#ifndef NDEBUG
> +cpumask_t *scratch_cpumask(bool use)
> +{
> +    static DEFINE_PER_CPU(void *, scratch_cpumask_use);
> +
> +    /*
> +     * Due to reentrancy scratch cpumask cannot be used in IRQ, #MC or #NMI
> +     * context.
> +     */
> +    BUG_ON(in_irq() || in_mc() || in_nmi());
> +
> +    if ( use && unlikely(this_cpu(scratch_cpumask_use)) )
> +    {
> +        printk("%p: scratch CPU mask already in use by %p\n",
> +               __builtin_return_address(0), this_cpu(scratch_cpumask_use));

__builtin_return_address(0) simply shows another time what ...

> +        BUG();

... this already will show. I'd suggest to drop it. Also I think
you want %ps here.

Jan

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