From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202606 header.b=AJDPiq1E; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id CC53C5A026E for ; Thu, 02 Jul 2026 08:31:50 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202606; t=1782973907; bh=qmLgWwowTYv3Fw2nF11ZiUmnb2zgZy7wzdnhAL1EfFQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=AJDPiq1EcYrYo+bal369GwZ9s29uCYlprf+Bx0gBUcoIWjIeNliz2si1EnzvcBKFE ZP8pBA1wqzlZQOpyovi/RaUdM3CJhfkyFgeAzO45epsvY2poYg/iXDgjjFHS8hNl6e E6E4/ZbmhfUic6tKMHX9PbM9K2XW0DOEIcEl89kDTfHmnYhhDemBC4X60Awh0lLWQ8 gE5J+lK9m+kNmfZjrTM4tlpiY5ONAZnUPxs+Gfaq1+98cRPCetC6jsfEdTVgwvnFBj a8ZcUQDdBJtIlBFbcuvX96EKjlUAfWEoOAH5oPxNBCrZODJFAdyHyaONZzS68kof8Y Wn41gvF1EkldQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4grRql056Yz58lv; Thu, 02 Jul 2026 16:31:47 +1000 (AEST) Date: Thu, 2 Jul 2026 16:08:48 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 1/3] fwd_rule: Parse target adddresses for forwarding rules Message-ID: References: <20260701070811.1944139-1-david@gibson.dropbear.id.au> <20260701070811.1944139-2-david@gibson.dropbear.id.au> <20260702065821.745bbd05@elisabeth> <20260702073109.35613343@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="vZWsi6KhDfdD6B3T" Content-Disposition: inline In-Reply-To: <20260702073109.35613343@elisabeth> Message-ID-Hash: HKPPKRFYZVQJY2I3B5XIRNZM5O3U6DC5 X-Message-ID-Hash: HKPPKRFYZVQJY2I3B5XIRNZM5O3U6DC5 X-MailFrom: dgibson@gandalf.ozlabs.org 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: --vZWsi6KhDfdD6B3T Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jul 02, 2026 at 07:31:10AM +0200, Stefano Brivio wrote: > On Thu, 2 Jul 2026 15:21:29 +1000 > David Gibson wrote: >=20 > > On Thu, Jul 02, 2026 at 06:58:23AM +0200, Stefano Brivio wrote: [snip] > > > > @@ -481,6 +495,12 @@ static bool parse_port_chunk(const char **curs= or, > > > > kind =3D CHUNK_INCLUDE; > > > > =20 > > > > if (parse_literal(&p, ":")) { > > > > + const char *tgtspec =3D p; > > > > + > > > > + if (!parse_inany(&p, &taddr_tmp) || > > > > + !parse_literal(&p, "/")) =20 > > >=20 > > > This is to support ":/PORT" together with ":ADDR/PORT", right? =20 > >=20 > > No, this is checking for the case where we didn't get a target address > > at all. De Morgan's law might make it clearer: > > !( && =20 > Right, yes, I understand, but you're doing the check this way because > "/PORT" without address would be accepted, right? No, I'm not, and it won't - in fact it's almost exactly the opposite. If parse_inany() fails (which it will on an empty "address"), we won't even call parse_literal() because of the rules of || > Otherwise checking > for !parse_inany(&p, &taddr_tmp) would have been enough? Not really, this is here so that we *don't* do anything with an address that *isn't* followed by a /. Instead we backtrack and try parse as something else instead. In fact that will (I'm pretty sure, necessarily) also fail, but that's looking at parts of the grammar outside what this specific piece is dealing with. The idea here is that we consume either "ADDR/" or we consume nothing at all before going on to parse the port range. > Anyway, not so important, if the next version looks different anyway. Right, though whether it's any less confusing is another question :/. Like I said, I'm not thrilled with how this parsing stuff turned out, but I spent two weeks and couldn't come up with something that wasn't at least as bad in one way or another. > > But I already reworked this to allow port to be omitted with address, > > and I think the new code is clearer. Or at least unclear in an > > unrelated way :/. > >=20 > > > I think > > > it's nice to have for users, but it looks inconsistent here, so maybe > > > you could add a comment before that, say: > > >=20 > > > /* Accept :/PORT as well as :ADDR/PORT */ > > > =20 > > > > + p =3D tgtspec; /* No target address, backtrack */ > > > > + > > > > if (!parse_port_range(&p, &tr)) > > > > return false; > > > > } else { > > > > @@ -492,6 +512,8 @@ static bool parse_port_chunk(const char **curso= r, > > > > =20 > > > > *kindp =3D kind; > > > > *lrange =3D lr; > > > > + if (taddr) > > > > + *taddr =3D taddr_tmp; > > > > if (trange) > > > > *trange =3D tr; > > > > *cursor =3D p; > > > > @@ -561,7 +583,7 @@ static void fwd_rule_parse_ports(struct fwd_tab= le *fwd, bool del, uint8_t proto, > > > > /* Consider excluded ranges and "auto" in the first pass */ > > > > p =3D spec; > > > > do { > > > > - if (!parse_port_chunk(&p, &kind, &lrange, NULL)) > > > > + if (!parse_port_chunk(&p, &kind, &lrange, NULL, NULL)) > > > > goto bad; > > > > =20 > > > > switch (kind) { > > > > @@ -586,8 +608,9 @@ static void fwd_rule_parse_ports(struct fwd_tab= le *fwd, bool del, uint8_t proto, > > > > p =3D spec; > > > > do { > > > > struct port_range trange; > > > > + union inany_addr taddr; > > > > =20 > > > > - if (!parse_port_chunk(&p, &kind, &lrange, &trange)) > > > > + if (!parse_port_chunk(&p, &kind, &lrange, &taddr, &trange)) > > > > goto bad; > > > > =20 > > > > switch (kind) { > > > > @@ -604,7 +627,8 @@ static void fwd_rule_parse_ports(struct fwd_tab= le *fwd, bool del, uint8_t proto, > > > > =20 > > > > fwd_rule_range_except(fwd, del, proto, addr, ifname, > > > > lrange.first, lrange.last, > > > > - exclude, trange.first, flags); > > > > + exclude, &taddr, trange.first, > > > > + flags); > > > > break; > > > > default: > > > > goto bad; > > > > @@ -620,7 +644,7 @@ static void fwd_rule_parse_ports(struct fwd_tab= le *fwd, bool del, uint8_t proto, > > > > =20 > > > > fwd_rule_range_except(fwd, del, proto, addr, ifname, > > > > 1, NUM_PORTS - 1, exclude, > > > > - 1, flags | FWD_WEAK); > > > > + NULL, 1, flags | FWD_WEAK); > > > > } > > > > return; > > > > bad: =20 > > >=20 > > > The rest of the series looks good to me, but I didn't test things at > > > all. By the way, man page and usage changes for 3/3 are missing. =20 > >=20 > > Ugh, yeah, I belatedly realised that. > >=20 > > > I haven't seen anything preventing mapping between IPv4 and IPv6, but= I > > > guess I missed something. I actually think it would be a nice feature > > > but I guess it needs some more effort to properly support it. =20 > >=20 > > Oh sod, good point. I forgot to add that check. I think we need it > > for now, because I'm pretty sure it won't work end to end yet. >=20 > I guess probably worth a quick check, I wouldn't be surprised if things > actually kind of work for TCP. Yes, but there are a lot of paths to check, so I'd prefer to explicitly exclude this for the first spin. I've now added that, though it has the mildly unfortunate side effect that it's now (usually) impossible to specify a target address without listening address (Because -t 5000:192.0.2.1 means */5000:192.0.2.1/5000.. and since * listens on IPv6 it would mean family translation in at least some cases). Rechecking and relaxing this is high on the list of followons. --=20 David Gibson (he or they) | 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 --vZWsi6KhDfdD6B3T Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmpGAGIACgkQzQJF27ox 2Ge8cw//R8Snaudi2aHAbaHtBPxj//JfebXD3RQX41dzCm790B2Kw7m2+8ZgLJ6S LXTmAXp+FFURlnxLsYJUMQpTMATEGhnG3wZUvFjhNsMcV8zsAkGCcYGo6apSyAk5 WmCmasqf2kGnuwgqA0bMwstU+hQ436XdBt2zKIYazj6QePLZvwChwryUrJGuqCU6 7s9KSfWxiJ5wAhCkf33uYxLNDRLx3LKOKVHD42lqfgxl/WUnkB0XaTtLjFx5lMMt AlKLURGCuZkLNpYlF908wF+KOq7VStohKIZLxMTlsX923oM35Y6uLot3R/Wr9fyw 1CV+fKHOFMOQaFrE+lmYmeBAh6wTY3ESiMFAz/02WPSav5Tx+oQ8n/R+C6wjlnAk gFLNU8iBX2fwV3dNvuDFnMmY+WgAVc3ApiUYjdsPInUjHLNB6bOcZIZbgCTuqlAK grMnZZd57stV6S0B0XGFI9d22EoIDZzWHI+7UTz0+NSbQMZ9QcWpCSPQdm9rEAQD VuyLzjsbf36/occEtcNyGhOTbLi4xZ4UBBjlW2Tqq0ec2x6TzvqQX19cBrNwLH8+ jGbMKzBSPl5AX6ah3ojXn/WTqBUREt6kweSvgm0TS1p1FJX6+kbS2eFeCyTP8qDL HetpuCutx6ru67EWdjtOQRuubiRLY1XRStEOT0xdkeG0Aw5rSKs= =MX7N -----END PGP SIGNATURE----- --vZWsi6KhDfdD6B3T--