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=d/wntes7; 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 BBAC45A004C for ; Wed, 18 Sep 2024 01:07:08 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1726614427; 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=+6SRDxXkzBFrMWfqpeA8kh+yO6UApxjD1VEEJKsj5rI=; b=d/wntes7J04sx+OSW35+dBa2bcOG/kXZVVYb3001s/qnWuu1sXOD+8Ed4NF+zjUy4IHQi/ QQ5lt/BiPcpQWibKg/BtsnojplssmNJppZ80nyifnrF6FZc+L1wOUHpcRH8B94Optyvr9/ djTfsVhd+xS5KeVaN+1VUm1izWdRqIk= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-520-jsMGluhVPT6BwW43ysYpvg-1; Tue, 17 Sep 2024 19:07:06 -0400 X-MC-Unique: jsMGluhVPT6BwW43ysYpvg-1 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-42cb5b01c20so35364535e9.1 for ; Tue, 17 Sep 2024 16:07:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726614424; x=1727219224; 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=+6SRDxXkzBFrMWfqpeA8kh+yO6UApxjD1VEEJKsj5rI=; b=adtfjuTKV6D+PKjlLkXn3o8tr5kYu5QoPMOTgUcJU6wVCh8Eo7FdVnzfWxTnCHlglJ /4VpmPDDNMLcOBdoGlfL341QdP8mne93/MB7xLiQ6u77ghMjXlBUxLqa6LjKQCO7LLt2 HmpAOQOhJqRwlwRqW4BOhb2X6veZFWC2n+tI4mUsz7X6OnNoe3q7gacUu0+0LeQOehoT VihWbTaFdotagLOXjKtpX6/aGmE49S/JP8WdYm5lToQ+/KOUhuSjgDHBWdvm1N6M4kWl yqRo/2zRwTPfWEOvLR7zJN65EskG/m8pRkWP560d38sjKrf5flCI91I7uAhIXrTtFcKT BGqw== X-Gm-Message-State: AOJu0Yz+HbhBT2Dfv7ABUloU5RzIzgi5TJ60kXFS/9ZawjomyWtSUGP3 DJO1VDr9r9PkWCGrHQqNuYyPipVG+cdHFkMDvu434gJQhI3EqUMQfRWqPSDXkDpZbLVttvpnePi ipL/zS8MiEBJ2jkp6WBubIM1JbvgkGqpBc21l4pVOkwm8ANBpRNSJMDpkI2nFkbT/cGWuiGmLH/ 2vta/t4VvdrVowu6xB5b859rGJSIRJmYd0 X-Received: by 2002:a05:600c:310a:b0:42b:892d:54c0 with SMTP id 5b1f17b1804b1-42d907221b4mr92117565e9.12.1726614423823; Tue, 17 Sep 2024 16:07:03 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGuA0u4RB/vcHLjBJjPwqkQg2ssk+4BZvjy7pPSbBOJG1N93si3riRFc9McEXL5xug0ajiXCQ== X-Received: by 2002:a05:600c:310a:b0:42b:892d:54c0 with SMTP id 5b1f17b1804b1-42d907221b4mr92117435e9.12.1726614423226; Tue, 17 Sep 2024 16:07:03 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-42e704f261esm626705e9.26.2024.09.17.16.07.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Sep 2024 16:07:02 -0700 (PDT) Date: Wed, 18 Sep 2024 01:07:00 +0200 From: Stefano Brivio To: Laurent Vivier Subject: Re: [PATCH v2 1/2] udp: Allow checksum to be disabled Message-ID: <20240918010650.417b2dbe@elisabeth> In-Reply-To: <20240917073058.3666887-2-lvivier@redhat.com> References: <20240917073058.3666887-1-lvivier@redhat.com> <20240917073058.3666887-2-lvivier@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-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: ORADCGUHBI3BHMYJIW3V72SZO2AKOX2D X-Message-ID-Hash: ORADCGUHBI3BHMYJIW3V72SZO2AKOX2D 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: Nits only: On Tue, 17 Sep 2024 09:30:57 +0200 Laurent Vivier wrote: > We can need not to set the UDP checksum. Add a parameter to > udp_update_hdr4() and udp_update_hdr6() to disable it. > > Signed-off-by: Laurent Vivier > --- > > Notes: > v2: s/UPD/UDP/ > > udp.c | 41 +++++++++++++++++++++++++++++++---------- > 1 file changed, 31 insertions(+), 10 deletions(-) > > diff --git a/udp.c b/udp.c > index 2ba00c9c20a8..95151efb9c46 100644 > --- a/udp.c > +++ b/udp.c > @@ -298,11 +298,13 @@ static void udp_splice_send(const struct ctx *c, size_t start, size_t n, > * @bp: Pointer to udp_payload_t to update > * @toside: Flowside for destination side > * @dlen: Length of UDP payload > + * @no_udp_csum: Do not set UDP checksum The description of all the other arguments are aligned, with tabs. You could just add one tab to all the others and have them aligned. Actually, this could simply be 'no_csum', it's obviously UDP. Same for no_tcp_csum in 2/2. > * > * Return: size of IPv4 payload (UDP header + data) > */ > static size_t udp_update_hdr4(struct iphdr *ip4h, struct udp_payload_t *bp, > - const struct flowside *toside, size_t dlen) > + const struct flowside *toside, size_t dlen, > + bool no_udp_csum) > { > const struct in_addr *src = inany_v4(&toside->oaddr); > const struct in_addr *dst = inany_v4(&toside->eaddr); > @@ -319,7 +321,10 @@ static size_t udp_update_hdr4(struct iphdr *ip4h, struct udp_payload_t *bp, > bp->uh.source = htons(toside->oport); > bp->uh.dest = htons(toside->eport); > bp->uh.len = htons(l4len); > - csum_udp4(&bp->uh, *src, *dst, bp->data, dlen); > + if (no_udp_csum) > + bp->uh.check = 0; > + else > + csum_udp4(&bp->uh, *src, *dst, bp->data, dlen); > > return l4len; > } > @@ -330,11 +335,13 @@ static size_t udp_update_hdr4(struct iphdr *ip4h, struct udp_payload_t *bp, > * @bp: Pointer to udp_payload_t to update > * @toside: Flowside for destination side > * @dlen: Length of UDP payload > + * @no_udp_csum: Do not set UDP checksum Same here. > * > * Return: size of IPv6 payload (UDP header + data) > */ > static size_t udp_update_hdr6(struct ipv6hdr *ip6h, struct udp_payload_t *bp, > - const struct flowside *toside, size_t dlen) > + const struct flowside *toside, size_t dlen, > + bool no_udp_csum) > { > uint16_t l4len = dlen + sizeof(bp->uh); > > @@ -348,7 +355,16 @@ static size_t udp_update_hdr6(struct ipv6hdr *ip6h, struct udp_payload_t *bp, > bp->uh.source = htons(toside->oport); > bp->uh.dest = htons(toside->eport); > bp->uh.len = ip6h->payload_len; > - csum_udp6(&bp->uh, &toside->oaddr.a6, &toside->eaddr.a6, bp->data, dlen); > + if (no_udp_csum) { > + /* O is an invalid checksum for UDP IPv6 and dropped by I'm not sure how this happened, but that's the letter O, not the number 0. Maybe you could spell it out, "Zero". > + * the kernel stack, even if the checksum is disabled by virtio > + * flags. We need to put any non-zero value here. > + */ > + bp->uh.check = 0xffff; > + } else { > + csum_udp6(&bp->uh, &toside->oaddr.a6, &toside->eaddr.a6, > + bp->data, dlen); > + } > > return l4len; > } > @@ -358,9 +374,11 @@ static size_t udp_update_hdr6(struct ipv6hdr *ip6h, struct udp_payload_t *bp, > * @mmh: Receiving mmsghdr array > * @idx: Index of the datagram to prepare > * @toside: Flowside for destination side > + * @no_udp_csum: Do not set UDP checksum > */ > -static void udp_tap_prepare(const struct mmsghdr *mmh, unsigned idx, > - const struct flowside *toside) > +static void udp_tap_prepare(const struct mmsghdr *mmh, > + unsigned idx, const struct flowside *toside, > + bool no_udp_csum) > { > struct iovec (*tap_iov)[UDP_NUM_IOVS] = &udp_l2_iov[idx]; > struct udp_payload_t *bp = &udp_payload[idx]; > @@ -368,13 +386,15 @@ static void udp_tap_prepare(const struct mmsghdr *mmh, unsigned idx, > size_t l4len; > > if (!inany_v4(&toside->eaddr) || !inany_v4(&toside->oaddr)) { > - l4len = udp_update_hdr6(&bm->ip6h, bp, toside, mmh[idx].msg_len); > + l4len = udp_update_hdr6(&bm->ip6h, bp, toside, > + mmh[idx].msg_len, no_udp_csum); > tap_hdr_update(&bm->taph, l4len + sizeof(bm->ip6h) + > sizeof(udp6_eth_hdr)); > (*tap_iov)[UDP_IOV_ETH] = IOV_OF_LVALUE(udp6_eth_hdr); > (*tap_iov)[UDP_IOV_IP] = IOV_OF_LVALUE(bm->ip6h); > } else { > - l4len = udp_update_hdr4(&bm->ip4h, bp, toside, mmh[idx].msg_len); > + l4len = udp_update_hdr4(&bm->ip4h, bp, toside, > + mmh[idx].msg_len, no_udp_csum); > tap_hdr_update(&bm->taph, l4len + sizeof(bm->ip4h) + > sizeof(udp4_eth_hdr)); > (*tap_iov)[UDP_IOV_ETH] = IOV_OF_LVALUE(udp4_eth_hdr); > @@ -565,7 +585,8 @@ void udp_listen_sock_handler(const struct ctx *c, union epoll_ref ref, > udp_splice_prepare(udp_mh_recv, i); > } else if (batchpif == PIF_TAP) { > udp_tap_prepare(udp_mh_recv, i, > - flowside_at_sidx(batchsidx)); > + flowside_at_sidx(batchsidx), > + false); > } > > if (++i >= n) > @@ -636,7 +657,7 @@ void udp_reply_sock_handler(const struct ctx *c, union epoll_ref ref, > if (pif_is_socket(topif)) > udp_splice_prepare(udp_mh_recv, i); > else if (topif == PIF_TAP) > - udp_tap_prepare(udp_mh_recv, i, toside); > + udp_tap_prepare(udp_mh_recv, i, toside, false); > /* Restore sockaddr length clobbered by recvmsg() */ > udp_mh_recv[i].msg_hdr.msg_namelen = sizeof(udp_meta[i].s_in); > } Other than that, this and 2/2 look good to me. -- Stefano