search for: vhost_transport_send_pkt

On 6/6/23 4:49 AM, Stefano Garzarella wrote: > On Mon, Jun 05, 2023 at 01:57:30PM -0500, Mike Christie wrote: >> If userspace does VHOST_VSOCK_SET_GUEST_CID before VHOST_SET_OWNER we >> can race where: >> 1. thread0 calls vhost_transport_send_pkt -> vhost_work_queue >> 2. thread1 does VHOST_SET_OWNER which calls vhost_worker_create. >> 3. vhost_worker_create will set the dev->worker pointer before setting >> the worker->vtsk pointer. >> 4. thread0's vhost_work_queue will see the dev->worker pointer is...

Remove the unused but set variable vq in vhost_transport_send_pkt() to fix the following GCC warning when building with 'W=1': drivers/vhost/vsock.c:198:26: warning: variable ?vq? set but not used Signed-off-by: Tobias Klauser <tklauser at distanz.ch> --- drivers/vhost/vsock.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/vhost/...

Remove the unused but set variable vq in vhost_transport_send_pkt() to fix the following GCC warning when building with 'W=1': drivers/vhost/vsock.c:198:26: warning: variable ?vq? set but not used Signed-off-by: Tobias Klauser <tklauser at distanz.ch> --- drivers/vhost/vsock.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/vhost/...

...0PM -0500, Mike Christie wrote: > On 6/6/23 4:49 AM, Stefano Garzarella wrote: > > On Mon, Jun 05, 2023 at 01:57:30PM -0500, Mike Christie wrote: > >> If userspace does VHOST_VSOCK_SET_GUEST_CID before VHOST_SET_OWNER we > >> can race where: > >> 1. thread0 calls vhost_transport_send_pkt -> vhost_work_queue > >> 2. thread1 does VHOST_SET_OWNER which calls vhost_worker_create. > >> 3. vhost_worker_create will set the dev->worker pointer before setting > >> the worker->vtsk pointer. > >> 4. thread0's vhost_work_queue will see the dev-&...

On Mon, Jun 05, 2023 at 01:57:30PM -0500, Mike Christie wrote: >If userspace does VHOST_VSOCK_SET_GUEST_CID before VHOST_SET_OWNER we >can race where: >1. thread0 calls vhost_transport_send_pkt -> vhost_work_queue >2. thread1 does VHOST_SET_OWNER which calls vhost_worker_create. >3. vhost_worker_create will set the dev->worker pointer before setting >the worker->vtsk pointer. >4. thread0's vhost_work_queue will see the dev->worker pointer is >set and try to...

If userspace does VHOST_VSOCK_SET_GUEST_CID before VHOST_SET_OWNER we can race where: 1. thread0 calls vhost_transport_send_pkt -> vhost_work_queue 2. thread1 does VHOST_SET_OWNER which calls vhost_worker_create. 3. vhost_worker_create will set the dev->worker pointer before setting the worker->vtsk pointer. 4. thread0's vhost_work_queue will see the dev->worker pointer is set and try to call vhost_task_wake...

If userspace does VHOST_VSOCK_SET_GUEST_CID before VHOST_SET_OWNER we can race where: 1. thread0 calls vhost_transport_send_pkt -> vhost_work_queue 2. thread1 does VHOST_SET_OWNER which calls vhost_worker_create. 3. vhost_worker_create will set the dev->worker pointer before setting the worker->vtsk pointer. 4. thread0's vhost_work_queue will see the dev->worker pointer is set and try to call vhost_task_wake...

...ng virtio vsock server. > > > > > > The abnormal flow looks like: > > > host-userspace vhost vsock guest vsock > > > ============== =========== ============ > > > connect() --------> vhost_transport_send_pkt_work() initializing > > > | vq->private_data==NULL > > > | will not be queued > > > V > > > schedule_timeout(2s) > > > vhost_vsock_start() <--------- device ready > &...

...ng virtio vsock server. > > > > > > The abnormal flow looks like: > > > host-userspace vhost vsock guest vsock > > > ============== =========== ============ > > > connect() --------> vhost_transport_send_pkt_work() initializing > > > | vq->private_data==NULL > > > | will not be queued > > > V > > > schedule_timeout(2s) > > > vhost_vsock_start() <--------- device ready > &...

...g/spinlock_debug.c:83 [inline] > do_raw_spin_lock+0x1c0/0x200 kernel/locking/spinlock_debug.c:112 > __raw_spin_lock_bh include/linux/spinlock_api_smp.h:136 [inline] > _raw_spin_lock_bh+0x39/0x40 kernel/locking/spinlock.c:168 > spin_lock_bh include/linux/spinlock.h:315 [inline] > vhost_transport_send_pkt+0x12e/0x380 drivers/vhost/vsock.c:223 Thanks for the vsock fuzzing. This is a useful bug report. It looks like vhost_vsock_get() needs to involve a reference count so that vhost_vsock instances cannot be freed while something is still using them. The reproducer probably involves racing close()...

...st vsock client tries to connect an > initlizing virtio vsock server. > > The abnormal flow looks like: > host-userspace vhost vsock guest vsock > ============== =========== ============ > connect() --------> vhost_transport_send_pkt_work() initializing > | vq->private_data==NULL > | will not be queued > V > schedule_timeout(2s) > vhost_vsock_start() <--------- device ready > set vq->private_data &g...

...gt; > > > > > > > The abnormal flow looks like: > > > > host-userspace vhost vsock guest vsock > > > > ============== =========== ============ > > > > connect() --------> vhost_transport_send_pkt_work() initializing > > > > | vq->private_data==NULL > > > > | will not be queued > > > > V > > > > schedule_timeout(2s) > > > > vhost_vsock_start() <------...

