search for: virtio_transport_deliver_tap_pkt

...; Thanks, I'll fix in the v2! > > @@ -137,6 +135,11 @@ virtio_transport_send_pkt_work(struct work_struct *work) > > break; > > } > > > > + /* Deliver to monitoring devices all correctly transmitted > > + * packets. > > + */ > > + virtio_transport_deliver_tap_pkt(pkt); > > + > > The device may see the tx packet and therefore receive a reply to it > before we can call virtio_transport_deliver_tap_pkt(). Does this mean > that replies can now appear in the packet capture before the transmitted > packet? hmm, you are right! And the same...

...; Thanks, I'll fix in the v2! > > @@ -137,6 +135,11 @@ virtio_transport_send_pkt_work(struct work_struct *work) > > break; > > } > > > > + /* Deliver to monitoring devices all correctly transmitted > > + * packets. > > + */ > > + virtio_transport_deliver_tap_pkt(pkt); > > + > > The device may see the tx packet and therefore receive a reply to it > before we can call virtio_transport_deliver_tap_pkt(). Does this mean > that replies can now appear in the packet capture before the transmitted > packet? hmm, you are right! And the same...

...nsport.c index dfbaf6bd8b1c..d8db837a96fe 100644 --- a/net/vmw_vsock/virtio_transport.c +++ b/net/vmw_vsock/virtio_transport.c @@ -115,8 +115,6 @@ virtio_transport_send_pkt_work(struct work_struct *work) list_del_init(&pkt->list); spin_unlock_bh(&vsock->send_pkt_list_lock); - virtio_transport_deliver_tap_pkt(pkt); - reply = pkt->reply; sg_init_one(&hdr, &pkt->hdr, sizeof(pkt->hdr)); @@ -137,6 +135,11 @@ virtio_transport_send_pkt_work(struct work_struct *work) break; } + /* Deliver to monitoring devices all correctly transmitted + * packets. + */ + virtio_transpor...

...nsport.c index dfbaf6bd8b1c..d8db837a96fe 100644 --- a/net/vmw_vsock/virtio_transport.c +++ b/net/vmw_vsock/virtio_transport.c @@ -115,8 +115,6 @@ virtio_transport_send_pkt_work(struct work_struct *work) list_del_init(&pkt->list); spin_unlock_bh(&vsock->send_pkt_list_lock); - virtio_transport_deliver_tap_pkt(pkt); - reply = pkt->reply; sg_init_one(&hdr, &pkt->hdr, sizeof(pkt->hdr)); @@ -137,6 +135,11 @@ virtio_transport_send_pkt_work(struct work_struct *work) break; } + /* Deliver to monitoring devices all correctly transmitted + * packets. + */ + virtio_transpor...

From: Cong Wang <cong.wang at bytedance.com> When virtqueue_add_sgs() fails, the skb is put back to send queue, we should not deliver the copy to tap device in this case. So we need to move virtio_transport_deliver_tap_pkt() down after all possible failures. Fixes: 82dfb540aeb2 ("VSOCK: Add virtio vsock vsockmon hooks") Cc: Stefan Hajnoczi <stefanha at redhat.com> Cc: Stefano Garzarella <sgarzare at redhat.com> Cc: Bobby Eshleman <bobby.eshleman at bytedance.com> Signed-off-by: Cong Wang...

...; > > @@ -137,6 +135,11 @@ virtio_transport_send_pkt_work(struct work_struct *work) > > > break; > > > } > > > > > > + /* Deliver to monitoring devices all correctly transmitted > > > + * packets. > > > + */ > > > + virtio_transport_deliver_tap_pkt(pkt); > > > + > > > > The device may see the tx packet and therefore receive a reply to it > > before we can call virtio_transport_deliver_tap_pkt(). Does this mean > > that replies can now appear in the packet capture before the transmitted > > packet? >...

