On Mon, Nov 22, 2021 at 08:55:24AM +0100, Stefano Garzarella wrote:
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.
Michael sent a patch to suppress the validation, so I think we should
just fix vhost-vsock. I mean something like this:
diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index 938aefbc75ec..4e3b95af7ee4 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -554,7 +554,7 @@ static void vhost_vsock_handle_tx_kick(struct vhost_work *work)
virtio_transport_free_pkt(pkt);
len += sizeof(pkt->hdr);
- vhost_add_used(vq, head, len);
+ vhost_add_used(vq, head, 0);
total_len += len;
added = true;
} while(likely(!vhost_exceeds_weight(vq, ++pkts, total_len)));
I checked and the problem is there from the first commit, so we should
add:
Fixes: 433fc58e6bf2 ("VSOCK: Introduce vhost_vsock.ko")
I tested this patch and it works even without suppressing validation in
the virtio core. But for backwards compatibility we have to suppress it
for sure as Michael did.
Maybe we can have a patch just with this change to backport it easily
and one after to clean up a bit the code that was added after (len,
total_len).
@Halil Let me know if you want to do it, otherwise I can do it.
Stefano
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
