| Message ID | 20170503121143.14925-1-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi Marc-André, On 05/03/2017 09:11 AM, Marc-André Lureau wrote: > Calling vu_queue_get_avail_bytes() when the queue doesn't yet have > addresses will result in the following crash: > > Program received signal SIGSEGV, Segmentation fault. > 0x000055c414112ce4 in vring_avail_idx (vq=0x55c41582fd68, vq=0x55c41582fd68) > at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940 > 940 vq->shadow_avail_idx = vq->vring.avail->idx; > (gdb) p vq > $1 = (VuVirtq *) 0x55c41582fd68 > (gdb) p vq->vring > $2 = {num = 0, desc = 0x0, avail = 0x0, used = 0x0, log_guest_addr = 0, flags = 0} > > at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940 > No locals. > at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:960 > num_heads = <optimized out> > out_bytes=out_bytes@entry=0x7fffd035d7c4, max_in_bytes=max_in_bytes@entry=0, > max_out_bytes=max_out_bytes@entry=0) at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:1034 > > Check if vring.avail != null before accessing it. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > contrib/libvhost-user/libvhost-user.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c > index af4faad60b..f9680b6279 100644 > --- a/contrib/libvhost-user/libvhost-user.c > +++ b/contrib/libvhost-user/libvhost-user.c > @@ -1031,6 +1031,10 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq, unsigned int *in_bytes, > idx = vq->last_avail_idx; > > total_bufs = in_total = out_total = 0; > + if (!vq->vring.avail) { > + goto done; > + } > + > while ((rc = virtqueue_num_heads(dev, vq, idx)) > 0) { > unsigned int max, num_bufs, indirect = 0; > struct vring_desc *desc; > It seems to me safer to fix instead vring_avail_ring(), and fix neighbours vring_avail_flags() and vring_avail_idx() while here.
Hi ----- Original Message ----- > Hi Marc-André, > > On 05/03/2017 09:11 AM, Marc-André Lureau wrote: > > Calling vu_queue_get_avail_bytes() when the queue doesn't yet have > > addresses will result in the following crash: > > > > Program received signal SIGSEGV, Segmentation fault. > > 0x000055c414112ce4 in vring_avail_idx (vq=0x55c41582fd68, > > vq=0x55c41582fd68) > > at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940 > > 940 vq->shadow_avail_idx = vq->vring.avail->idx; > > (gdb) p vq > > $1 = (VuVirtq *) 0x55c41582fd68 > > (gdb) p vq->vring > > $2 = {num = 0, desc = 0x0, avail = 0x0, used = 0x0, log_guest_addr = 0, > > flags = 0} > > > > at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940 > > No locals. > > at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:960 > > num_heads = <optimized out> > > out_bytes=out_bytes@entry=0x7fffd035d7c4, > > max_in_bytes=max_in_bytes@entry=0, > > max_out_bytes=max_out_bytes@entry=0) at > > /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:1034 > > > > Check if vring.avail != null before accessing it. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > contrib/libvhost-user/libvhost-user.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/contrib/libvhost-user/libvhost-user.c > > b/contrib/libvhost-user/libvhost-user.c > > index af4faad60b..f9680b6279 100644 > > --- a/contrib/libvhost-user/libvhost-user.c > > +++ b/contrib/libvhost-user/libvhost-user.c > > @@ -1031,6 +1031,10 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq, > > unsigned int *in_bytes, > > idx = vq->last_avail_idx; > > > > total_bufs = in_total = out_total = 0; > > + if (!vq->vring.avail) { > > + goto done; > > + } > > + > > while ((rc = virtqueue_num_heads(dev, vq, idx)) > 0) { > > unsigned int max, num_bufs, indirect = 0; > > struct vring_desc *desc; > > > > It seems to me safer to fix instead vring_avail_ring(), and fix > neighbours vring_avail_flags() and vring_avail_idx() while here. Those are internal/static functions, possibly on hot paths. So I would rather keep the check on the external/public functions only.
>> On 05/03/2017 09:11 AM, Marc-André Lureau wrote: >>> Calling vu_queue_get_avail_bytes() when the queue doesn't yet have >>> addresses will result in the following crash: >>> >>> Program received signal SIGSEGV, Segmentation fault. >>> 0x000055c414112ce4 in vring_avail_idx (vq=0x55c41582fd68, >>> vq=0x55c41582fd68) >>> at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940 >>> 940 vq->shadow_avail_idx = vq->vring.avail->idx; >>> (gdb) p vq >>> $1 = (VuVirtq *) 0x55c41582fd68 >>> (gdb) p vq->vring >>> $2 = {num = 0, desc = 0x0, avail = 0x0, used = 0x0, log_guest_addr = 0, >>> flags = 0} >>> >>> at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940 >>> No locals. >>> at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:960 >>> num_heads = <optimized out> >>> out_bytes=out_bytes@entry=0x7fffd035d7c4, >>> max_in_bytes=max_in_bytes@entry=0, >>> max_out_bytes=max_out_bytes@entry=0) at >>> /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:1034 >>> >>> Check if vring.avail != null before accessing it. >>> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>> --- >>> contrib/libvhost-user/libvhost-user.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/contrib/libvhost-user/libvhost-user.c >>> b/contrib/libvhost-user/libvhost-user.c >>> index af4faad60b..f9680b6279 100644 >>> --- a/contrib/libvhost-user/libvhost-user.c >>> +++ b/contrib/libvhost-user/libvhost-user.c >>> @@ -1031,6 +1031,10 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq, >>> unsigned int *in_bytes, >>> idx = vq->last_avail_idx; >>> >>> total_bufs = in_total = out_total = 0; >>> + if (!vq->vring.avail) { >>> + goto done; >>> + } >>> + >>> while ((rc = virtqueue_num_heads(dev, vq, idx)) > 0) { >>> unsigned int max, num_bufs, indirect = 0; >>> struct vring_desc *desc; >>> >> >> It seems to me safer to fix instead vring_avail_ring(), and fix >> neighbours vring_avail_flags() and vring_avail_idx() while here. > > Those are internal/static functions, possibly on hot paths. So I would rather keep the check on the external/public functions only. Fair enough. Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
* Marc-André Lureau (marcandre.lureau@redhat.com) wrote: > Calling vu_queue_get_avail_bytes() when the queue doesn't yet have > addresses will result in the following crash: > > Program received signal SIGSEGV, Segmentation fault. > 0x000055c414112ce4 in vring_avail_idx (vq=0x55c41582fd68, vq=0x55c41582fd68) > at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940 > 940 vq->shadow_avail_idx = vq->vring.avail->idx; > (gdb) p vq > $1 = (VuVirtq *) 0x55c41582fd68 > (gdb) p vq->vring > $2 = {num = 0, desc = 0x0, avail = 0x0, used = 0x0, log_guest_addr = 0, flags = 0} > > at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940 > No locals. > at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:960 > num_heads = <optimized out> > out_bytes=out_bytes@entry=0x7fffd035d7c4, max_in_bytes=max_in_bytes@entry=0, > max_out_bytes=max_out_bytes@entry=0) at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:1034 > > Check if vring.avail != null before accessing it. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> This doesn't seem to fully solve it, it's now failing at: #0 vring_used_idx_set (val=0, vq=0x5570fcb49d68, dev=0x5570fcb49c20) at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:1472 No locals. #1 vu_queue_flush (dev=0x5570fcb49c20, vq=0x5570fcb49d68, count=0) at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:1494 old = 0 new = 0 #2 0x00005570fb8d9639 in vubr_backend_recv_cb (sock=<optimized out>, ctx=0x5570fcb49c20) at /home/dgilbert/git/qemu/tests/vhost-user-bridge.c:349 vubr = 0x5570fcb49c20 dev = 0x5570fcb49c20 vq = 0x5570fcb49d68 elem = <optimized out> mhdr_sg = {{iov_base = 0x0, iov_len = 0} <repeats 740 times>, {iov_base = 0x0, iov_len = 139877843420656}, { iov_len = 0}...} mhdr = {hdr = {flags = 0 '\000', gso_type = 0 '\000', hdr_len = 0, gso_size = 0, csum_start = 0, csum_offset = 0}, num_buffers = 0} mhdr_cnt = <optimized out> hdrlen = 0 i = <optimized out> hdr = {flags = 0 '\000', gso_type = 0 '\000', hdr_len = 0, gso_size = 0, csum_start = 0, csum_offset = 0} __PRETTY_FUNCTION__ = "vubr_backend_recv_cb" #3 0x00005570fb8d8ad3 in dispatcher_wait (timeout=200000, dispr=0x5570fcb4a0b8) at /home/dgilbert/git/qemu/tests/vhost-user-bridge.c:154 e = 0x5570fcb4a180 tv = {tv_sec = 0, tv_usec = 199998} fdset = {fds_bits = {48, 0 <repeats 15 times>}} rc = <optimized out> sock = 4 #4 vubr_run (dev=<optimized out>) at /home/dgilbert/git/qemu/tests/vhost-user-bridge.c:624 No locals. #5 main (argc=<optimized out>, argv=<optimized out>) at /home/dgilbert/git/qemu/tests/vhost-user-bridge.c:697 opt = <optimized out> client = <optimized out> Dave > --- > contrib/libvhost-user/libvhost-user.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c > index af4faad60b..f9680b6279 100644 > --- a/contrib/libvhost-user/libvhost-user.c > +++ b/contrib/libvhost-user/libvhost-user.c > @@ -1031,6 +1031,10 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq, unsigned int *in_bytes, > idx = vq->last_avail_idx; > > total_bufs = in_total = out_total = 0; > + if (!vq->vring.avail) { > + goto done; > + } > + > while ((rc = virtqueue_num_heads(dev, vq, idx)) > 0) { > unsigned int max, num_bufs, indirect = 0; > struct vring_desc *desc; > -- > 2.12.0.191.gc5d8de91d > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c index af4faad60b..f9680b6279 100644 --- a/contrib/libvhost-user/libvhost-user.c +++ b/contrib/libvhost-user/libvhost-user.c @@ -1031,6 +1031,10 @@ vu_queue_get_avail_bytes(VuDev *dev, VuVirtq *vq, unsigned int *in_bytes, idx = vq->last_avail_idx; total_bufs = in_total = out_total = 0; + if (!vq->vring.avail) { + goto done; + } + while ((rc = virtqueue_num_heads(dev, vq, idx)) > 0) { unsigned int max, num_bufs, indirect = 0; struct vring_desc *desc;
Calling vu_queue_get_avail_bytes() when the queue doesn't yet have addresses will result in the following crash: Program received signal SIGSEGV, Segmentation fault. 0x000055c414112ce4 in vring_avail_idx (vq=0x55c41582fd68, vq=0x55c41582fd68) at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940 940 vq->shadow_avail_idx = vq->vring.avail->idx; (gdb) p vq $1 = (VuVirtq *) 0x55c41582fd68 (gdb) p vq->vring $2 = {num = 0, desc = 0x0, avail = 0x0, used = 0x0, log_guest_addr = 0, flags = 0} at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:940 No locals. at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:960 num_heads = <optimized out> out_bytes=out_bytes@entry=0x7fffd035d7c4, max_in_bytes=max_in_bytes@entry=0, max_out_bytes=max_out_bytes@entry=0) at /home/dgilbert/git/qemu/contrib/libvhost-user/libvhost-user.c:1034 Check if vring.avail != null before accessing it. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- contrib/libvhost-user/libvhost-user.c | 4 ++++ 1 file changed, 4 insertions(+)