During the review of v1, Stefan pointed out an issue introduced by that patch, where replies can appear in the packet capture before the transmitted packet. While fixing my patch, reverting it and adding a new flag in 'struct virtio_vsock_pkt' (patch 2/2), I found that we already had that issue in vhost-vsock, so I fixed it (patch 1/2). v1 -> v2: - reverted the v1 patch, to avoid that

...| 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c index 50de0642dea6..c2d7d57e98cf 100644 --- a/drivers/vhost/vsock.c +++ b/drivers/vhost/vsock.c @@ -480,7 +480,9 @@ static void vhost_vsock_handle_tx_kick(struct vhost_work *work) virtio_transport_deliver_tap_pkt(pkt); /* Only accept correctly addressed packets */ - if (le64_to_cpu(pkt->hdr.src_cid) == vsock->guest_cid) + if (le64_to_cpu(pkt->hdr.src_cid) == vsock->guest_cid && + le64_to_cpu(pkt->hdr.dst_cid) == + vhost_transport_get_local_cid()) virtio_transport_...

...| 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c index 50de0642dea6..c2d7d57e98cf 100644 --- a/drivers/vhost/vsock.c +++ b/drivers/vhost/vsock.c @@ -480,7 +480,9 @@ static void vhost_vsock_handle_tx_kick(struct vhost_work *work) virtio_transport_deliver_tap_pkt(pkt); /* Only accept correctly addressed packets */ - if (le64_to_cpu(pkt->hdr.src_cid) == vsock->guest_cid) + if (le64_to_cpu(pkt->hdr.src_cid) == vsock->guest_cid && + le64_to_cpu(pkt->hdr.dst_cid) == + vhost_transport_get_local_cid()) virtio_transport_...

On Thu, May 16, 2019 at 03:47:41AM -0400, Jason Wang wrote: > @@ -183,7 +184,8 @@ static struct vhost_vsock *vhost_vsock_get(u32 guest_cid) > virtio_transport_deliver_tap_pkt(pkt); > > virtio_transport_free_pkt(pkt); > - } > + total_len += pkt->len; Please increment total_len before virtio_transport_free_pkt(pkt) to avoid use-after-free. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: applicat...

...it is properly queued in the s/,// > virqueue. s/virqueue/virtqueue/ > @@ -137,6 +135,11 @@ virtio_transport_send_pkt_work(struct work_struct *work) > break; > } > > + /* Deliver to monitoring devices all correctly transmitted > + * packets. > + */ > + virtio_transport_deliver_tap_pkt(pkt); > + The device may see the tx packet and therefore receive a reply to it before we can call virtio_transport_deliver_tap_pkt(). Does this mean that replies can now appear in the packet capture before the transmitted packet? -------------- next part -------------- A non-text attachment wa...

..._vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index 709038a4783e..69efc891885f 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -157,7 +157,11 @@ static struct sk_buff *virtio_transport_build_skb(void *opaque) void virtio_transport_deliver_tap_pkt(struct virtio_vsock_pkt *pkt) { + if (pkt->tap_delivered) + return; + vsock_deliver_tap(virtio_transport_build_skb, pkt); + pkt->tap_delivered = true; } EXPORT_SYMBOL_GPL(virtio_transport_deliver_tap_pkt); -- 2.25.3

