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=202502 header.b=MByZF2uq; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id E0AA15A0274 for ; Thu, 20 Feb 2025 04:13:26 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202502; t=1740021190; bh=YoK0KvWWRJFRk8/UFpsaWj645wUXN/fcSsH4ZUZf4Ik=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=MByZF2uqXglVDdxf1DHS7J5O6j5VivCLoi3WCON+jF9rU8xRbYwD1BxeqUZIcDqXn ZvoekR1H7Mcm5RNld6BaNWq5Y8uIIXQue5wD1QpltGd0ozKPOnA6kVvd6mQxBTYIU2 er+qcofZLh2WpZGc3W/HmXgQM0QmmxlZ/qyyn4sT4RvZRVlUBbmosi7bfr1IdbyyGq vna3jxkLGOFFY/pEKwJOWnZ7E3o6vnEhGs0gX7cCPgW3xJ4wGf+5wi9nBIZxPOIk4E ZwteKdB4eIFG3nICM84Vadali1za+siElTeNdRQcI100bdps7tnr3tUPCUUIWiSDMv 2XI2ghsjqaAuw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Yyyxy1jDyz4wyk; Thu, 20 Feb 2025 14:13:10 +1100 (AEDT) Date: Thu, 20 Feb 2025 14:13:00 +1100 From: David Gibson To: Jon Maloy Subject: Re: [PATCH v3 2/2] udp: create and send ICMPv4 to local peer when applicable Message-ID: References: <20250219193007.2336670-1-jmaloy@redhat.com> <20250219193007.2336670-3-jmaloy@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="AmNwCp8dtdccyg7B" Content-Disposition: inline In-Reply-To: <20250219193007.2336670-3-jmaloy@redhat.com> Message-ID-Hash: TU5UTEVHE7OAPY227WBCCZX7APYQAGDB X-Message-ID-Hash: TU5UTEVHE7OAPY227WBCCZX7APYQAGDB 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, sbrivio@redhat.com, lvivier@redhat.com, dgibson@redhat.com 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: --AmNwCp8dtdccyg7B Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 19, 2025 at 02:30:07PM -0500, Jon Maloy wrote: > When a local peer sends a UDP message to a non-existing port on an > existing remote host, that host will return an ICMP message containing > the error code ICMP_PORT_UNREACH, plus the header and the first eight > bytes of the original message. If the sender socket has been connected, > it uses this message to issue a "Connection Refused" event to the user. >=20 > Until now, we have only read such events from the externally facing > socket, but we don't forward them back to the local sender because > we cannot read the ICMP message directly to user space. Because of > this, the local peer will hang and wait for a response that never > arrives. >=20 > We now fix this for IPv4 by recreating and forwarding a correct ICMP > message back to the internal sender. We synthesize the message based > on the information in the extended error structure, plus the returned > part of the original message body. >=20 > Note that for the sake of completeness, we even produce ICMP messages > for other error codes. We have noticed that at least ICMP_PROT_UNREACH > is propagated as an error event back to the user. >=20 > Signed-off-by: Jon Maloy Almost there... [snip] > ee =3D (const struct sock_extended_err *)CMSG_DATA(hdr); > - > - /* TODO: When possible propagate and otherwise handle errors */ > + if (ee->ee_type =3D=3D ICMP_DEST_UNREACH) { > + flow_sidx_t sidx; > + struct udp_flow *flow; > + const struct flowside *toside; > + > + if (ref.type =3D=3D EPOLL_TYPE_UDP_LISTEN) { > + sidx =3D flow_lookup_sa(c, IPPROTO_UDP, ref.udp.pif, > + &saddr, ref.udp.port); > + flow =3D udp_at_sidx(sidx); > + if (!flow) { > + err("Unexpected cmsg reading error queue"); > + return -1; This is too strong a response if you can't find a flow. This will happen if we get some random ICMP which doesn't match an existing flow. It could be a long delayed ICMP from some flow that already expired, some peer confused by something, or a malicious actor. This will log a hard error (without rate limiting) and break scanning for any additional errors. I think we want just a trace() and return 1; here (we did receive and process an error - but all we could do was drop it). > + } > + flow->ts =3D now->tv_sec; > + sidx =3D flow_sidx_opposite(sidx); > + } else { > + sidx =3D flow_sidx_opposite(ref.flowside); > + } > + toside =3D flowside_at_sidx(sidx); > + udp_send_conn_fail_icmp4(c, ee, toside, udp_data, rc); > + } > debug("%s error on UDP socket %i: %s", > str_ee_origin(ee), s, strerror_(ee->ee_errno)); > =20 > @@ -461,15 +528,18 @@ static int udp_sock_recverr(int s) > /** > * udp_sock_errs() - Process errors on a socket > * @c: Execution context > - * @s: Socket to receive from > + * @ref: epoll reference > * @events: epoll events bitmap > + * @now: Current timestamp > * > * Return: Number of errors handled, or < 0 if we have an unrecoverable = error > */ > -int udp_sock_errs(const struct ctx *c, int s, uint32_t events) > +int udp_sock_errs(const struct ctx *c, union epoll_ref ref, uint32_t eve= nts, > + const struct timespec *now) > { > unsigned n_err =3D 0; > socklen_t errlen; > + int s =3D ref.fd; > int rc, err; > =20 > ASSERT(!c->no_udp); > @@ -478,7 +548,7 @@ int udp_sock_errs(const struct ctx *c, int s, uint32_= t events) > return 0; /* Nothing to do */ > =20 > /* Empty the error queue */ > - while ((rc =3D udp_sock_recverr(s)) > 0) > + while ((rc =3D udp_sock_recverr(c, ref, now)) > 0) > n_err +=3D rc; > =20 > if (rc < 0) > @@ -558,7 +628,7 @@ static void udp_buf_listen_sock_handler(const struct = ctx *c, > const socklen_t sasize =3D sizeof(udp_meta[0].s_in); > int n, i; > =20 > - if (udp_sock_errs(c, ref.fd, events) < 0) { > + if (udp_sock_errs(c, ref, events, now) < 0) { > err("UDP: Unrecoverable error on listening socket:" > " (%s port %hu)", pif_name(ref.udp.pif), ref.udp.port); > /* FIXME: what now? close/re-open socket? */ > @@ -661,7 +731,7 @@ static void udp_buf_reply_sock_handler(const struct c= tx *c, union epoll_ref ref, > =20 > from_s =3D uflow->s[ref.flowside.sidei]; > =20 > - if (udp_sock_errs(c, from_s, events) < 0) { > + if (udp_sock_errs(c, ref, events, now) < 0) { > flow_err(uflow, "Unrecoverable error on reply socket"); > flow_err_details(uflow); > udp_flow_close(c, uflow); > diff --git a/udp_internal.h b/udp_internal.h > index cc80e30..c5f8304 100644 > --- a/udp_internal.h > +++ b/udp_internal.h > @@ -30,5 +30,6 @@ size_t udp_update_hdr4(struct iphdr *ip4h, struct udp_p= ayload_t *bp, > size_t udp_update_hdr6(struct ipv6hdr *ip6h, struct udp_payload_t *bp, > const struct flowside *toside, size_t dlen, > bool no_udp_csum); > -int udp_sock_errs(const struct ctx *c, int s, uint32_t events); > +int udp_sock_errs(const struct ctx *c, union epoll_ref ref, uint32_t eve= nts, > + const struct timespec *now); > #endif /* UDP_INTERNAL_H */ > diff --git a/udp_vu.c b/udp_vu.c > index 4123510..0b1d3c6 100644 > --- a/udp_vu.c > +++ b/udp_vu.c > @@ -227,7 +227,7 @@ void udp_vu_listen_sock_handler(const struct ctx *c, = union epoll_ref ref, > struct vu_virtq *vq =3D &vdev->vq[VHOST_USER_RX_QUEUE]; > int i; > =20 > - if (udp_sock_errs(c, ref.fd, events) < 0) { > + if (udp_sock_errs(c, ref, events, now) < 0) { > err("UDP: Unrecoverable error on listening socket:" > " (%s port %hu)", pif_name(ref.udp.pif), ref.udp.port); > return; > @@ -302,7 +302,7 @@ void udp_vu_reply_sock_handler(const struct ctx *c, u= nion epoll_ref ref, > =20 > ASSERT(!c->no_udp); > =20 > - if (udp_sock_errs(c, from_s, events) < 0) { > + if (udp_sock_errs(c, ref, events, now) < 0) { > flow_err(uflow, "Unrecoverable error on reply socket"); > flow_err_details(uflow); > udp_flow_close(c, uflow); --=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 --AmNwCp8dtdccyg7B Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAme2nagACgkQzQJF27ox 2GfDwA/9EiKQZcPzUM9G86t2J/XGhJAJInH6QaDCbTm7sk3MfLh4OGZqfKCk6UHp VoBR1gZHHuv8TaDLcSvdaonJ3Oqvpm56Os2P8SoCbBcSNVl1/iPBEPUgAbbBmIKn i1CB9dLQkb1B1IaFwLCWpXgfUfHyndFiSeGFMas9xgMdtDauXLc5PcKfzqGF7ys1 WcULmaomhgn9+NhHnnTxqgbbh6UfYYEif/Qpd7Zj5s6W77qpAA57uwn3Ft3csBKu +QyNw5PrJDZuiphatjfz9z03K9mtcjVGfeR1W3BfD2ENOxg2ACtDBvRjorS5pz+O PI9HLiD2D9GIX1SQmhnlik3GjNMJsgEhz6E3Rqj4DHb/HsvfSKbAwvMH3TFio6PP 7grszaOLw3zjnP7ajCv6PqxT70fSywOmZBF1VElhjhPwq0Sm26PRm0TMP8aEagF/ x6WGXB2t1ayZVv2ICGTWH2JPuU50Gzpuaw3hazaUwgweea5zJTCmvXyUBAlxLUM0 Jg16DhcapP5QxFZzfRQ/Wnvk8CnH6JGtOZ98Dv2nHAI0R/dTaPVgACwioex/uVUd EMchJBB0d0s4NoUemvnprk/pzSeSPRXfO1RITQZK1pxNJ9wtBpfUBDyVEK/fAwxS 7T63R8iG50c5aRvPODCCwelYRr275t+KP8c/E7JukeahvskH0FY= =2wOX -----END PGP SIGNATURE----- --AmNwCp8dtdccyg7B--