From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from imap.gmail.com [173.194.76.109] by localhost with POP3 (fetchmail-6.3.26) for (single-drop); Sun, 19 May 2024 09:04:41 +0200 (CEST) Received: by 2002:a05:6a10:271d:b0:547:81a2:46ee with SMTP id ib29csp6373756pxb; Sun, 19 May 2024 00:04:13 -0700 (PDT) X-Forwarded-Encrypted: i=2; AJvYcCWTLpMAdDTiTE9Pm8NkMhJec7j84uokxAgfFaFYeqxNRv7KFHOGxEigCJf/5p/QswCiFPbMCLum/Em3SCuIIDVMWpbRkZIx4zM= X-Google-Smtp-Source: AGHT+IHAkQcls7Zh2fanbk7i4PHqAXhc8c/rOYJh1HlFS+aJA8BUMMwKzJ8Rcre3OTDwyB2UyzSG X-Received: by 2002:a05:6214:4a01:b0:6aa:5eb7:8585 with SMTP id 6a1803df08f44-6aa5eb786b5mr196636d6.38.1716102253024; Sun, 19 May 2024 00:04:13 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1716102253; cv=none; d=google.com; s=arc-20160816; b=JP0N2c83vQfsPYw+x/f3TByBVrbLpYzXNlxK4w4b/2lXVyrjpZONKrqE5c6hT0JvYe 06Vk+4frq5SyZp9TJ6BWl/kcQxm/4Cvvz9ltufw8zJaMTFZ4RCVQQC3E92+yblhOBqRO ZXJc8CuBoRN0En1ngTlzP6Dm68TgXZy7Hfd+25hxIZHfjuLS1D9GGOfgcDMZ6Uq4wGrg 3HvzjQiGkGN1Dg28tJuGyil4OTJlsIwRt3S1d6BbNp4KeMEcqZ9415nFy0TYBc1EVJsb AyL8ds0vD3+vtQ+KhajHQnLwzhUVAvLN/8tmoNYRUvfGamKQS2MSMsuKC24GagEdAYa7 A3oA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:dkim-signature:delivered-to; bh=Cm+5uBYEFAxZ5s7VB8wayPBhEc9K4GqcP8tqQ3NrL18=; fh=htis3z6nDPD3FFPmKX7SGQyhLv1B9tDlnJYWrC8d10o=; b=cF0+aM4I45LTZ4vn+Hj33YWwFIsZZO5rpYdmZ+sNcMec0cRwqGld7OF6yMXWGVTzLr FWIptT1F5+5Mdask9aOgh2ILDPZmNesy1aDOVFkXZEJRf+8XYFhhirY5zFoBsTRxrUxd yMW13E0Gwk3d1+NzyjawTxpIYisbdxkrxpOqhPuiTqRxgKPUZsS5hpMKZ8nYs7ORl2OC XQz8KkELu4r/kJnbBXs3kZvY/1yV1bWhPgWfCHVSapL0KbPrctyt1LL+WbZOSOQ32IsI rVuW36QDqDB1bV+/HJLp0AyQDkQFknMh3ag/tli8xHwNY50jAFL5NseFwLe+TpiUaljX cfnA==; dara=google.com ARC-Authentication-Results: i=1; mx.google.com; dkim=temperror (no key for signature) header.i=@gibson.dropbear.id.au header.s=202312 header.b=LsJu5SSC; spf=pass (google.com: domain of dgibson@gandalf.ozlabs.org designates 150.107.74.76 as permitted sender) smtp.mailfrom=dgibson@gandalf.ozlabs.org Return-Path: Received: from us-smtp-inbound-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com. [205.139.110.120]) by mx.google.com with ESMTPS id 6a1803df08f44-6a15f29aa90si3003446d6.298.2024.05.19.00.04.12 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 19 May 2024 00:04:12 -0700 (PDT) Received-SPF: pass (google.com: domain of dgibson@gandalf.ozlabs.org designates 150.107.74.76 as permitted sender) client-ip=150.107.74.76; Authentication-Results: mx.google.com; dkim=temperror (no key for signature) header.i=@gibson.dropbear.id.au header.s=202312 header.b=LsJu5SSC; spf=pass (google.com: domain of dgibson@gandalf.ozlabs.org designates 150.107.74.76 as permitted sender) smtp.mailfrom=dgibson@gandalf.ozlabs.org Received: from mx-prod-mc-05.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-88-W8E6FfY4PcGeVEaHXk3jyQ-1; Sun, 19 May 2024 03:04:11 -0400 X-MC-Unique: W8E6FfY4PcGeVEaHXk3jyQ-1 Received: from mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.40]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-05.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 2840419560A2 for ; Sun, 19 May 2024 07:04:10 +0000 (UTC) Received: by mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (Postfix) id 1814A1955D7B; Sun, 19 May 2024 07:04:10 +0000 (UTC) Received: from mx-prod-mc-04.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-mc-04.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.23]) by mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 159E11956061 for ; Sun, 19 May 2024 07:04:10 +0000 (UTC) Received: from us-smtp-inbound-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [170.10.128.131]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by mx-prod-mc-04.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id BC75F1955F2F for ; Sun, 19 May 2024 07:04:09 +0000 (UTC) Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-227-VeWS4HJoNi-WNZqe9o4gag-1; Sun, 19 May 2024 03:04:05 -0400 X-MC-Unique: VeWS4HJoNi-WNZqe9o4gag-1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1716102240; bh=Cm+5uBYEFAxZ5s7VB8wayPBhEc9K4GqcP8tqQ3NrL18=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=LsJu5SSCHdO0cJLYRHLnZWfLytPLuWYl88BMyahhc/z4FeprYMPbfp409uNxMDN+/ sPR5dQU3kTcEjrw0feNPLGIMMd5RLc8H7ZtTjVX/fIbeVM9s+Q8JfAW04BxyR3Wejv Kiy23GB16ygJ2DpXTkPxpjFX5sunOE1grk2Hxa0a91XawE+gtzW63WuUm5fo+C78T4 LqOz4gO8aF4KoYnvy97pBEcDFodtqHgEWdiTzytuhRmsS496zLegNLOgopCyg+ghWJ c6949vPSgFr7tWaW0wrfbMl5/C86B2hIml4BCH4KxXbWoZzEUjHeWvn6PLwzubxakJ TmfojrSx7EYZA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4VhsB83qYNz4wyQ; Sun, 19 May 2024 17:04:00 +1000 (AEST) Date: Sat, 18 May 2024 17:08:53 +1000 From: David Gibson To: Stefano Brivio Cc: passt-dev@passt.top Subject: Re: [PATCH v5 15/19] icmp: Use flowsides as the source of truth wherever possible Message-ID: References: <20240514010337.1104606-1-david@gibson.dropbear.id.au> <20240514010337.1104606-16-david@gibson.dropbear.id.au> <20240516225350.06aebcd7@elisabeth> <20240517221123.1c7197a3@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="DhkMkwyc91CHdbAT" Content-Disposition: inline In-Reply-To: <20240517221123.1c7197a3@elisabeth> X-Scanned-By: MIMEDefang 3.0 on 10.30.177.40 List-Id: --DhkMkwyc91CHdbAT Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, May 17, 2024 at 10:11:23PM +0200, Stefano Brivio wrote: > On Fri, 17 May 2024 16:58:38 +1000 > David Gibson wrote: >=20 > > On Thu, May 16, 2024 at 10:53:50PM +0200, Stefano Brivio wrote: > > > On Tue, 14 May 2024 11:03:33 +1000 > > > David Gibson wrote: > > > =20 > > > > icmp_sock_handler() obtains the guest address from it's most recent= ly > > > > observed IP, and the ICMP id from the epoll reference. Both of the= se > > > > can be obtained readily from the flow. > > > >=20 > > > > icmp_tap_handler() builds its socket address for sendto() directly > > > > from the destination address supplied by the incoming tap packet. > > > > This can instead be generated from the flow. > > > >=20 > > > > struct icmp_ping_flow contains a field for the ICMP id of the ping,= but > > > > this is now redundant, since the id is also stored as the "port" in= the > > > > common flowsides. > > > >=20 > > > > Using the flowsides as the common source of truth here prepares us = for > > > > allowing more flexible NAT and forwarding by properly initialising > > > > that flowside information. > > > >=20 > > > > Signed-off-by: David Gibson > > > > --- > > > > icmp.c | 37 ++++++++++++++++++++++--------------- > > > > icmp_flow.h | 1 - > > > > tap.c | 11 ----------- > > > > tap.h | 1 - > > > > 4 files changed, 22 insertions(+), 28 deletions(-) > > > >=20 > > > > diff --git a/icmp.c b/icmp.c > > > > index 37a3586..1e9a05e 100644 > > > > --- a/icmp.c > > > > +++ b/icmp.c > > > > @@ -58,6 +58,7 @@ static struct icmp_ping_flow *icmp_id_map[IP_VERS= IONS][ICMP_NUM_IDS]; > > > > void icmp_sock_handler(const struct ctx *c, union epoll_ref ref) > > > > { > > > > struct icmp_ping_flow *pingf =3D PINGF(ref.flowside.flow); > > > > + const struct flowside *ini =3D &pingf->f.side[INISIDE]; > > > > union sockaddr_inany sr; > > > > socklen_t sl =3D sizeof(sr); > > > > char buf[USHRT_MAX]; > > > > @@ -83,7 +84,7 @@ void icmp_sock_handler(const struct ctx *c, union= epoll_ref ref) > > > > goto unexpected; > > > > =20 > > > > /* Adjust packet back to guest-side ID */ > > > > - ih4->un.echo.id =3D htons(pingf->id); > > > > + ih4->un.echo.id =3D htons(ini->eport); > > > > seq =3D ntohs(ih4->un.echo.sequence); > > > > } else if (pingf->f.type =3D=3D FLOW_PING6) { > > > > struct icmp6hdr *ih6 =3D (struct icmp6hdr *)buf; > > > > @@ -93,7 +94,7 @@ void icmp_sock_handler(const struct ctx *c, union= epoll_ref ref) > > > > goto unexpected; > > > > =20 > > > > /* Adjust packet back to guest-side ID */ > > > > - ih6->icmp6_identifier =3D htons(pingf->id); > > > > + ih6->icmp6_identifier =3D htons(ini->eport); > > > > seq =3D ntohs(ih6->icmp6_sequence); > > > > } else { > > > > ASSERT(0); > > > > @@ -108,13 +109,20 @@ void icmp_sock_handler(const struct ctx *c, u= nion epoll_ref ref) > > > > } > > > > =20 > > > > flow_dbg(pingf, "echo reply to tap, ID: %"PRIu16", seq: %"PRIu16, > > > > - pingf->id, seq); > > > > + ini->eport, seq); > > > > =20 > > > > - if (pingf->f.type =3D=3D FLOW_PING4) > > > > - tap_icmp4_send(c, sr.sa4.sin_addr, tap_ip4_daddr(c), buf, n); > > > > - else if (pingf->f.type =3D=3D FLOW_PING6) > > > > - tap_icmp6_send(c, &sr.sa6.sin6_addr, > > > > - tap_ip6_daddr(c, &sr.sa6.sin6_addr), buf, n); > > > > + if (pingf->f.type =3D=3D FLOW_PING4) { > > > > + const struct in_addr *saddr =3D inany_v4(&ini->faddr); > > > > + const struct in_addr *daddr =3D inany_v4(&ini->eaddr); > > > > + > > > > + ASSERT(saddr && daddr); /* Must have IPv4 addresses */ =20 > > >=20 > > > ...are those somehow special compared to IPv6 ones? =20 > >=20 > > Well, we're about to call a function that's specific to IPv4 ICMP and > > will require IPv4 addresses for both ends. >=20 > Yes, I understand that part, but I was wondering why "Must have IPv4 > addresses" and not IPv6 ones below. On the other hand, due to how the > inany thing works, we'll always have IPv6 addresses "set" there. Well, there are different strength of requirements here. We simply can't proceed without an IPv4 address here: if we tried we'd either NULL pointer dereference from inany_v4() or we'd take a garbage chunk out of an IPv6 address. For the IPv6 case, if the address are v4 it means we'll send ICMPv6 packets with IPv4 mapped addresses in them. That's kinda weird, but not necessarily wrong. >=20 > >=20 > > [snip] > > > > diff --git a/icmp_flow.h b/icmp_flow.h > > > > index 5a2eed9..f053211 100644 > > > > --- a/icmp_flow.h > > > > +++ b/icmp_flow.h > > > > @@ -22,7 +22,6 @@ struct icmp_ping_flow { > > > > int seq; > > > > int sock; > > > > time_t ts; > > > > - uint16_t id; =20 > > >=20 > > > Nit: drop 'id' from struct comment. =20 > >=20 > > Fixed, thanks. > >=20 >=20 --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --DhkMkwyc91CHdbAT Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZIVAQACgkQzQJF27ox 2GcMUg/7BLzGREaKASir3fQVp9rra/vRGdn8aQqM5v4xIrt4b7KGjlUAXykuAKdP IT5tM1PCii0nF1veF3ArTluAmP5iMg0x8VfFlJh651+CDEo9//HAfPK1I3h7M4AP GDCSF7UZDfbIV02aNOSeZh9YJvY68yLgwaFFdo3FpcfvcNin6nB6XjZooK6LNG4P 6R0REFqUjJl1fJIWdMiCHFiHAX6AVBcd2mLMkDuZRxOnyYzXL4HYeOpXAyopiiKX 13P76M+9IArELuHHTBLBfHhJRg+jI/sO471lYUPC4GUQMbfBp1xJjcU31WdYrVp7 2rUbWz+GTd1RaYywDlCuXyBrmtMM18hcoIU1PYVFehzX95C4ELl98fxpcqydETti 8CRxVoMt2fUtae1Riar8LfjzB1o8pTg6J+4bJ2bvxVqUIIwoapHlrJCVsTUDQOTh ttpggR5QQw4pocOilzvNURkuyvBTFzogKc6XkgDT1oSp5RPB82IspsBle3enFU4Q jXA+pY/QZva4zr1t8va1YUpUs6H1M6lWvWd+9q/ko1w3X+DoLRSqy4bn2pbTqkep Cma9qm+kt6+ViCS+b6GFWURO5u4RGILOcrIMhFxcfA2TW2q28glJEFMniCuJuPup gxR2pOppsYp77U2F4fY4DU+73jRnm/CR02VznFF8H8f/4/JdVt8= =qjaI -----END PGP SIGNATURE----- --DhkMkwyc91CHdbAT--