...gt; + */ > > + if (pkt->off < pkt->len) { > > + spin_lock_bh(&vsock->send_pkt_list_lock); > > + list_add(&pkt->list, &vsock->send_pkt_list); > > + spin_unlock_bh(&vsock->send_pkt_list_lock); > > + continue; > > The virtio_transport_deliver_tap_pkt() call is skipped. Packet capture > should see the exact packets that are delivered. I think this patch > will present one large packet instead of several smaller packets that > were actually delivered. I'll modify virtio_transport_build_skb() to take care of pkt->off and reading...

...gt; + */ > > + if (pkt->off < pkt->len) { > > + spin_lock_bh(&vsock->send_pkt_list_lock); > > + list_add(&pkt->list, &vsock->send_pkt_list); > > + spin_unlock_bh(&vsock->send_pkt_list_lock); > > + continue; > > The virtio_transport_deliver_tap_pkt() call is skipped. Packet capture > should see the exact packets that are delivered. I think this patch > will present one large packet instead of several smaller packets that > were actually delivered. I'll modify virtio_transport_build_skb() to take care of pkt->off and reading...

...if (!ret) { > spin_lock_bh(&vsock->send_pkt_list_lock); > list_add(&pkt->list, &vsock->send_pkt_list); > spin_unlock_bh(&vsock->send_pkt_list_lock); > @@ -186,7 +188,8 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock, > */ > virtio_transport_deliver_tap_pkt(pkt); > > - vhost_add_used(vq, head, sizeof(pkt->hdr) + payload_len); > + buf.in_len = sizeof(pkt->hdr) + payload_len; > + vhost_put_used_buf(vq, &buf); > added = true; > > pkt->off += payload_len; > @@ -440,7 +443,8 @@ static void vhost_vsock_handl...

...if (!ret) { > spin_lock_bh(&vsock->send_pkt_list_lock); > list_add(&pkt->list, &vsock->send_pkt_list); > spin_unlock_bh(&vsock->send_pkt_list_lock); > @@ -186,7 +188,8 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock, > */ > virtio_transport_deliver_tap_pkt(pkt); > > - vhost_add_used(vq, head, sizeof(pkt->hdr) + payload_len); > + buf.in_len = sizeof(pkt->hdr) + payload_len; > + vhost_put_used_buf(vq, &buf); > added = true; > > pkt->off += payload_len; > @@ -440,7 +443,8 @@ static void vhost_vsock_handl...

This series mainly solves a possible null-pointer dereference in virtio_transport_recv_listen() introduced with the multi-transport support [PATCH 1]. PATCH 2 adds a WARN_ON check for the same potential issue and a returned error in the virtio_transport_send_pkt_info() function to avoid crashing the kernel. Stefano Garzarella (2): vsock/virtio: fix null-pointer dereference in

...{ - break; - } - - vsock->rx_buf_nr--; - - /* Drop short/long packets */ - if (unlikely(len < sizeof(pkt->hdr) || - len > sizeof(pkt->hdr) + pkt->len)) { - virtio_transport_free_pkt(pkt); - continue; - } - - pkt->len = len - sizeof(pkt->hdr); - virtio_transport_deliver_tap_pkt(pkt); - virtio_transport_recv_pkt(pkt); - } - } while (!virtqueue_enable_cb(vq)); - -out: - if (vsock->rx_buf_nr < vsock->rx_buf_max_nr / 2) - virtio_vsock_rx_fill(vsock); - mutex_unlock(&vsock->rx_lock); -} - /* event_lock must be held */ static int virtio_vsock_event_fill_on...

...gt;hdr) || - len > sizeof(pkt->hdr) + pkt->len)) { + if (unlikely(len < vsock_hlen || + len > vsock_hlen + pkt->len)) { virtio_transport_free_pkt(pkt); continue; } - pkt->len = len - sizeof(pkt->hdr); + pkt->len = len - vsock_hlen; virtio_transport_deliver_tap_pkt(pkt); virtio_transport_recv_pkt(pkt); } diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index 3ae3a33..7bef1d5 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -272,14 +272,49 @@ static int vi...

...gt;hdr) || - len > sizeof(pkt->hdr) + pkt->len)) { + if (unlikely(len < vsock_hlen || + len > vsock_hlen + pkt->len)) { virtio_transport_free_pkt(pkt); continue; } - pkt->len = len - sizeof(pkt->hdr); + pkt->len = len - vsock_hlen; virtio_transport_deliver_tap_pkt(pkt); virtio_transport_recv_pkt(pkt); } diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index 3ae3a33..7bef1d5 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -272,14 +272,49 @@ static int vi...