linux-kernel - Re: [PATCH 5 of 5] virtio: expose added descriptors immediately

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]

Date:	Mon, 21 Nov 2011 13:57:04 +0200
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	Christoph Hellwig <hch@...radead.org>,
	linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
	virtualization <virtualization@...ts.linux-foundation.org>
Subject: Re: [PATCH 5 of 5] virtio: expose added descriptors immediately

On Mon, Nov 21, 2011 at 12:18:45PM +1030, Rusty Russell wrote:
> On Wed, 16 Nov 2011 09:18:38 +0200, "Michael S. Tsirkin" <mst@...hat.com> wrote:
> > My unlocked kick patches will trip this warning: they make
> > virtio-net do add + get without kick.
> 
> Heh, it's a good sign if they do, since that means you're running really
> well :)

They don't in fact, in my testing :(. But I think they can with luck.

> > I think block with unlocked kick can trip it too:
> > add, lock is dropped and then an interrupt can get.
> > 
> > We also don't need a kick each num - each 2^15 is enough.
> > Why don't we do this at start of add_buf:
> > if (vq->num_added >= 0x7fff)
> > 	return -ENOSPC;
> 
> The warning was there in case a driver is never doing a kick, and
> getting away with it (mostly) because the device is polling.  Let's not
> penalize good drivers to catch bad ones.
> 
> How about we do this properly, like so:

Absolutely. But I think we also need to handle num_added
overflow of a 15 bit counter, no? Otherwise the
vring_need_event logic might give us false negatives ....
I'm guessing we can just assume we need a kick in that case.

> From: Rusty Russell <rusty@...tcorp.com.au>
> Subject: virtio: add debugging if driver doesn't kick.
> 
> Under the existing #ifdef DEBUG, check that they don't have more than
> 1/10 of a second between an add_buf() and a
> virtqueue_notify()/virtqueue_kick_prepare() call.
> 
> We could get false positives on a really busy system, but good for
> development.
> 
> Signed-off-by: Rusty Russell <rusty@...tcorp.com.au>
> ---
>  drivers/virtio/virtio_ring.c |   31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -22,6 +22,7 @@
>  #include <linux/device.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> +#include <linux/hrtimer.h>
>  
>  /* virtio guest is communicating with a virtual "device" that actually runs on
>   * a host processor.  Memory barriers are used to control SMP effects. */
> @@ -102,6 +103,10 @@ struct vring_virtqueue
>  #ifdef DEBUG
>  	/* They're supposed to lock for us. */
>  	unsigned int in_use;
> +
> +	/* Figure out if their kicks are too delayed. */
> +	bool last_add_time_valid;
> +	ktime_t last_add_time;
>  #endif
>  
>  	/* Tokens for callbacks. */
> @@ -192,6 +197,19 @@ int virtqueue_add_buf(struct virtqueue *
>  
>  	BUG_ON(data == NULL);
>  
> +#ifdef DEBUG
> +	{
> +		ktime_t now = ktime_get();
> +
> +		/* No kick or get, with .1 second between?  Warn. */
> +		if (vq->last_add_time_valid)
> +			WARN_ON(ktime_to_ms(ktime_sub(now, vq->last_add_time))
> +					    > 100);
> +		vq->last_add_time = now;
> +		vq->last_add_time_valid = true;
> +	}
> +#endif
> +
>  	/* If the host supports indirect descriptor tables, and we have multiple
>  	 * buffers, then go indirect. FIXME: tune this threshold */
>  	if (vq->indirect && (out + in) > 1 && vq->num_free) {
> @@ -291,6 +309,14 @@ bool virtqueue_kick_prepare(struct virtq
>  	new = vq->vring.avail->idx;
>  	vq->num_added = 0;
>  
> +#ifdef DEBUG
> +	if (vq->last_add_time_valid) {
> +		WARN_ON(ktime_to_ms(ktime_sub(ktime_get(),
> +					      vq->last_add_time)) > 100);
> +	}
> +	vq->last_add_time_valid = false;
> +#endif
> +
>  	if (vq->event) {
>  		needs_kick = vring_need_event(vring_avail_event(&vq->vring),
>  					      new, old);
> @@ -428,6 +454,10 @@ void *virtqueue_get_buf(struct virtqueue
>  		virtio_mb();
>  	}
>  
> +#ifdef DEBUG
> +	vq->last_add_time_valid = false;
> +#endif
> +
>  	END_USE(vq);
>  	return ret;
>  }
> @@ -611,6 +641,7 @@ struct virtqueue *vring_new_virtqueue(un
>  	list_add_tail(&vq->vq.list, &vdev->vqs);
>  #ifdef DEBUG
>  	vq->in_use = false;
> +	vq->last_add_time_valid = false;
>  #endif
>  
>  	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/