From: Laurent Vivier <lvivier@redhat.com>
To: Stefano Brivio <sbrivio@redhat.com>
Cc: passt-dev@passt.top
Subject: Re: [PATCH v14 7/9] vhost-user: add vhost-user
Date: Tue, 26 Nov 2024 16:41:14 +0100 [thread overview]
Message-ID: <1113d539-447a-41fb-95fb-166d0349a1fb@redhat.com> (raw)
In-Reply-To: <20241126162005.004ae713@elisabeth>
On 26/11/2024 16:20, Stefano Brivio wrote:
> On Tue, 26 Nov 2024 15:11:25 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
>
>> On 26/11/2024 14:53, Stefano Brivio wrote:
>>> On Tue, 26 Nov 2024 06:14:43 +0100
>>> Stefano Brivio <sbrivio@redhat.com> wrote:
>>>
>>>> On Fri, 22 Nov 2024 17:43:34 +0100
>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>
>>>>> +/**
>>>>> + * tcp_vu_data_from_sock() - Handle new data from socket, queue to vhost-user,
>>>>> + * in window
>>>>> + * @c: Execution context
>>>>> + * @conn: Connection pointer
>>>>> + *
>>>>> + * Return: Negative on connection reset, 0 otherwise
>>>>> + */
>>>>> +int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
>>>>> +{
>>>>> + uint32_t wnd_scaled = conn->wnd_from_tap << conn->ws_from_tap;
>>>>> + struct vu_dev *vdev = c->vdev;
>>>>> + struct vu_virtq *vq = &vdev->vq[VHOST_USER_RX_QUEUE];
>>>>> + const struct flowside *tapside = TAPFLOW(conn);
>>>>> + size_t fillsize, hdrlen;
>>>>> + int v6 = CONN_V6(conn);
>>>>> + uint32_t already_sent;
>>>>> + const uint16_t *check;
>>>>> + int i, iov_cnt;
>>>>> + ssize_t len;
>>>>> +
>>>>> + if (!vu_queue_enabled(vq) || !vu_queue_started(vq)) {
>>>>> + debug("Got packet, but RX virtqueue not usable yet");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
>>>>> +
>>>>> + if (SEQ_LT(already_sent, 0)) {
>>>>> + /* RFC 761, section 2.1. */
>>>>> + flow_trace(conn, "ACK sequence gap: ACK for %u, sent: %u",
>>>>> + conn->seq_ack_from_tap, conn->seq_to_tap);
>>>>> + conn->seq_to_tap = conn->seq_ack_from_tap;
>>>>> + already_sent = 0;
>>>>> + if (tcp_set_peek_offset(conn->sock, 0)) {
>>>>> + tcp_rst(c, conn);
>>>>> + return -1;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + if (!wnd_scaled || already_sent >= wnd_scaled) {
>>>>> + conn_flag(c, conn, STALLED);
>>>>> + conn_flag(c, conn, ACK_FROM_TAP_DUE);
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + /* Set up buffer descriptors we'll fill completely and partially. */
>>>>> +
>>>>> + fillsize = wnd_scaled - already_sent;
>>>>> +
>>>>> + /* collect the buffers from vhost-user and fill them with the
>>>>> + * data from the socket
>>>>> + */
>>>>> + len = tcp_vu_sock_recv(c, conn, v6, already_sent, fillsize, &iov_cnt);
>>>>> + if (len < 0) {
>>>>> + if (len != -EAGAIN && len != -EWOULDBLOCK) {
>>>>> + tcp_rst(c, conn);
>>>>> + return len;
>>>>> + }
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + if (!len) {
>>>>> + if (already_sent) {
>>>>> + conn_flag(c, conn, STALLED);
>>>>> + } else if ((conn->events & (SOCK_FIN_RCVD | TAP_FIN_SENT)) ==
>>>>> + SOCK_FIN_RCVD) {
>>>>> + int ret = tcp_vu_send_flag(c, conn, FIN | ACK);
>>>>> + if (ret) {
>>>>> + tcp_rst(c, conn);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + conn_event(c, conn, TAP_FIN_SENT);
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + conn_flag(c, conn, ~STALLED);
>>>>> +
>>>>> + /* Likely, some new data was acked too. */
>>>>> + tcp_update_seqack_wnd(c, conn, false, NULL);
>>>>> +
>>>>> + /* initialize headers */
>>>>> + /* iov_vu is an array of buffers and the buffer size can be
>>>>> + * smaller than the frame size we want to use but with
>>>>> + * num_buffer we can merge several virtio iov buffers in one packet
>>>>> + * we need only to set the packet headers in the first iov and
>>>>> + * num_buffer to the number of iov entries
>>>>> + */
>>>>> +
>>>>> + hdrlen = tcp_vu_hdrlen(v6);
>>>>> + for (i = 0, check = NULL; i < head_cnt; i++) {
>>>>> + struct iovec *iov = &elem[head[i]].in_sg[0];
>>>>> + int buf_cnt = head[i + 1] - head[i];
>>>>> + int dlen = iov_size(iov, buf_cnt) - hdrlen;
>>>>
>>>> Unless I'm missing something, to me this looks like a false positive,
>>>> but Coverity now reports, for this line:
>>>>
>>>> (17) Event function_return: Function "iov_size(iov, buf_cnt)" returns 0.
>>>> (18) Event overflow_const: Expression "iov_size(iov, buf_cnt) - hdrlen", which is equal to 18446744073709551550, where "iov_size(iov, buf_cnt)" is known to be equal to 0, and "hdrlen" is known to be equal to 66, underflows the type that receives it, an unsigned integer 64 bits wide.
>>>>
>>>> ...I don't think iov_size() can ever return 0 if we reach this point,
>>>> but I would try to cover this by either, in order of preference:
>>>>
>>>> 1. not sending this frame if iov_size(iov, buf_cnt) < hdrlen
>>>>
>>>> 2. an ASSERT(iov_size(iov, buf_cnt) >= hdrlen)
>>>>
>>>> It can be a follow-up patch, there's no need to re-post the whole thing
>>>> (at least not just for this), unless you see something that actually
>>>> needs to be fixed.
>>>
>>> ...nothing to be fixed in your opinion, I suppose?
>>
>> There is an ASSERT() in tcp_vu_sock_recv() that ensure size of the first iovec of segment
>> is at least hdrlen.
>
> Oh, I didn't see that. Instead of duplicating it here, turning 'dlen'
> to ssize_t also takes care of the warning. Probably size_t would be a
> better fit, but ssize_t is anyway harmless. I can change that in a
> follow-up patch too.
dlen is limited by MTU size, so any type bigger then unsigned short is OK...
>
> By the way, I built the series on different architectures and C
> libraries, there are a few "formal" issues, which I can also fix up on
> merge or as follow-up.
>
> That is, if you want to re-post I'm also fine with it of course, but I
> don't see a reason to delay this because of those. I would wait a bit
> to see if David has further comments, and if not, I would make a
> (further) release *first*, so that we have one just before these
> changes, then merge (with fix-ups).
You can fix some of them on merge if you want, I will fix all the remaining ones once merged.
Thanks,
Laurent
>
> - Debian i686:
>
> --
> In file included from util.h:21,
> from packet.c:22:
> packet.c: In function ‘packet_check_range’:
> packet.c:57:23: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 5 has type ‘size_t’ {aka ‘unsigned int’} [-Wformat=]
> 57 | trace("packet offset plus length %lu from size %lu, "
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 58 | "%s:%i", start - p->buf + len + offset,
> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | |
> | size_t {aka unsigned int}
> log.h:25:66: note: in definition of macro ‘debug’
> 25 | #define debug(...) logmsg(true, false, LOG_DEBUG, __VA_ARGS__)
> | ^~~~~~~~~~~
> packet.c:57:17: note: in expansion of macro ‘trace’
> 57 | trace("packet offset plus length %lu from size %lu, "
> | ^~~~~
> packet.c:57:52: note: format string is defined here
> 57 | trace("packet offset plus length %lu from size %lu, "
> | ~~^
> | |
> | long unsigned int
> | %u
> packet.c:57:23: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 6 has type ‘size_t’ {aka ‘unsigned int’} [-Wformat=]
> 57 | trace("packet offset plus length %lu from size %lu, "
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 58 | "%s:%i", start - p->buf + len + offset,
> 59 | p->buf_size, func, line);
> | ~~~~~~~~~~~
> | |
> | size_t {aka unsigned int}
> log.h:25:66: note: in definition of macro ‘debug’
> 25 | #define debug(...) logmsg(true, false, LOG_DEBUG, __VA_ARGS__)
> | ^~~~~~~~~~~
> packet.c:57:17: note: in expansion of macro ‘trace’
> 57 | trace("packet offset plus length %lu from size %lu, "
> | ^~~~~
> packet.c:57:66: note: format string is defined here
> 57 | trace("packet offset plus length %lu from size %lu, "
> | ~~^
> | |
> | long unsigned int
> | %u
> vhost_user.c: In function ‘qva_to_va’:
> vhost_user.c:139:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 139 | return (void *)(qemu_addr - r->qva + r->mmap_addr +
> | ^
> vhost_user.c: In function ‘vu_set_mem_table_exec’:
> vhost_user.c:439:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 439 | munmap((void *)r->mmap_addr, r->size + r->mmap_offset);
> | ^
> vhost_user.c: In function ‘vu_cleanup’:
> vhost_user.c:900:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 900 | munmap((void *)r->mmap_addr, r->size + r->mmap_offset);
> | ^
> virtio.c: In function ‘vu_gpa_to_va’:
> virtio.c:111:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 111 | return (void *)(guest_addr - r->gpa + r->mmap_addr +
> | ^
> vu_common.c: In function ‘vu_packet_check_range’:
> vu_common.c:37:27: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 37 | char *m = (char *)dev_region->mmap_addr;
> | ^
> --
>
> - Alpine (musl) x86:
>
> --
> In file included from passt.h:185,
> from tcp_vu.c:21:
> /usr/include/netinet/if_ether.h:115:8: error: redefinition of 'struct ethhdr'
> 115 | struct ethhdr {
> | ^~~~~~
> In file included from /usr/include/linux/virtio_net.h:32,
> from tcp_vu.c:17:
> /usr/include/linux/if_ether.h:173:8: note: originally defined here
> 173 | struct ethhdr {
> | ^~~~~~
> In file included from passt.h:185,
> from vu_common.c:14:
> /usr/include/netinet/if_ether.h:115:8: error: redefinition of 'struct ethhdr'
> 115 | struct ethhdr {
> | ^~~~~~
> In file included from /usr/include/linux/virtio_net.h:32,
> from vu_common.c:11:
> /usr/include/linux/if_ether.h:173:8: note: originally defined here
> 173 | struct ethhdr {
> | ^~~~~~
> make: *** [Makefile:87: passt] Error 1
> --
>
> - Debian armhf (same issues as i686):
>
> --
> In file included from util.h:21,
> from packet.c:22:
> packet.c: In function ‘packet_check_range’:
> packet.c:57:23: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 5 has type ‘size_t’ {aka ‘unsigned int’} [-Wformat=]
> 57 | trace("packet offset plus length %lu from size %lu, "
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 58 | "%s:%i", start - p->buf + len + offset,
> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> | |
> | size_t {aka unsigned int}
> log.h:25:66: note: in definition of macro ‘debug’
> 25 | #define debug(...) logmsg(true, false, LOG_DEBUG, __VA_ARGS__)
> | ^~~~~~~~~~~
> packet.c:57:17: note: in expansion of macro ‘trace’
> 57 | trace("packet offset plus length %lu from size %lu, "
> | ^~~~~
> packet.c:57:52: note: format string is defined here
> 57 | trace("packet offset plus length %lu from size %lu, "
> | ~~^
> | |
> | long unsigned int
> | %u
> packet.c:57:23: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 6 has type ‘size_t’ {aka ‘unsigned int’} [-Wformat=]
> 57 | trace("packet offset plus length %lu from size %lu, "
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 58 | "%s:%i", start - p->buf + len + offset,
> 59 | p->buf_size, func, line);
> | ~~~~~~~~~~~
> | |
> | size_t {aka unsigned int}
> log.h:25:66: note: in definition of macro ‘debug’
> 25 | #define debug(...) logmsg(true, false, LOG_DEBUG, __VA_ARGS__)
> | ^~~~~~~~~~~
> packet.c:57:17: note: in expansion of macro ‘trace’
> 57 | trace("packet offset plus length %lu from size %lu, "
> | ^~~~~
> packet.c:57:66: note: format string is defined here
> 57 | trace("packet offset plus length %lu from size %lu, "
> | ~~^
> | |
> | long unsigned int
> | %u
> vhost_user.c: In function ‘qva_to_va’:
> vhost_user.c:139:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 139 | return (void *)(qemu_addr - r->qva + r->mmap_addr +
> | ^
> vhost_user.c: In function ‘vu_set_mem_table_exec’:
> vhost_user.c:439:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 439 | munmap((void *)r->mmap_addr, r->size + r->mmap_offset);
> | ^
> vhost_user.c: In function ‘vu_cleanup’:
> vhost_user.c:900:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 900 | munmap((void *)r->mmap_addr, r->size + r->mmap_offset);
> | ^
> virtio.c: In function ‘vu_gpa_to_va’:
> virtio.c:111:32: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 111 | return (void *)(guest_addr - r->gpa + r->mmap_addr +
> | ^
> vu_common.c: In function ‘vu_packet_check_range’:
> vu_common.c:37:27: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 37 | char *m = (char *)dev_region->mmap_addr;
> | ^
> --
>
next prev parent reply other threads:[~2024-11-26 15:41 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-11-22 16:43 [PATCH v14 0/9] Add vhost-user support to passt. (part 3) Laurent Vivier
2024-11-22 16:43 ` [PATCH v14 1/9] packet: replace struct desc by struct iovec Laurent Vivier
2024-11-22 16:43 ` [PATCH v14 2/9] vhost-user: introduce virtio API Laurent Vivier
2024-11-22 16:43 ` [PATCH v14 3/9] vhost-user: introduce vhost-user API Laurent Vivier
2024-11-22 16:43 ` [PATCH v14 4/9] udp: Prepare udp.c to be shared with vhost-user Laurent Vivier
2024-11-22 16:43 ` [PATCH v14 5/9] tcp: Export headers functions Laurent Vivier
2024-11-22 16:43 ` [PATCH v14 6/9] passt: rename tap_sock_init() to tap_backend_init() Laurent Vivier
2024-11-22 16:43 ` [PATCH v14 7/9] vhost-user: add vhost-user Laurent Vivier
2024-11-26 5:14 ` Stefano Brivio
2024-11-26 13:53 ` Stefano Brivio
2024-11-26 14:11 ` Laurent Vivier
2024-11-26 15:20 ` Stefano Brivio
2024-11-26 15:41 ` Laurent Vivier [this message]
2024-11-26 5:24 ` David Gibson
2024-11-28 12:57 ` Laurent Vivier
2024-11-27 4:47 ` Stefano Brivio
2024-11-27 9:09 ` Laurent Vivier
2024-11-27 9:45 ` Stefano Brivio
2024-11-27 9:48 ` Laurent Vivier
2024-11-27 10:03 ` Stefano Brivio
2024-11-27 10:11 ` Laurent Vivier
2024-11-27 10:14 ` Stefano Brivio
2024-11-22 16:43 ` [PATCH v14 8/9] test: Add tests for passt in vhost-user mode Laurent Vivier
2024-11-22 16:43 ` [PATCH v14 9/9] tcp: Move tcp_l2_buf_fill_headers() to tcp_buf.c Laurent Vivier
2024-11-27 16:21 ` [PATCH v14 0/9] Add vhost-user support to passt. (part 3) Stefano Brivio
2024-11-27 16:40 ` Laurent Vivier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1113d539-447a-41fb-95fb-166d0349a1fb@redhat.com \
--to=lvivier@redhat.com \
--cc=passt-dev@passt.top \
--cc=sbrivio@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).