[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 2/2] evtchn/fifo: don't corrupt queues if an old tail is linked



>>> On 10.12.13 at 14:57, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
> From: David Vrabel <david.vrabel@xxxxxxxxxx>
> 
> An event may still be the tail of a queue even if the queue is now
> empty (an 'old tail' event).  There is logic to handle the case when
> this old tail event needs to be added to the now empty queue (by
> checking for q->tail == port).
> 
> However, this does not cover all cases.
> 
> 1. An old tail may be re-added simultaneously with another event.
>    LINKED is set on the old tail, and the other CPU may misinterpret
>    this as the old tail still being valid and set LINK instead of
>    HEAD.  All events on this queue will then be lost.
> 
> 2. If the old tail event on queue A is moved to a different queue B
>    (by changing its VCPU or priority), the event may then be linked
>    onto queue B.  When another event is linked onto queue A it will
>    check the old tail, see that it is linked (but on queue B) and
>    overwrite the LINK field, corrupting both queues.
> 
> When an event is linked, save the vcpu id and priority of the queue it
> is being linked onto.  Use this when linking an event to check if it
> is an unlinked old tail event.  If it is an old tail event, the old
> queue is empty and old_q->tail is invalidated to ensure adding another
> event to old_q will update HEAD.  The tail is invalidated by setting
> it to 0 since the event 0 is never linked.
> 
> The old_q->lock is held while setting LINKED to avoid the race with
> the test of LINKED in evtchn_fifo_set_link().
> 
> Since a event channel may move queues after old_q->lock is acquired,
> we must check that we have the correct lock and retry if not.  Since
> changing VCPUs or priority is expected to be rare events that are
> serialized in the guest, we try at most 3 times before dropping the
> event.  This prevents a malicious guest from repeatedly adjusting
> priority to prevent another domain from acquiring old_q->lock.
> 
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> ---
>  xen/common/event_fifo.c |   80 
> ++++++++++++++++++++++++++++++++++++++++-------
>  xen/include/xen/sched.h |    2 +
>  2 files changed, 70 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
> index 2ab4c29..fc43e62 100644
> --- a/xen/common/event_fifo.c
> +++ b/xen/common/event_fifo.c
> @@ -50,6 +50,36 @@ static void evtchn_fifo_init(struct domain *d, struct 
> evtchn *evtchn)
>                   d->domain_id, evtchn->port);
>  }
>  
> +static struct evtchn_fifo_queue *lock_old_queue(const struct domain *d,
> +                                                struct evtchn *evtchn,
> +                                                unsigned long *flags)
> +{
> +    struct vcpu *v;
> +    struct evtchn_fifo_queue *q, *old_q;
> +    unsigned int try;
> +
> +    for ( try = 0; try < 3; try++ )
> +    {
> +        v = d->vcpu[evtchn->last_vcpu_id];
> +        old_q = &v->evtchn_fifo->queue[evtchn->last_priority];
> +
> +        spin_lock_irqsave(&old_q->lock, *flags);
> +
> +        v = d->vcpu[evtchn->last_vcpu_id];
> +        q = &v->evtchn_fifo->queue[evtchn->last_priority];
> +
> +        if ( old_q == q )
> +            return old_q;
> +
> +        spin_unlock_irqrestore(&old_q->lock, *flags);
> +    }
> +
> +    gdprintk(XENLOG_WARNING,
> +             "domain %d, port %d lost event (too many queue changes)\n",
> +             d->domain_id, evtchn->port);
> +    return NULL;
> +}          
> +
>  static int try_set_link(event_word_t *word, event_word_t *w, uint32_t link)
>  {
>      event_word_t new, old;
> @@ -119,7 +149,6 @@ static void evtchn_fifo_set_pending(struct vcpu *v, 
> struct evtchn *evtchn)
>      struct domain *d = v->domain;
>      unsigned int port;
>      event_word_t *word;
> -    struct evtchn_fifo_queue *q;
>      unsigned long flags;
>      bool_t was_pending;
>  
> @@ -136,25 +165,52 @@ static void evtchn_fifo_set_pending(struct vcpu *v, 
> struct evtchn *evtchn)
>          return;
>      }
>  
> -    /*
> -     * No locking around getting the queue. This may race with
> -     * changing the priority but we are allowed to signal the event
> -     * once on the old priority.
> -     */
> -    q = &v->evtchn_fifo->queue[evtchn->priority];
> -
>      was_pending = test_and_set_bit(EVTCHN_FIFO_PENDING, word);
>  
>      /*
>       * Link the event if it unmasked and not already linked.
>       */
>      if ( !test_bit(EVTCHN_FIFO_MASKED, word)
> -         && !test_and_set_bit(EVTCHN_FIFO_LINKED, word) )
> +         && !test_bit(EVTCHN_FIFO_LINKED, word) )
>      {
> +        struct evtchn_fifo_queue *q, *old_q;
>          event_word_t *tail_word;
>          bool_t linked = 0;
>  
> -        spin_lock_irqsave(&q->lock, flags);
> +        /*
> +         * No locking around getting the queue. This may race with
> +         * changing the priority but we are allowed to signal the
> +         * event once on the old priority.
> +         */
> +        q = &v->evtchn_fifo->queue[evtchn->priority];
> +
> +        old_q = lock_old_queue(d, evtchn, &flags);
> +        if ( !old_q )
> +            goto done;
> +
> +        if ( test_and_set_bit(EVTCHN_FIFO_LINKED, word) )
> +        {
> +            spin_unlock_irqrestore(&old_q->lock, flags);
> +            goto done;
> +        }
> +
> +        /*
> +         * If this event was a tail, the old queue is now empty and
> +         * its tail must be invalidated to prevent adding an event to
> +         * the old queue from corrupting the new queue.
> +         */
> +        if ( old_q->tail == port )
> +            old_q->tail = 0;
> +
> +        /* Moved to a different queue? */
> +        if ( old_q != q )
> +        {
> +            evtchn->last_vcpu_id = evtchn->notify_vcpu_id;
> +            evtchn->last_priority = evtchn->priority;
> +
> +            spin_unlock_irqrestore(&old_q->lock, flags);
> +            spin_lock_irqsave(&q->lock, flags);
> +        }
>  
>          /*
>           * Atomically link the tail to port iff the tail is linked.
> @@ -166,7 +222,7 @@ static void evtchn_fifo_set_pending(struct vcpu *v, 
> struct evtchn *evtchn)
>           * If the queue is empty (i.e., we haven't linked to the new
>           * event), head must be updated.
>           */
> -        if ( port != q->tail )
> +        if ( q->tail )
>          {
>              tail_word = evtchn_fifo_word_from_port(d, q->tail);
>              linked = evtchn_fifo_set_link(d, tail_word, port);
> @@ -182,7 +238,7 @@ static void evtchn_fifo_set_pending(struct vcpu *v, 
> struct evtchn *evtchn)
>                                    &v->evtchn_fifo->control_block->ready) )
>              vcpu_mark_events_pending(v);
>      }
> -
> + done:
>      if ( !was_pending )
>          evtchn_check_pollers(d, port);
>  }
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index cbdf377..5ab92dd 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -98,6 +98,8 @@ struct evtchn
>      } u;
>      u8 priority;
>      u8 pending:1;
> +    u16 last_vcpu_id;
> +    u8 last_priority;
>  #ifdef FLASK_ENABLE
>      void *ssid;
>  #endif
> -- 
> 1.7.2.5



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.