Re: [PATCH 2/2] udp: Remove socket from udp_{tap,splice}_map when timed out

[Stefano Brivio <sbrivio@redhat.com>]

On Mon,  6 Nov 2023 13:17:09 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> We save sockets bound to particular ports in udp_{tap,splice}_map for
> reuse later.  If they're not used for a time, we time them out and close
> them. However, when that happened, we weren't actually removing the fds
> from the relevant map.  That meant that later interactions on the same port
> could get a stale fd from the map.
> 
> The stale fd might be closed, leading to unexpected EBADF errors, or it
> could have been re-used by a completely different socket bound to a
> different port, which could lead to us incorrectly forwarding packets.
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=57

Adding:

Reported-by: Reported-by: Chris Kuhn <kuhnchris@kuhnchris.eu>
Reported-by: Jay <bugs.passt.top@bitsbetwixt.com>

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  udp.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/udp.c b/udp.c
> index a8473e3..b40d1f3 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -1146,14 +1146,14 @@ static void udp_timer_one(struct ctx *c, int v6, enum udp_act_type type,
>  {
>  	struct udp_splice_port *sp;
>  	struct udp_tap_port *tp;
> -	int s = -1;
> +	int *sockp = NULL;
>  
>  	switch (type) {
>  	case UDP_ACT_TAP:
>  		tp = &udp_tap_map[v6 ? V6 : V4][port];
>  
>  		if (ts->tv_sec - tp->ts > UDP_CONN_TIMEOUT) {
> -			s = tp->sock;
> +			sockp = &tp->sock;
>  			tp->flags = 0;
>  		}
>  
> @@ -1162,21 +1162,23 @@ static void udp_timer_one(struct ctx *c, int v6, enum udp_act_type type,
>  		sp = &udp_splice_init[v6 ? V6 : V4][port];
>  
>  		if (ts->tv_sec - sp->ts > UDP_CONN_TIMEOUT)
> -			s = sp->sock;
> +			sockp = &sp->sock;
>  
>  		break;
>  	case UDP_ACT_SPLICE_NS:
>  		sp = &udp_splice_ns[v6 ? V6 : V4][port];
>  
>  		if (ts->tv_sec - sp->ts > UDP_CONN_TIMEOUT)
> -			s = sp->sock;
> +			sockp = &sp->sock;
>  
>  		break;
>  	default:
>  		return;
>  	}
>  
> -	if (s >= 0) {
> +	if (sockp && *sockp >= 0) {
> +		int s = *sockp;
> +		*sockp = -1;
>  		epoll_ctl(c->epollfd, EPOLL_CTL_DEL, s, NULL);
>  		close(s);
>  		bitmap_clear(udp_act[v6 ? V6 : V4][type], port);

-- 
Stefano