Re: [Qemu-devel] [PATCH] virtio: move sanity checks to ifdef DEBUG

[Cornelia Huck]

On Wed, 18 Mar 2015 12:42:46 +0100
"Michael S. Tsirkin" <address@hidden> wrote:

> All that happens when virtqueue_fill is invoked incorrectly is that we
> corrupt guest memory, so this check is not a security measure.
> Move the check to ifdef DEBUG to make sure we don't introduce new
> crashes close to release.

I don't really disagree with this patch, but is anybody actually making
regular runs with VIRTIO_DEBUG activated?

> Array scans aren't free either, so it's a good idea from performance
> point of view, too.
> 
> Cc: Rusty Russell <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  hw/virtio/virtio.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 27429c2..37fb2ee 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -243,15 +243,22 @@ int virtio_queue_empty(VirtQueue *vq)
>  void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
>                      unsigned int len_written, unsigned int idx)
>  {
> -    unsigned int offset, tot_wlen;
> +    unsigned int offset;
>      int i;
> 
>      trace_virtqueue_fill(vq, elem, len_written, idx);
> 
> -    for (tot_wlen = i = 0; i < elem->in_num; i++) {
> -        tot_wlen += elem->in_sg[i].iov_len;
> +#ifdef DEBUG_VIRTIO
> +    {
> +        /* Check that len_written is <= the writable length. */
> +        unsigned int tot_wlen;
> +
> +        for (tot_wlen = i = 0; i < elem->in_num; i++) {
> +            tot_wlen += elem->in_sg[i].iov_len;
> +        }
> +        assert(len_written <= tot_wlen);
>      }
> -    assert(len_written <= tot_wlen);
> +#endif
> 
>      offset = 0;
>      for (i = 0; i < elem->in_num; i++) {