| Message ID | 1569522214-28223-1-git-send-email-matiasevara@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | vsock/virtio: add support for MSG_PEEK | expand |
On 9/26/19 11:23 AM, Matias Ezequiel Vara Larsen wrote: > This patch adds support for MSG_PEEK. In such a case, packets are not > removed from the rx_queue and credit updates are not sent. > > Signed-off-by: Matias Ezequiel Vara Larsen <matiasevara@gmail.com> > --- > net/vmw_vsock/virtio_transport_common.c | 50 +++++++++++++++++++++++++++++++-- > 1 file changed, 47 insertions(+), 3 deletions(-) > > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c > index 94cc0fa..938f2ed 100644 > --- a/net/vmw_vsock/virtio_transport_common.c > +++ b/net/vmw_vsock/virtio_transport_common.c > @@ -264,6 +264,50 @@ static int virtio_transport_send_credit_update(struct vsock_sock *vsk, > } > > static ssize_t > +virtio_transport_stream_do_peek(struct vsock_sock *vsk, > + struct msghdr *msg, > + size_t len) > +{ > + struct virtio_vsock_sock *vvs = vsk->trans; > + struct virtio_vsock_pkt *pkt; > + size_t bytes, total = 0; > + int err = -EFAULT; > + > + spin_lock_bh(&vvs->rx_lock); > + > + list_for_each_entry(pkt, &vvs->rx_queue, list) { > + if (total == len) > + break; > + > + bytes = len - total; > + if (bytes > pkt->len - pkt->off) > + bytes = pkt->len - pkt->off; > + > + /* sk_lock is held by caller so no one else can dequeue. > + * Unlock rx_lock since memcpy_to_msg() may sleep. > + */ > + spin_unlock_bh(&vvs->rx_lock); > + > + err = memcpy_to_msg(msg, pkt->buf + pkt->off, bytes); > + if (err) > + goto out; > + > + spin_lock_bh(&vvs->rx_lock); > + > + total += bytes; > + } > + > + spin_unlock_bh(&vvs->rx_lock); > + > + return total; > + > +out: > + if (total) > + err = total; > + return err; > +} > This seems buggy to me. virtio_transport_recv_enqueue() seems to be able to add payload to the last packet in the queue. The loop you wrote here would miss newly added chunks while the vvs->rx_lock spinlock has been released. virtio_transport_stream_do_dequeue() is ok, because it makes sure pkt->off == pkt->len before cleaning the packet from the queue.
On Thu, Sep 26, 2019 at 12:33:36PM -0700, Eric Dumazet wrote: > > > On 9/26/19 11:23 AM, Matias Ezequiel Vara Larsen wrote: > > This patch adds support for MSG_PEEK. In such a case, packets are not > > removed from the rx_queue and credit updates are not sent. > > > > Signed-off-by: Matias Ezequiel Vara Larsen <matiasevara@gmail.com> > > --- > > net/vmw_vsock/virtio_transport_common.c | 50 +++++++++++++++++++++++++++++++-- > > 1 file changed, 47 insertions(+), 3 deletions(-) > > > > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c > > index 94cc0fa..938f2ed 100644 > > --- a/net/vmw_vsock/virtio_transport_common.c > > +++ b/net/vmw_vsock/virtio_transport_common.c > > @@ -264,6 +264,50 @@ static int virtio_transport_send_credit_update(struct vsock_sock *vsk, > > } > > > > static ssize_t > > +virtio_transport_stream_do_peek(struct vsock_sock *vsk, > > + struct msghdr *msg, > > + size_t len) > > +{ > > + struct virtio_vsock_sock *vvs = vsk->trans; > > + struct virtio_vsock_pkt *pkt; > > + size_t bytes, total = 0; > > + int err = -EFAULT; > > + > > + spin_lock_bh(&vvs->rx_lock); > > + > > + list_for_each_entry(pkt, &vvs->rx_queue, list) { > > + if (total == len) > > + break; > > + > > + bytes = len - total; > > + if (bytes > pkt->len - pkt->off) > > + bytes = pkt->len - pkt->off; > > + > > + /* sk_lock is held by caller so no one else can dequeue. > > + * Unlock rx_lock since memcpy_to_msg() may sleep. > > + */ > > + spin_unlock_bh(&vvs->rx_lock); > > + > > + err = memcpy_to_msg(msg, pkt->buf + pkt->off, bytes); > > + if (err) > > + goto out; > > + > > + spin_lock_bh(&vvs->rx_lock); > > + > > + total += bytes; > > + } > > + > > + spin_unlock_bh(&vvs->rx_lock); > > + > > + return total; > > + > > +out: > > + if (total) > > + err = total; > > + return err; > > +} > > > > This seems buggy to me. > > virtio_transport_recv_enqueue() seems to be able to add payload to the last packet in the queue. > > The loop you wrote here would miss newly added chunks while the vvs->rx_lock spinlock has been released. > > virtio_transport_stream_do_dequeue() is ok, because it makes sure pkt->off == pkt->len > before cleaning the packet from the queue. Good catch! Maybe we can solve in this way: list_for_each_entry(pkt, &vvs->rx_queue, list) { size_t off = pkt->off; if (total == len) break; while (total < len && off < pkt->len) { /* using 'off' instead of 'pkt->off' */ ... total += bytes; off += bytes; } } What do you think? Cheers, Stefano
On 9/27/19 1:55 AM, Stefano Garzarella wrote: > Good catch! > > Maybe we can solve in this way: > > list_for_each_entry(pkt, &vvs->rx_queue, list) { > size_t off = pkt->off; > > if (total == len) > break; > > while (total < len && off < pkt->len) { > /* using 'off' instead of 'pkt->off' */ > ... > > total += bytes; > off += bytes; > } > } > > What do you think? > Maybe, but I need to see a complete patch, evil is in the details :)
On Fri, Sep 27, 2019 at 06:37:00AM -0700, Eric Dumazet wrote: > > > On 9/27/19 1:55 AM, Stefano Garzarella wrote: > > > Good catch! > > > > Maybe we can solve in this way: > > > > list_for_each_entry(pkt, &vvs->rx_queue, list) { > > size_t off = pkt->off; > > > > if (total == len) > > break; > > > > while (total < len && off < pkt->len) { > > /* using 'off' instead of 'pkt->off' */ > > ... > > > > total += bytes; > > off += bytes; > > } > > } > > > > What do you think? > > > > Maybe, but I need to see a complete patch, evil is in the details :) > Thanks both for your comments, I will take them into account and submit a second version. Matias
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index 94cc0fa..938f2ed 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -264,6 +264,50 @@ static int virtio_transport_send_credit_update(struct vsock_sock *vsk, } static ssize_t +virtio_transport_stream_do_peek(struct vsock_sock *vsk, + struct msghdr *msg, + size_t len) +{ + struct virtio_vsock_sock *vvs = vsk->trans; + struct virtio_vsock_pkt *pkt; + size_t bytes, total = 0; + int err = -EFAULT; + + spin_lock_bh(&vvs->rx_lock); + + list_for_each_entry(pkt, &vvs->rx_queue, list) { + if (total == len) + break; + + bytes = len - total; + if (bytes > pkt->len - pkt->off) + bytes = pkt->len - pkt->off; + + /* sk_lock is held by caller so no one else can dequeue. + * Unlock rx_lock since memcpy_to_msg() may sleep. + */ + spin_unlock_bh(&vvs->rx_lock); + + err = memcpy_to_msg(msg, pkt->buf + pkt->off, bytes); + if (err) + goto out; + + spin_lock_bh(&vvs->rx_lock); + + total += bytes; + } + + spin_unlock_bh(&vvs->rx_lock); + + return total; + +out: + if (total) + err = total; + return err; +} + +static ssize_t virtio_transport_stream_do_dequeue(struct vsock_sock *vsk, struct msghdr *msg, size_t len) @@ -330,9 +374,9 @@ virtio_transport_stream_dequeue(struct vsock_sock *vsk, size_t len, int flags) { if (flags & MSG_PEEK) - return -EOPNOTSUPP; - - return virtio_transport_stream_do_dequeue(vsk, msg, len); + return virtio_transport_stream_do_peek(vsk, msg, len); + else + return virtio_transport_stream_do_dequeue(vsk, msg, len); } EXPORT_SYMBOL_GPL(virtio_transport_stream_dequeue);
This patch adds support for MSG_PEEK. In such a case, packets are not removed from the rx_queue and credit updates are not sent. Signed-off-by: Matias Ezequiel Vara Larsen <matiasevara@gmail.com> --- net/vmw_vsock/virtio_transport_common.c | 50 +++++++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 3 deletions(-)