...k.c | 41 +++++++++++++++++++++++++++++++++++++++++ include/linux/virtio_vsock.h | 3 +++ 2 files changed, 44 insertions(+) diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c index a504e2e0..d01e4a4 100644 --- a/drivers/vhost/vsock.c +++ b/drivers/vhost/vsock.c @@ -218,6 +218,46 @@ vhost_transport_send_pkt(struct virtio_vsock_pkt *pkt) return len; } +static int +vhost_transport_cancel_pkt(struct vsock_sock *vsk) +{ + struct vhost_vsock *vsock; + struct virtio_vsock_pkt *pkt, *n; + int cnt = 0; + LIST_HEAD(freeme); + + /* Find the vhost_vsock according to guest context id */ + vsock = vhost_vsoc...

...s/vhost/vsock.c | 29 +++++++++++++++++++++++++++++ include/linux/virtio_vsock.h | 3 +++ 2 files changed, 32 insertions(+) diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c index a504e2e0..0c23b55 100644 --- a/drivers/vhost/vsock.c +++ b/drivers/vhost/vsock.c @@ -218,6 +218,34 @@ vhost_transport_send_pkt(struct virtio_vsock_pkt *pkt) return len; } +static int +vhost_transport_cancel_pkt(struct vsock_sock *vsk) +{ + struct vhost_vsock *vsock; + struct virtio_vsock_pkt *pkt, *n; + LIST_HEAD(freeme); + + /* Find the vhost_vsock according to guest context id */ + vsock = vhost_vsock_get(vsk->r...

Signed-off-by: Peng Tao <bergwolf at gmail.com> --- drivers/vhost/vsock.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c index 0c23b55..3e01d58 100644 --- a/drivers/vhost/vsock.c +++ b/drivers/vhost/vsock.c @@ -195,7 +195,6 @@ static int vhost_transport_send_pkt(struct virtio_vsock_pkt *pkt) { struct vhost_vsock *vsock; - struct vhost_virtqueue *vq; int len = pkt->len; /* Find the vhost_vsock according to guest context id */ @@ -205,8 +204,6 @@ vhost_transport_send_pkt(struct virtio_vsock_pkt *pkt) return -ENODEV; } - vq = &vsock-&g...

Signed-off-by: Peng Tao <bergwolf at gmail.com> --- drivers/vhost/vsock.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c index 0c23b55..3e01d58 100644 --- a/drivers/vhost/vsock.c +++ b/drivers/vhost/vsock.c @@ -195,7 +195,6 @@ static int vhost_transport_send_pkt(struct virtio_vsock_pkt *pkt) { struct vhost_vsock *vsock; - struct vhost_virtqueue *vq; int len = pkt->len; /* Find the vhost_vsock according to guest context id */ @@ -205,8 +204,6 @@ vhost_transport_send_pkt(struct virtio_vsock_pkt *pkt) return -ENODEV; } - vq = &vsock-&g...

..._credit might return less than pkt_len credit */ pkt_len = virtio_transport_get_credit(vvs, pkt_len); /* Do not send zero length OP_RW pkt */ if (pkt_len == 0 && info->op == VIRTIO_VSOCK_OP_RW) return pkt_len; ... } then it queues the packet for the TX worker calling .send_pkt() [vhost_transport_send_pkt() in the vhost_transport case] The main function executed by the TX worker is vhost_transport_do_send_pkt() that picks up a buffer from the virtqueue and it tries to copy the packet (up to 4K) on it. If the buffer allocated from the guest will be smaller then 4K, I think here it will be discarded...

..._credit might return less than pkt_len credit */ pkt_len = virtio_transport_get_credit(vvs, pkt_len); /* Do not send zero length OP_RW pkt */ if (pkt_len == 0 && info->op == VIRTIO_VSOCK_OP_RW) return pkt_len; ... } then it queues the packet for the TX worker calling .send_pkt() [vhost_transport_send_pkt() in the vhost_transport case] The main function executed by the TX worker is vhost_transport_do_send_pkt() that picks up a buffer from the virtqueue and it tries to copy the packet (up to 4K) on it. If the buffer allocated from the guest will be smaller then 4K, I think here it will be discarded...

...(pkt); > + vq_err(vq, "Faulted on copying pkt buf\n"); > + break; > + } > + } > + > + vhost_add_used(vq, head, pkt->len); > + > + virtio_transport_dec_tx_pkt(pkt); > + > + sk = sk_vsock(pkt->trans->vsk); > + /* Release refcnt taken in vhost_transport_send_pkt */ > + sock_put(sk); > + > + virtio_transport_free_pkt(pkt); > + } > + vhost_signal(&vsock->dev, vq); I think you should not signal if used was not updated. > + mutex_unlock(&vq->mutex); > +} > + > +static void vhost_transport_send_pkt_work(struct vhost_...

...(pkt); > + vq_err(vq, "Faulted on copying pkt buf\n"); > + break; > + } > + } > + > + vhost_add_used(vq, head, pkt->len); > + > + virtio_transport_dec_tx_pkt(pkt); > + > + sk = sk_vsock(pkt->trans->vsk); > + /* Release refcnt taken in vhost_transport_send_pkt */ > + sock_put(sk); > + > + virtio_transport_free_pkt(pkt); > + } > + vhost_signal(&vsock->dev, vq); I think you should not signal if used was not updated. > + mutex_unlock(&vq->mutex); > +} > + > +static void vhost_transport_send_pkt_work(struct vhost_...