On Mon, Nov 22, 2021 at 02:25:26PM +0800, Jason Wang wrote:
On Mon, Nov 22, 2021 at 1:49 PM Halil Pasic <pasic@xxxxxxxxxxxxx> wrote:
On Mon, 22 Nov 2021 06:35:18 +0100
Halil Pasic <pasic@xxxxxxxxxxxxx> wrote:
> > I think it should be a common issue, looking at
> > vhost_vsock_handle_tx_kick(), it did:
> >
> > len += sizeof(pkt->hdr);
> > vhost_add_used(vq, head, len);
> >
> > which looks like a violation of the spec since it's TX.
>
> I'm not sure the lines above look like a violation of the spec. If you
> examine vhost_vsock_alloc_pkt() I believe that you will agree that:
> len == pkt->len == pkt->hdr.len
> which makes sense since according to the spec both tx and rx messages
> are hdr+payload. And I believe hdr.len is the size of the payload,
> although that does not seem to be properly documented by the spec.
Sorry for being unclear, what I meant is that we probably should use
zero here. TX doesn't use in buffer actually.
According to the spec, 0 should be the used length:
"and len the total of bytes written into the buffer."
>
> On the other hand tx messages are stated to be device read-only (in the
> spec) so if the device writes stuff, that is certainly wrong.
>
Yes.
> If that is what happens.
>
> Looking at virtqueue_get_buf_ctx_split() I'm not sure that is what
> happens. My hypothesis is that we just a last descriptor is an 'in'
> type descriptor (i.e. a device writable one). For tx that assumption
> would be wrong.
>
> I will have another look at this today and send a fix patch if my
> suspicion is confirmed.
If my suspicion is right something like:
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 00f64f2f8b72..efb57898920b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -764,6 +764,7 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
struct vring_virtqueue *vq = to_vvq(_vq);
void *ret;
unsigned int i;
+ bool has_in;
u16 last_used;
START_USE(vq);
@@ -787,6 +788,9 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
vq->split.vring.used->ring[last_used].id);
*len = virtio32_to_cpu(_vq->vdev,
vq->split.vring.used->ring[last_used].len);
+ has_in = virtio16_to_cpu(_vq->vdev,
+ vq->split.vring.used->ring[last_used].flags)
+ & VRING_DESC_F_WRITE;
Did you mean vring.desc actually? If yes, it's better not depend on
the descriptor ring which can be modified by the device. We've stored
the flags in desc_extra[].
if (unlikely(i >= vq->split.vring.num)) {
BAD_RING(vq, "id %u out of range\n", i);
@@ -796,7 +800,7 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
BAD_RING(vq, "id %u is not a head!\n", i);
return NULL;
}
- if (vq->buflen && unlikely(*len > vq->buflen[i])) {
+ if (has_in && q->buflen && unlikely(*len > vq->buflen[i])) {
BAD_RING(vq, "used len %d is larger than in buflen %u\n",
*len, vq->buflen[i]);
return NULL;
would fix the problem for split. I will try that out and let you know
later.
I'm not sure I get this, in virtqueue_add_split, the buflen[i] only
contains the in buffer length.
I think the fixes are:
1) fixing the vhost vsock
Yep, in vhost_vsock_handle_tx_kick() we should have vhost_add_used(vq,
head, 0) since the device doesn't write anything.
2) use suppress_used_validation=true to let vsock driver to validate
the in buffer length
3) probably a new feature so the driver can only enable the validation
when the feature is enabled.
I fully agree with these steps.
Thanks,
Stefano
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
