Re: [PATCH 7/7] udp: Propagate errors on listening and brand new sockets

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 15 Apr 2025 17:16:24 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> udp_sock_recverr() processes errors on UDP sockets and attempts to
> propagate them as ICMP packets on the tap interface.  To do this it
> currently requires the flow with which the error is associated as a
> parameter.  If that's missing it will clear the error condition, but not
> propagate it.
> 
> That means that we largely ignore errors on "listening" sockets.  It also
> means we may discard some errors on flow specific sockets if they occur
> very shortly after the socket is created.  In udp_flush_flow() we need to
> clear any datagrams received between bind() and connect() which might not
> be associated with the "final" flow for the socket.  If we get errors
> before that point we'll ignore them in the same way because we don't know
> the flow they're associated with in advance.
> 
> This can happen in practice if we have errors which occur almost
> immediately after connect(), such as ECONNREFUSED when we connect() to a
> local address where nothing is listening.
> 
> Between the extended error message itself and the PKTINFO information we
> do actually have enough information to find the correct flow.  So, rather
> than ignoring errors where we don't have a flow "hint", determine the flow
> the hard way in udp_sock_recverr().
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  udp.c | 41 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/udp.c b/udp.c
> index 6db3accc..c04a091b 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -504,27 +504,34 @@ static int udp_pktinfo(struct msghdr *msg, union inany_addr *dst)
>   * @c:		Execution context
>   * @s:		Socket to receive errors from
>   * @sidx:	Flow and side of @s, or FLOW_SIDX_NONE if unknown
> + * @pif:	Interface on which the error occurred
> + *              (only used if @sidx == FLOW_SIDX_NONE)
> + * @port:	Local port number of @s (only used if @sidx == FLOW_SIDX_NONE)
>   *
>   * Return: 1 if error received and processed, 0 if no more errors in queue, < 0
>   *         if there was an error reading the queue
>   *
>   * #syscalls recvmsg
>   */
> -static int udp_sock_recverr(const struct ctx *c, int s, flow_sidx_t sidx)
> +static int udp_sock_recverr(const struct ctx *c, int s, flow_sidx_t sidx,
> +			    uint8_t pif, in_port_t port)
>  {
>  	struct errhdr {
>  		struct sock_extended_err ee;
>  		union sockaddr_inany saddr;
>  	};
>  	char buf[PKTINFO_SPACE + CMSG_SPACE(sizeof(struct errhdr))];
> +	const struct errhdr *eh = NULL;
>  	char data[ICMP6_MAX_DLEN];
> -	const struct errhdr *eh;
>  	struct cmsghdr *hdr;
>  	struct iovec iov = {
>  		.iov_base = data,
>  		.iov_len = sizeof(data)
>  	};
> +	union sockaddr_inany src;
>  	struct msghdr mh = {
> +		.msg_name = &src,
> +		.msg_namelen = sizeof(src),
>  		.msg_iov = &iov,
>  		.msg_iovlen = 1,
>  		.msg_control = buf,
> @@ -554,7 +561,7 @@ static int udp_sock_recverr(const struct ctx *c, int s, flow_sidx_t sidx)
>  		      hdr->cmsg_type == IP_RECVERR) ||
>  		     (hdr->cmsg_level == IPPROTO_IPV6 &&
>  		      hdr->cmsg_type == IPV6_RECVERR))
> -		    break;
> +			break;
>  	}
>  
>  	if (!hdr) {
> @@ -568,8 +575,19 @@ static int udp_sock_recverr(const struct ctx *c, int s, flow_sidx_t sidx)
>  	      str_ee_origin(&eh->ee), s, strerror_(eh->ee.ee_errno));
>  
>  	if (!flow_sidx_valid(sidx)) {
> -		trace("Ignoring received IP_RECVERR cmsg on listener socket");
> -		return 1;
> +		/* No hint from the socket, determine flow from addresses */
> +		union inany_addr dst;
> +
> +		if (udp_pktinfo(&mh, &dst) < 0) {
> +			warn("Missing PKTINFO on UDP error");

...changed this to debug(), see my comments to 5/7.

-- 
Stefano