From mboxrd@z Thu Jan 1 00:00:00 1970 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 BA86C5A0262 for ; Thu, 4 May 2023 23:53:16 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1683237195; 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=YD5VT+wGOI1i/acmpLaEQHEBkaArbTt7Yp82KYQHS4U=; b=JY/w37A1HYYN7xcTXgAxrnwzzvTA88j0a3N+m6Rqrlyhmj8uy5MBxaaST7BYoqp80KAL77 DjEX7Zn/IXXhMoW/IwLXsNmqCmX1G4FTFi8l6eeSLURYDCDq6S3Kpm7F1WxJQykZgPI/NZ UMOhu+3EHj4FV3TkzLZ5hGjcfTiG+3s= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-529-2h852JbrMC-JB3K6v6_DRw-1; Thu, 04 May 2023 17:53:14 -0400 X-MC-Unique: 2h852JbrMC-JB3K6v6_DRw-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 1C6581C060E9; Thu, 4 May 2023 21:53:14 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.8]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6D966492C13; Thu, 4 May 2023 21:53:13 +0000 (UTC) Date: Thu, 4 May 2023 23:53:08 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 2/7] udp: Simplify setting od destination IPv6 address for inbound packets Message-ID: <20230504235308.76f2b260@elisabeth> In-Reply-To: <20230501110702.3915529-3-david@gibson.dropbear.id.au> References: <20230501110702.3915529-1-david@gibson.dropbear.id.au> <20230501110702.3915529-3-david@gibson.dropbear.id.au> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: NCHRVJU2SDM3FR5AL6YXZTJ3JLPFXVXT X-Message-ID-Hash: NCHRVJU2SDM3FR5AL6YXZTJ3JLPFXVXT 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 Mon, 1 May 2023 21:06:57 +1000 David Gibson wrote: > Depending on several possible NAT situations, udp_update_hdr6() has a few > different paths for setting the destination IPv6 address. However, this > isn't really separate from the selection of the IPv6 source address: > if the adjusted source address is link-local then we need to use a link > local destination, otherwise we need the global destination address. > > This fixes a small bug: it's theoretically possible, although unlikely, for > the dns_match address to be link-local, in which case the previous code > would have used the wrong destination. > > This does do slightly more work in the case of NATting packets originating > on the host. Currently we always NAT those to a link-local address so we > could statically set a link-local destination address, but we now recheck. > However, as well as being simpler, the new approach is more robust if we > want to allow non-link-local NAT-to-host addresses, which we do plan to in > future. > > Signed-off-by: David Gibson > --- > udp.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/udp.c b/udp.c > index 361f24c..9c96c0f 100644 > --- a/udp.c > +++ b/udp.c > @@ -640,18 +640,13 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport, > > b->ip6h.payload_len = htons(udp6_l2_mh_sock[n].msg_len + sizeof(b->uh)); > > - if (IN6_IS_ADDR_LINKLOCAL(src)) { > - b->ip6h.daddr = c->ip6.addr_ll_seen; > - } else if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_match) && > + if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_match) && > IN6_ARE_ADDR_EQUAL(src, &c->ip6.dns_host) && > src_port == 53) { > - b->ip6h.daddr = c->ip6.addr_seen; > src = &c->ip6.dns_match; > } else if (IN6_IS_ADDR_LOOPBACK(src) || > IN6_ARE_ADDR_EQUAL(src, &c->ip6.addr_seen) || > IN6_ARE_ADDR_EQUAL(src, &c->ip6.addr)) { > - b->ip6h.daddr = c->ip6.addr_ll_seen; > - This part... > udp_tap_map[V6][src_port].ts = now->tv_sec; > udp_tap_map[V6][src_port].flags |= PORT_LOCAL; > > @@ -671,11 +666,13 @@ static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstport, > src = &c->ip6.gw; > else > src = &c->ip6.addr_ll; > - } else { > - b->ip6h.daddr = c->ip6.addr_seen; > } > > b->ip6h.saddr = *src; > + if (IN6_IS_ADDR_LINKLOCAL(src)) > + b->ip6h.daddr = c->ip6.addr_ll_seen; > + else > + b->ip6h.daddr = c->ip6.addr_seen; is not equivalent to this, and I guess not by intention. The rationale for setting a link-local destination address if the source address (on the host) is the loopback address (commit 86b273150a47 "tcp, udp: Allow binding ports in init namespace to both tap and loopback"... not a great description) is that we might otherwise collide with a global unicast destination address also assigned to the host. A link-local destination should be the only safe option. After all, that's what link-local addresses are for: surely a packet with that source address is local to the "link" to the guest. If the source address matches the configured or observed address (presumably local), the same reasoning applies: we want to actually send that packet to the guest, and signal that it's local. I guess this change might explain the failures you see: if we already saw a non-link-local address from the guest, we'll use that as destination, and the packet won't reach the guest. The rest looks correct to me, and also the rest of the series... just, as you mentioned, if we want to generalise this right away (and I think it's a better approach), patches from 3/7 on will be substantially different. -- Stefano