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=ZewBV3nG; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 3052A5A061A for ; Tue, 26 Nov 2024 14:53:41 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1732629220; 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=QGJcgOQa0/pg9vgQfIVqJBp5bfxyIdknToTVpiImT1k=; b=ZewBV3nG3N/ZWirq+yiOPWpwHQo6mOr0IvWQ5qyJG2I4haEC9tdFPKpA4cRrPGBZnXWINa VQkjcgOvLyjAspx20A1bgch4AeSRmAcqhwmQskxlQbUS5f/1Lf4UPs9r3QSSEB2gfFRycz DXaV+lWu+4L0xsYL4xwQ0/Xiq+k3JY8= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-568-dYUlHUdOM0C27tpFPRf2MA-1; Tue, 26 Nov 2024 08:53:38 -0500 X-MC-Unique: dYUlHUdOM0C27tpFPRf2MA-1 X-Mimecast-MFC-AGG-ID: dYUlHUdOM0C27tpFPRf2MA Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-38243a4ba5fso4000326f8f.2 for ; Tue, 26 Nov 2024 05:53:38 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732629217; x=1733234017; 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=QGJcgOQa0/pg9vgQfIVqJBp5bfxyIdknToTVpiImT1k=; b=uCXnjgNxKvObN0tRaVvXA20G8SA8QZa6vzdotqKtR/L4XFfuJhaD9sfQJHAIysdCR3 m4Dq9DuDUYuhU2T63CrWbFYl5gBTbneuSuFGZehAonxMx63ySFkHEs9dRt3hgZYz7rXa 2f+Ly4vKtoUvBhXLPsC195IueTeG582EQTwEsYSZJc6VzoivayseC4OQkoJEqvB8YIC/ ShJCp0m47+vwIvCqT8d5XmlM3nTx0lat4Lgf9owRy7e5xcEHuaIO2smP/CnbGy5GC5lV NTEogLhw6rSkbnnjPfywE3BZmfCQfoKzRXEVvqxxeYAFQxxl4FY7j4akInU43sqSWuiY +Ngg== X-Gm-Message-State: AOJu0YzkI8OPEnSVqVInofygcbRHIW7UQS7m8MjIVqFPgJtttRvp9xlG HzxD7z6mYcoKRujhEEMseAxswTjuT2YxyQi+j+nRGRmZBF0qirISknK/UrtCls7Ex6l9j6ziSfy /BJDSU6waOd6IExhBIL/MhyqVCTj2E6tPbmzPKT+ZjtKpOZMm9ks6A+kCGpoDT7MszSwz8k2K6W 2xHXPL4xXCIEXCdJ4gnWwCrpc8phpsyW/b X-Gm-Gg: ASbGncvvNlHiQlugt0C9bDl/YVQXv2dUTDEXPy3D2qg5ORxI45TYm1TaCW812wZ9NdX 1sitwYXzQxgNzTb41ruGKinCC3c9Y+rkCOV0ykHrVe3rR5wY/2aqu4T2ipXkjmpSwBJCiFCOvZR 02M8NEgIRAVPhPpOm6GEBbXTh9N9+zYWr6Am9mV6XZJyv3ibqiXmJkhogZR+pJ5FYYJZtL+uRCb IwrEq71LpkIEOwqa9ziM0P+TD586nex1hmmi/89LOBg4hfjotOxt8abI7am6Q6Z0/cd/MirAAPf 6E0= X-Received: by 2002:a5d:6482:0:b0:382:46a2:3788 with SMTP id ffacd0b85a97d-38260b6186cmr17636196f8f.25.1732629216799; Tue, 26 Nov 2024 05:53:36 -0800 (PST) X-Google-Smtp-Source: AGHT+IHxEFaL9cDv5F+X2sOKeWIJHNCaOHuRIpp/QXHlV8lE+O1YtJ97bOy655Cz/LS0+4z8Pr9NtA== X-Received: by 2002:a5d:6482:0:b0:382:46a2:3788 with SMTP id ffacd0b85a97d-38260b6186cmr17636174f8f.25.1732629216402; Tue, 26 Nov 2024 05:53:36 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3825fb27196sm13394493f8f.55.2024.11.26.05.53.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Nov 2024 05:53:35 -0800 (PST) Date: Tue, 26 Nov 2024 14:53:34 +0100 From: Stefano Brivio To: Laurent Vivier Subject: Re: [PATCH v14 7/9] vhost-user: add vhost-user Message-ID: <20241126145334.13d2301b@elisabeth> In-Reply-To: <20241126061443.3287f86d@elisabeth> References: <20241122164337.3377854-1-lvivier@redhat.com> <20241122164337.3377854-8-lvivier@redhat.com> <20241126061443.3287f86d@elisabeth> 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: OrkUvNvIMTK-6hWvhPkUObwKn6mo_wYub8G66OoQXF0_1732629218 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: 2ZTARPI2EYIVUNXAGQ4YBGDOCAGKYHJT X-Message-ID-Hash: 2ZTARPI2EYIVUNXAGQ4YBGDOCAGKYHJT 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 06:14:43 +0100 Stefano Brivio wrote: > On Fri, 22 Nov 2024 17:43:34 +0100 > Laurent Vivier 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? -- Stefano