From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Wnm7+s89; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id E8BC35A061A for ; Tue, 26 Nov 2024 16:20:13 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1732634412; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=qgQLW5vxyqbdANwktpa/5LcloeIAqsOLn5Hp4dO+BPI=; b=Wnm7+s89RLc2agxlQw9KdG6g9TsDruh44ds0GcKHD4T9+H7FgbdjNztvhPFKXh7p0qB3uW Ef63ah3B8NIGltw+xRolNXfslY7tSceWeEPMyACHGLxC44CKPMZZTgmQx8oK/MCwMFphVd FRrTFLMOmoZGb0YBmt4Xv/Up2rJCuTg= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-427-KGZ9ucMANx-36B6Q4lCxDQ-1; Tue, 26 Nov 2024 10:20:11 -0500 X-MC-Unique: KGZ9ucMANx-36B6Q4lCxDQ-1 X-Mimecast-MFC-AGG-ID: KGZ9ucMANx-36B6Q4lCxDQ Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-4349c5e58a3so22134825e9.2 for ; Tue, 26 Nov 2024 07:20:11 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732634409; x=1733239209; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=e4Hj6L8gs0TuyF+/b0GCD64QFVDpvpl/JcxciaxiP1k=; b=EUzyfyMDwIR2GwV6iTVf48lHlfznj/A22AT1lSqHXqPb5L51mcEnl/icRxOkyD4GcE eE0kbgdPPrE70IyrlxV2NLCa+sLwnAfbeLh0dsOmLD/bWor535EsjgBPkWo4ZtN/s314 BQBOlfuHYylGy/MJRIKAyN8Pw7lD2tDXBClT5Sf7tDrCDjc4JvQ+cMGY1+ExyWk5o/Hb yZ3SnjNnFh6Hv/O8bMGoaTOI14i8JIlgVYfQMcLc9A5O/s0Jhw5h9ya3C5z+E3BzxSEQ k9fBwGdgGhneQTc408Nxo7XVO6ymrs03TFm0jgY6uqIi9CPgqizfr+oE38a9lyLUkAnA vAuA== X-Gm-Message-State: AOJu0Yz/gGIQ6NgqpJ4ZBhPC5AGaDHwyIleCbvuP+WCfTMcRurhXbloO h/82kN83enZOhQD/WBmLFKUZyOQzKKl2SeKN073zsfU+DW3UUl/fFGYTRm5LUTdeHoAE3qptuVT do1nFKuvRbmpwnsgIXUc8c2tDtfunUYi2yqm1vaZOnKMCJYVYVPdv17TwHEEs9dTmCa8ZCTOFTM +s5/JfzxqpedejuadgZIFm1eGUKhfLHr/q X-Gm-Gg: ASbGncu0pThHEmYByayqN4Q+/B/iISeSocNR/3nUszFIWArp/oh+EZfKBz81Kqgl/8O mZ97oQ5On5ky/L5ITRzvSVx9vcIItTc+p+7+XsTRXQP0eB8q01gu9dB01FZl/uatrjUi/XYKP0V JY96Saw77ZKdhNxBz2C6Fc7cArtnnT42sKEFOlU6LxZUAGMlckQHsimyPyGDDss9J6MjTcgpVrT SIVtUTn246UI6Vy0upl5/vboMf5LrP4OFQgsU7UMHM41tXq+V6pAGJrQi8a4w== X-Received: by 2002:a05:600c:154d:b0:434:a815:2b57 with SMTP id 5b1f17b1804b1-434a8152d9dmr14862095e9.20.1732634409249; Tue, 26 Nov 2024 07:20:09 -0800 (PST) X-Google-Smtp-Source: AGHT+IHh0/dpDrrKGkQesxns4v0qNplYkaJLvyAGffGt8BaqIKo3DxnNSdnaEtDqEGhOAkg3yJSRhQ== X-Received: by 2002:a05:600c:154d:b0:434:a815:2b57 with SMTP id 5b1f17b1804b1-434a8152d9dmr14861255e9.20.1732634407715; Tue, 26 Nov 2024 07:20:07 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-434a101337dsm59027975e9.37.2024.11.26.07.20.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Nov 2024 07:20:06 -0800 (PST) Date: Tue, 26 Nov 2024 16:20:05 +0100 From: Stefano Brivio To: Laurent Vivier Subject: Re: [PATCH v14 7/9] vhost-user: add vhost-user Message-ID: <20241126162005.004ae713@elisabeth> In-Reply-To: <50e95450-620c-47b3-bc70-168d1659fdb5@redhat.com> References: <20241122164337.3377854-1-lvivier@redhat.com> <20241122164337.3377854-8-lvivier@redhat.com> <20241126061443.3287f86d@elisabeth> <20241126145334.13d2301b@elisabeth> <50e95450-620c-47b3-bc70-168d1659fdb5@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: MjE1usuuCxHFnrIPjydYfXc7aKobTV6l2NOHyGbFJ0A_1732634410 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: DKGWDDPPKA7ZAUECIFYKGMVR2S7GQ5NO X-Message-ID-Hash: DKGWDDPPKA7ZAUECIFYKGMVR2S7GQ5NO X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Tue, 26 Nov 2024 15:11:25 +0100 Laurent Vivier wrote: > On 26/11/2024 14:53, Stefano Brivio wrote: > > On Tue, 26 Nov 2024 06:14:43 +0100 > > Stefano Brivio wrote: > > =20 > >> On Fri, 22 Nov 2024 17:43:34 +0100 > >> Laurent Vivier wrote: > >> =20 > >>> +/** > >>> + * tcp_vu_data_from_sock() - Handle new data from socket, queue to v= host-user, > >>> + *=09=09=09 in window > >>> + * @c:=09=09Execution context > >>> + * @conn:=09Connection pointer > >>> + * > >>> + * Return: Negative on connection reset, 0 otherwise > >>> + */ > >>> +int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *= conn) > >>> +{ > >>> +=09uint32_t wnd_scaled =3D conn->wnd_from_tap << conn->ws_from_tap; > >>> +=09struct vu_dev *vdev =3D c->vdev; > >>> +=09struct vu_virtq *vq =3D &vdev->vq[VHOST_USER_RX_QUEUE]; > >>> +=09const struct flowside *tapside =3D TAPFLOW(conn); > >>> +=09size_t fillsize, hdrlen; > >>> +=09int v6 =3D CONN_V6(conn); > >>> +=09uint32_t already_sent; > >>> +=09const uint16_t *check; > >>> +=09int i, iov_cnt; > >>> +=09ssize_t len; > >>> + > >>> +=09if (!vu_queue_enabled(vq) || !vu_queue_started(vq)) { > >>> +=09=09debug("Got packet, but RX virtqueue not usable yet"); > >>> +=09=09return 0; > >>> +=09} > >>> + > >>> +=09already_sent =3D conn->seq_to_tap - conn->seq_ack_from_tap; > >>> + > >>> +=09if (SEQ_LT(already_sent, 0)) { > >>> +=09=09/* RFC 761, section 2.1. */ > >>> +=09=09flow_trace(conn, "ACK sequence gap: ACK for %u, sent: %u", > >>> +=09=09=09 conn->seq_ack_from_tap, conn->seq_to_tap); > >>> +=09=09conn->seq_to_tap =3D conn->seq_ack_from_tap; > >>> +=09=09already_sent =3D 0; > >>> +=09=09if (tcp_set_peek_offset(conn->sock, 0)) { > >>> +=09=09=09tcp_rst(c, conn); > >>> +=09=09=09return -1; > >>> +=09=09} > >>> +=09} > >>> + > >>> +=09if (!wnd_scaled || already_sent >=3D wnd_scaled) { > >>> +=09=09conn_flag(c, conn, STALLED); > >>> +=09=09conn_flag(c, conn, ACK_FROM_TAP_DUE); > >>> +=09=09return 0; > >>> +=09} > >>> + > >>> +=09/* Set up buffer descriptors we'll fill completely and partially.= */ > >>> + > >>> +=09fillsize =3D wnd_scaled - already_sent; > >>> + > >>> +=09/* collect the buffers from vhost-user and fill them with the > >>> +=09 * data from the socket > >>> +=09 */ > >>> +=09len =3D tcp_vu_sock_recv(c, conn, v6, already_sent, fillsize, &io= v_cnt); > >>> +=09if (len < 0) { > >>> +=09=09if (len !=3D -EAGAIN && len !=3D -EWOULDBLOCK) { > >>> +=09=09=09tcp_rst(c, conn); > >>> +=09=09=09return len; > >>> +=09=09} > >>> +=09=09return 0; > >>> +=09} > >>> + > >>> +=09if (!len) { > >>> +=09=09if (already_sent) { > >>> +=09=09=09conn_flag(c, conn, STALLED); > >>> +=09=09} else if ((conn->events & (SOCK_FIN_RCVD | TAP_FIN_SENT)) =3D= =3D > >>> +=09=09=09 SOCK_FIN_RCVD) { > >>> +=09=09=09int ret =3D tcp_vu_send_flag(c, conn, FIN | ACK); > >>> +=09=09=09if (ret) { > >>> +=09=09=09=09tcp_rst(c, conn); > >>> +=09=09=09=09return ret; > >>> +=09=09=09} > >>> + > >>> +=09=09=09conn_event(c, conn, TAP_FIN_SENT); > >>> +=09=09} > >>> + > >>> +=09=09return 0; > >>> +=09} > >>> + > >>> +=09conn_flag(c, conn, ~STALLED); > >>> + > >>> +=09/* Likely, some new data was acked too. */ > >>> +=09tcp_update_seqack_wnd(c, conn, false, NULL); > >>> + > >>> +=09/* initialize headers */ > >>> +=09/* iov_vu is an array of buffers and the buffer size can be > >>> +=09 * smaller than the frame size we want to use but with > >>> +=09 * num_buffer we can merge several virtio iov buffers in one pack= et > >>> +=09 * we need only to set the packet headers in the first iov and > >>> +=09 * num_buffer to the number of iov entries > >>> +=09 */ > >>> + > >>> +=09hdrlen =3D tcp_vu_hdrlen(v6); > >>> +=09for (i =3D 0, check =3D NULL; i < head_cnt; i++) { > >>> +=09=09struct iovec *iov =3D &elem[head[i]].in_sg[0]; > >>> +=09=09int buf_cnt =3D head[i + 1] - head[i]; > >>> +=09=09int dlen =3D iov_size(iov, buf_cnt) - hdrlen; =20 > >> > >> Unless I'm missing something, to me this looks like a false positive, > >> but Coverity now reports, for this line: > >> > >> (17) Event function_return: =09Function "iov_size(iov, buf_cnt)" retur= ns 0. > >> (18) Event overflow_const: =09Expression "iov_size(iov, buf_cnt) - hdr= len", 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, under= flows 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) >=3D hdrlen) > >> > >> It can be a follow-up patch, there's no need to re-post the whole thin= g > >> (at least not just for this), unless you see something that actually > >> needs to be fixed. =20 > >=20 > > ...nothing to be fixed in your opinion, I suppose? >=20 > There is an ASSERT() in tcp_vu_sock_recv() that ensure size of the first = iovec of segment=20 > 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. 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). - Debian i686: -- In file included from util.h:21, from packet.c:22: packet.c: In function =E2=80=98packet_check_range=E2=80=99: packet.c:57:23: warning: format =E2=80=98%lu=E2=80=99 expects argument of t= ype =E2=80=98long unsigned int=E2=80=99, but argument 5 has type =E2=80=98s= ize_t=E2=80=99 {aka =E2=80=98unsigned int=E2=80=99} [-Wformat=3D] 57 | trace("packet offset plus length %lu from size %lu,= " | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~= ~~ 58 | "%s:%i", start - p->buf + len + offset, | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | size_t {aka uns= igned int} log.h:25:66: note: in definition of macro =E2=80=98debug=E2=80=99 25 | #define debug(...) logmsg(true, false, LOG_DEBUG, __= VA_ARGS__) | ^~= ~~~~~~~~~ packet.c:57:17: note: in expansion of macro =E2=80=98trace=E2=80=99 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 in= t | %u packet.c:57:23: warning: format =E2=80=98%lu=E2=80=99 expects argument of t= ype =E2=80=98long unsigned int=E2=80=99, but argument 6 has type =E2=80=98s= ize_t=E2=80=99 {aka =E2=80=98unsigned int=E2=80=99} [-Wformat=3D] 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 =E2=80=98debug=E2=80=99 25 | #define debug(...) logmsg(true, false, LOG_DEBUG, __= VA_ARGS__) | ^~= ~~~~~~~~~ packet.c:57:17: note: in expansion of macro =E2=80=98trace=E2=80=99 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,= " | ~~^ | | | lo= ng unsigned int | %u vhost_user.c: In function =E2=80=98qva_to_va=E2=80=99: vhost_user.c:139:32: warning: cast to pointer from integer of different siz= e [-Wint-to-pointer-cast] 139 | return (void *)(qemu_addr - r->qva + r->mma= p_addr + | ^ vhost_user.c: In function =E2=80=98vu_set_mem_table_exec=E2=80=99: vhost_user.c:439:32: warning: cast to pointer from integer of different siz= e [-Wint-to-pointer-cast] 439 | munmap((void *)r->mmap_addr, r->size + r->m= map_offset); | ^ vhost_user.c: In function =E2=80=98vu_cleanup=E2=80=99: vhost_user.c:900:32: warning: cast to pointer from integer of different siz= e [-Wint-to-pointer-cast] 900 | munmap((void *)r->mmap_addr, r->size + r->m= map_offset); | ^ virtio.c: In function =E2=80=98vu_gpa_to_va=E2=80=99: 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->mm= ap_addr + | ^ vu_common.c: In function =E2=80=98vu_packet_check_range=E2=80=99: vu_common.c:37:27: warning: cast to pointer from integer of different size = [-Wint-to-pointer-cast] 37 | char *m =3D (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 ethhd= r' 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 ethhd= r' 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 =E2=80=98packet_check_range=E2=80=99: packet.c:57:23: warning: format =E2=80=98%lu=E2=80=99 expects argument of t= ype =E2=80=98long unsigned int=E2=80=99, but argument 5 has type =E2=80=98s= ize_t=E2=80=99 {aka =E2=80=98unsigned int=E2=80=99} [-Wformat=3D] 57 | trace("packet offset plus length %lu from size %lu,= " | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~= ~~ 58 | "%s:%i", start - p->buf + len + offset, | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | size_t {aka uns= igned int} log.h:25:66: note: in definition of macro =E2=80=98debug=E2=80=99 25 | #define debug(...) logmsg(true, false, LOG_DEBUG, __= VA_ARGS__) | ^~= ~~~~~~~~~ packet.c:57:17: note: in expansion of macro =E2=80=98trace=E2=80=99 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 in= t | %u packet.c:57:23: warning: format =E2=80=98%lu=E2=80=99 expects argument of t= ype =E2=80=98long unsigned int=E2=80=99, but argument 6 has type =E2=80=98s= ize_t=E2=80=99 {aka =E2=80=98unsigned int=E2=80=99} [-Wformat=3D] 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 =E2=80=98debug=E2=80=99 25 | #define debug(...) logmsg(true, false, LOG_DEBUG, __= VA_ARGS__) | ^~= ~~~~~~~~~ packet.c:57:17: note: in expansion of macro =E2=80=98trace=E2=80=99 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,= " | ~~^ | | | lo= ng unsigned int | %u vhost_user.c: In function =E2=80=98qva_to_va=E2=80=99: vhost_user.c:139:32: warning: cast to pointer from integer of different siz= e [-Wint-to-pointer-cast] 139 | return (void *)(qemu_addr - r->qva + r->mma= p_addr + | ^ vhost_user.c: In function =E2=80=98vu_set_mem_table_exec=E2=80=99: vhost_user.c:439:32: warning: cast to pointer from integer of different siz= e [-Wint-to-pointer-cast] 439 | munmap((void *)r->mmap_addr, r->size + r->m= map_offset); | ^ vhost_user.c: In function =E2=80=98vu_cleanup=E2=80=99: vhost_user.c:900:32: warning: cast to pointer from integer of different siz= e [-Wint-to-pointer-cast] 900 | munmap((void *)r->mmap_addr, r->size + r->m= map_offset); | ^ virtio.c: In function =E2=80=98vu_gpa_to_va=E2=80=99: 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->mm= ap_addr + | ^ vu_common.c: In function =E2=80=98vu_packet_check_range=E2=80=99: vu_common.c:37:27: warning: cast to pointer from integer of different size = [-Wint-to-pointer-cast] 37 | char *m =3D (char *)dev_region->mmap_addr; | ^ -- --=20 Stefano