On Tue, Apr 15, 2025 at 08:54:17PM +0200, Stefano Brivio wrote:
> 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.

Actually this one can go away entirely.  As you pointed out it's
redundant with the one in udp_pktinfo() itself.  Oh well, the extra
debug() should be harmless enough.

-- 
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