netdev - Re: [PATCH][VHOST] fix race with guest on multi-buffer used buffer updates

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <OF343AB88F.017B31FE-ON8825772D.00564524-8825772D.00573773@us.ibm.com>
Date:	Mon, 24 May 2010 08:52:40 -0700
From:	David Stevens <dlstevens@...ibm.com>
To:	"Michael S. Tsirkin" <mst@...hat.com>
Cc:	kvm@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH][VHOST] fix race with guest on multi-buffer used buffer	updates

"Michael S. Tsirkin" <mst@...hat.com> wrote on 05/24/2010 03:17:10 AM:

> On Thu, May 20, 2010 at 09:58:06AM -0700, David L Stevens wrote:
> > [for Michael Tsirkin's vhost development git tree]
> > 
> > This patch fixes a race between guest and host when
> > adding used buffers wraps the ring. Without it, guests
> > can see partial packets before num_buffers is set in
> > the vnet header.
> > 
> > Signed-off-by: David L Stevens <dlstevens@...ibm.com>
> 
> Could you please explain what the race is?

        Sure. The pre-patch code in the ring-wrap case
does this:

        add part1 bufs
        update used index
        add part2 bufs
        update used index

        After we update the used index for part1, the part1
buffers are available to the guest. If the guest is
consuming at that point, it can process the partial
packet before the rest of the packet is there. In that
case, num_buffers will be greater than the number of
buffers available to the guest and it'll drop the
packet with a framing error. I was seeing 2 or 3 framing
errors every 100 million packets or so pre-patch, none
post-patch.
        Actually, the second sentence is incorrect in the
original description-- num_buffers is up to date when
the guest sees it, but the used index is not.

                                                +-DLS


> 
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 7f2568d..74790ab 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -1065,14 +1065,6 @@ static int __vhost_add_used_n(struct 
vhost_virtqueue *vq,
> >        vq_err(vq, "Failed to write used");
> >        return -EFAULT;
> >     }
> > -   /* Make sure buffer is written before we update index. */
> > -   smp_wmb();
> > -   if (put_user(vq->last_used_idx + count, &vq->used->idx)) {
> > -      vq_err(vq, "Failed to increment used idx");
> > -      return -EFAULT;
> > -   }
> > -   if (unlikely(vq->log_used))
> > -      vhost_log_used(vq, used);
> >     vq->last_used_idx += count;
> >     return 0;
> >  }
> > @@ -1093,7 +1085,17 @@ int vhost_add_used_n(struct vhost_virtqueue 
*vq, 
> struct vring_used_elem *heads,
> >        heads += n;
> >        count -= n;
> >     }
> > -   return __vhost_add_used_n(vq, heads, count);
> > +   r = __vhost_add_used_n(vq, heads, count);
> > +
> > +   /* Make sure buffer is written before we update index. */
> > +   smp_wmb();
> > +   if (put_user(vq->last_used_idx, &vq->used->idx)) {
> > +      vq_err(vq, "Failed to increment used idx");
> > +      return -EFAULT;
> > +   }
> > +   if (unlikely(vq->log_used))
> > +      vhost_log_used(vq, vq->used->ring + start);
> > +   return r;
> >  }
> > 
> >  /* This actually signals the guest, using eventfd. */
> > 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html