Re: [Xen-devel] [PATCH v5 3/3] xen/arm: vgic_migrate_irq: do not race against GIC_IRQ_GUEST_MIGRATING

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 01/03/17 22:15, Stefano Stabellini wrote:
> A potential race condition occurs when vgic_migrate_irq is called a
> second time, while GIC_IRQ_GUEST_MIGRATING is already set. In that case,
> vgic_migrate_irq takes a different vgic lock from gic_update_one_lr.
Hmmm, vgic_migrate_irq will bail out before accessing inflight list if 
GIC_IRQ_GUEST_MIGRATING is already set:
/* migrating already in progress, no need to do anything */
if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status )
  return;

And test_bit is atomic. So I don't understand what is the corruption 
problem you mention.
> vgic_migrate_irq running concurrently with gic_update_one_lr could cause
> data corruptions, as they both access the inflight list.
>
> This patch fixes this problem. In vgic_migrate_irq after setting the new
> vcpu target, it checks both GIC_IRQ_GUEST_MIGRATING and
> GIC_IRQ_GUEST_VISIBLE. If they are both set we can just return because
> we have already set the new target: when gic_update_one_lr reaches
> the GIC_IRQ_GUEST_MIGRATING test, it will do the right thing.
>
> Otherwise, if GIC_IRQ_GUEST_MIGRATING is set but GIC_IRQ_GUEST_VISIBLE
> is not, gic_update_one_lr is running at the very same time on another
> pcpu, so it just waits until it completes (GIC_IRQ_GUEST_MIGRATING is
> cleared).
>
> Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> ---
>  xen/arch/arm/gic.c  |  5 ++++-
>  xen/arch/arm/vgic.c | 16 ++++++++++++++--
>  2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 16bb150..a805300 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -508,10 +508,13 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>               * next pcpu, inflight is already cleared. No concurrent
>               * accesses to inflight. */
>              smp_mb();
> -            if ( test_and_clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> +            if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
>              {
>                  struct vcpu *v_target = vgic_get_target_vcpu(v, irq);
>                  irq_set_affinity(p->desc, cpumask_of(v_target->processor));
> +                /* Set the new affinity, then clear MIGRATING. */
> +                smp_mb();
> +                clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
>              }
>          }
>      }
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index a323e7e..9141b34 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -252,13 +252,25 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new,
>      spin_lock_irqsave(&old->arch.vgic.lock, flags);
>      write_atomic(t_vcpu, new->vcpu_id);
>
> -    /* migration already in progress, no need to do anything */
> -    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> +    /* Set the new target, then check MIGRATING and VISIBLE, it pairs
> +     * with the barrier in gic_update_one_lr. */
> +    smp_mb();
> +
> +    /* no need to do anything, gic_update_one_lr will take care of it */
> +    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) &&
> +         test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
>      {
>          spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
>          return;
>      }
>
> +    /* gic_update_one_lr is currently running, wait until its completion */
> +    while ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> +    {
> +        cpu_relax();
> +        smp_rmb();
> +    }
> +
>      if ( list_empty(&p->inflight) )
>      {
>          irq_set_affinity(p->desc, cpumask_of(new->processor));
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel