From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine 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=ZCOzlGQv; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id 9160B5A0279 for ; Wed, 10 Sep 2025 11:29:23 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1757496562; 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=7U9FWYX3UrPP5W2eEzOi6HoZugak3Xf8Np10e09RZJQ=; b=ZCOzlGQvEmjCGXNoOEDwtuUH80OBlzdC8oRT8unQlT8m6M+YObO8ZZ4x0d1RPwlTK1rsUY kJ7UGlwNqjf8GW+PP+MI969Y6RRzvXN5pTEAGkkxoLFZPEFeHT4rFiF6oMHhktMThEhvG6 EP5jexjCibc+BqjbIJOyzF3uwYmuBVY= 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-435-VDCFwrR3PvCTdXc1LcQyzQ-1; Wed, 10 Sep 2025 05:29:20 -0400 X-MC-Unique: VDCFwrR3PvCTdXc1LcQyzQ-1 X-Mimecast-MFC-AGG-ID: VDCFwrR3PvCTdXc1LcQyzQ_1757496560 Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-3e403b84456so3402824f8f.0 for ; Wed, 10 Sep 2025 02:29:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1757496559; x=1758101359; 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=7U9FWYX3UrPP5W2eEzOi6HoZugak3Xf8Np10e09RZJQ=; b=MzvXasXGxiFxSDCIownx2gg4XqrdA4c5kSDcRxWYMql1aBg6YDokqaTDc5D1XAe+1a MiQ48pnA0MV311LzPiILX7j9kL275GhvSYCRetg6TZTRdKcnZdnO55XY8VjDt7Wwq7fP nWGxMwpK6MPZ5eNZbPWjW5Y1Htqc5Cpor+3uQGgggw5cPU9Vyp2oLaYcmhQrZX75fHrB D6Pc5BatwDzdol7q/em5LkDF00vvzXczEXdO65LddtTqcdC+YMM9ktFl8mpxknFRzyhe xceJyVqiLYy8P+2Wum3oqQfzcwv3YtN2DvV3dbgGZymNGMgsBP0qTb6iLnb31y8LFSNC /OWg== X-Gm-Message-State: AOJu0YynLUtG9TRYC4tgXTldiqrEre+1itVOdwBOkdEnFE6gJL/1gNyN 9T8kU4HRGVe9PhPR+Ygfx93K9M84j8a9N+pZBJU8GT6I9p3VhjbNklzfM8Dj5fVdc34n3rT9/QC E9Me6hysdKkbPwj2+TZck5sCgGaChJB8vzVcVN88e2CBbVPFHuUZv7Q== X-Gm-Gg: ASbGncvELS7mEyeYiXEB1APWKwtPq6vvqGQHJS/uXF2JVHU0Q7KODnq25dZxME0oUJX A7LNIK6HxDwZ7sL4Gj0FSdt4J4y1eQxx8QD9g1Z9NP/7ZhZUxHGnskCF156+43bmqrBkTanFksj wAwp0BT+qJ/yISGWj4+jj4x8LFwRuhqmTOehAO1sGtaYVfVG6YBZ29bd7fvbqR48lfdPYdJvbLL XM99S4N6hOdqC+SPD0rCbvccjgUXPcREcNkm+SUBekLuttaObXkTg78IOJoExNgRv5UdjWo9tlH MLqehTpJjSva05g3fzdTwy0GGeuOmkhqryB2+ohyDysf1aljLDDapXMZvqfANXAJfMEM X-Received: by 2002:a05:6000:184d:b0:3e7:4265:66de with SMTP id ffacd0b85a97d-3e742656b0emr8397353f8f.8.1757496559415; Wed, 10 Sep 2025 02:29:19 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEHvv5X/HbixDhE9KKT9neDuiu3MrJAhrRAp392bx+9mxN2UwIV4n5BZbNuZ7apVmH2DB2UIw== X-Received: by 2002:a05:6000:184d:b0:3e7:4265:66de with SMTP id ffacd0b85a97d-3e742656b0emr8397335f8f.8.1757496558882; Wed, 10 Sep 2025 02:29:18 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3e75224d04dsm6063568f8f.55.2025.09.10.02.29.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 10 Sep 2025 02:29:18 -0700 (PDT) Date: Wed, 10 Sep 2025 11:29:06 +0200 From: Stefano Brivio To: Volker Diels-Grabsch Subject: Re: [PATCH] Send an initial ARP and NDP request to resolve the guest IP address Message-ID: <20250910112906.5e1b7e5a@elisabeth> In-Reply-To: <20250909145516.762957-2-v@njh.eu> References: <20250909145516.762957-1-v@njh.eu> <20250909145516.762957-2-v@njh.eu> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: vdhFQcHx0AYS7haIImv_dWQT1NHfwzkOgNiTxH7D1a4_1757496560 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: V26AUTXSQYUY2UQWVBRMQOJE5KHTBAGT X-Message-ID-Hash: V26AUTXSQYUY2UQWVBRMQOJE5KHTBAGT 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, David Gibson 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, 9 Sep 2025 16:49:20 +0200 Volker Diels-Grabsch wrote: > When restarting passt while QEMU keeps running with a configured > "reconnect-ms" setting, the port forwardings will stop working until > the guest sends some outgoing network traffic. > > Reason: Although QEMU reconnects successfully to the unix domain > socket of the new passt process, that one no longer knows the guest's > MAC address and uses instead the broadcast MAC address. However, this > is ignored by the guest, at least if the guest runs Linux. Only after > the guest sends some network package on its own initiative, passt will > know the MAC address and will be able to establish forwarded > connections. > > This change fixes this issue by sending an ARP and an NDP request to > resolve the guest's MAC address via its IPv4 and IPv6 address, which > we do know, right after the unix domain socket (re)connection. > > The only case where the IP is "wrong" would be if the configuration > changed, or on the very first start right after qemu started. But in > those cases, we just wouldn't get an ARP/NDP response, and can't do > anything until we receive the guest's DHCP request - just as before. > In other words, in the worst case the ARP/NDP requests would be > harmless. Thanks for the implementation, this looks like a small but quite relevant feature we missed until now. I have a couple of comments on top of David's ones: > Signed-off-by: Volker Diels-Grabsch > --- > arp.c | 33 +++++++++++++++++++++++++++++++++ > arp.h | 1 + > ndp.c | 19 +++++++++++++++++++ > ndp.h | 1 + > tap.c | 16 ++++++++++++---- > util.h | 1 + > 6 files changed, 67 insertions(+), 4 deletions(-) > > diff --git a/arp.c b/arp.c > index 44677ad..c1bd63b 100644 > --- a/arp.c > +++ b/arp.c > @@ -112,3 +112,36 @@ int arp(const struct ctx *c, struct iov_tail *data) > > return 1; > } > + > +/** > + * arp_send_init_req() - Send initial ARP request to retrieve guest MAC address > + * @c: Execution context > + */ > +void arp_send_init_req(const struct ctx *c) > +{ > + struct { > + struct ethhdr eh; > + struct arphdr ah; > + struct arpmsg am; > + } __attribute__((__packed__)) req; > + > + /* Ethernet header */ > + req.eh.h_proto = htons(ETH_P_ARP); > + memcpy(req.eh.h_dest, MAC_BROADCAST, sizeof(req.eh.h_dest)); > + memcpy(req.eh.h_source, c->our_tap_mac, sizeof(req.eh.h_source)); > + > + /* ARP header */ > + req.ah.ar_op = htons(ARPOP_REQUEST); > + req.ah.ar_hrd = htons(ARPHRD_ETHER); > + req.ah.ar_pro = htons(ETH_P_IP); > + req.ah.ar_hln = ETH_ALEN; > + req.ah.ar_pln = 4; > + > + /* ARP message */ > + memcpy(req.am.sha, c->our_tap_mac, sizeof(req.am.sha)); > + memcpy(req.am.sip, &c->ip4.our_tap_addr, sizeof(req.am.sip)); > + memcpy(req.am.tha, MAC_BROADCAST, sizeof(req.am.tha)); > + memcpy(req.am.tip, &c->ip4.addr, sizeof(req.am.tip)); > + > + tap_send_single(c, &req, sizeof(req)); > +} > diff --git a/arp.h b/arp.h > index 86bcbf8..d5ad0e1 100644 > --- a/arp.h > +++ b/arp.h > @@ -21,5 +21,6 @@ struct arpmsg { > } __attribute__((__packed__)); > > int arp(const struct ctx *c, struct iov_tail *data); > +void arp_send_init_req(const struct ctx *c); > > #endif /* ARP_H */ > diff --git a/ndp.c b/ndp.c > index eb090cd..b3bdedb 100644 > --- a/ndp.c > +++ b/ndp.c > @@ -438,3 +438,22 @@ void ndp_timer(const struct ctx *c, const struct timespec *now) > first: > next_ra = now->tv_sec + interval; > } > + > +/** > + * ndp_send_init_req() - Send initial NDP NS to retrieve guest MAC address > + * @c: Execution context > + */ > +void ndp_send_init_req(const struct ctx *c) > +{ > + struct ndp_ns ns = { > + .ih = { > + .icmp6_type = NS, > + .icmp6_code = 0, > + .icmp6_router = 0, /* Reserved */ > + .icmp6_solicited = 0, /* Reserved */ > + .icmp6_override = 0, /* Reserved */ > + }, > + .target_addr = c->ip6.addr > + }; > + ndp_send(c, &c->ip6.addr, &ns, sizeof(ns)); > +} > diff --git a/ndp.h b/ndp.h > index b1dd5e8..781ea86 100644 > --- a/ndp.h > +++ b/ndp.h > @@ -11,5 +11,6 @@ struct icmp6hdr; > int ndp(const struct ctx *c, const struct in6_addr *saddr, > struct iov_tail *data); > void ndp_timer(const struct ctx *c, const struct timespec *now); > +void ndp_send_init_req(const struct ctx *c); > > #endif /* NDP_H */ > diff --git a/tap.c b/tap.c > index 7ba6399..ea61eae 100644 > --- a/tap.c > +++ b/tap.c > @@ -1088,6 +1088,7 @@ void tap_add_packet(struct ctx *c, struct iov_tail *data, > { > struct ethhdr eh_storage; > const struct ethhdr *eh; > + char bufmac[ETH_ADDRSTRLEN]; > > pcap_iov(data->iov, data->cnt, data->off); > > @@ -1097,6 +1098,7 @@ void tap_add_packet(struct ctx *c, struct iov_tail *data, > > if (memcmp(c->guest_mac, eh->h_source, ETH_ALEN)) { > memcpy(c->guest_mac, eh->h_source, ETH_ALEN); > + info("Guest MAC address: %s", eth_ntop(c->guest_mac, bufmac, sizeof(bufmac))); > proto_update_l2_buf(c->guest_mac, NULL); > } > > @@ -1355,6 +1357,11 @@ static void tap_start_connection(const struct ctx *c) > ev.events = EPOLLIN | EPOLLRDHUP; > ev.data.u64 = ref.u64; > epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev); > + > + info("Sending initial ARP and NDP request to retrieve" > + " guest MAC address after reconnect"); > + arp_send_init_req(c); This should be conditional to whether we have IPv4 support enabled or not, and the check would need to be analogous to the one from tap4_handler() (sorry, it's a bit hidden): if (!c->ifi4 || ...) return ...; > + ndp_send_init_req(c); And this should only happen if IPv6 is enabled, see tap6_handler(): if (!c->ifi6 || ...) return ...; and also, arguably, iff NDP support is not disabled by means of --no-ndp (c->no_ndp). Strictly speaking, we could send this anyway and still fit the current documentation of --no-ndp: --no-ndp Disable NDP responses. NDP messages coming from guest or target namespace will be ignored. but this would make --no-ndp a misnomer, and given that we'll ignore neighbour advertisements, it makes no sense to send a solicitation anyway. All in all, I would just not do this on c->no_ndp. If you can think of a terse way of updating the man page to reflect this, that would be appreciated, but I think it's also fine like it is. By the way, we'll also ignore responses on --no-icmp. I just realised that the man page is currently inaccurate, because it refers to echo messages only, but in tap6_handler() we have: if (proto == IPPROTO_ICMPV6) { ... if (c->no_icmp) continue; ... if (ndp(c, saddr, &ndp_data)) continue; ... } So I think we should update the man page to mention that --no-icmp means no ICMP and no ICMPv6, and also skip sending the NDP solicitation in that case. Or update the code to reflect what the man page says, but then the option could be considered a misnomer, so I wouldn't go this way. > } > > /** > @@ -1503,11 +1510,12 @@ void tap_backend_init(struct ctx *c) > case MODE_PASST: > tap_sock_unix_init(c); > > - /* In passt mode, we don't know the guest's MAC address until it > - * sends us packets. Use the broadcast address so that our > - * first packets will reach it. > + /* In passt mode, we don't know the guest's MAC address until > + * it sends us packets (e.g. responds to our initial ARP or I don't think the response is an example, so I wouldn't use "e.g." here, rather "i.e." / "that is", if that's the expected behaviour. > + * NDP request). Until then, use the broadcast address so > + * that our first packets will have a chance to reach it. > */ > - memset(&c->guest_mac, 0xff, sizeof(c->guest_mac)); > + memcpy(&c->guest_mac, MAC_BROADCAST, sizeof(c->guest_mac)); > break; > } > > diff --git a/util.h b/util.h > index 2a8c38f..3719f0c 100644 > --- a/util.h > +++ b/util.h > @@ -97,6 +97,7 @@ void abort_with_msg(const char *fmt, ...) > #define FD_PROTO(x, proto) \ > (IN_INTERVAL(c->proto.fd_min, c->proto.fd_max, (x))) > > +#define MAC_BROADCAST ((uint8_t [ETH_ALEN]){ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }) This can be easily wrapped to fit 80 columns without otherwise affecting readability, see examples just above and below: #define MAC_BROADCAST \ ((uint8_t [ETH_ALEN]){ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }) > #define MAC_ZERO ((uint8_t [ETH_ALEN]){ 0 }) > #define MAC_IS_ZERO(addr) (!memcmp((addr), MAC_ZERO, ETH_ALEN)) > The rest looks good to me! -- Stefano