From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id DC7F75A026D for ; Tue, 7 Nov 2023 09:35:31 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1699346130; 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=qcVVzOm/0307iIYZPDAnf48gq/dN1l7n6OHVl5zCboQ=; b=KBiuMMsv/ubF/vRlqalzof/eTB2f/3QKlPv3wTSpfUsa8jRiuy5jeyNDW5OSktu1wQqWIO 00uRotaYY8qmhIYccbvPpvNgwaukd1lXaclb9GZJvzhIuyxOF2cHN4ua1Ka6kU9FtSrc1B ETVN/lOS5ml5+3rv1ejKj8LiqDDHP8Q= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-369-g2_yeOKDNCqUMuhp_tuQeA-1; Tue, 07 Nov 2023 03:35:27 -0500 X-MC-Unique: g2_yeOKDNCqUMuhp_tuQeA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 07AEA3C10148; Tue, 7 Nov 2023 08:35:27 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.12]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C162D40C6EBA; Tue, 7 Nov 2023 08:35:25 +0000 (UTC) Date: Tue, 7 Nov 2023 09:35:23 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 2/2] udp: Remove socket from udp_{tap,splice}_map when timed out Message-ID: <20231107093523.2210a930@elisabeth> In-Reply-To: <20231106021709.603571-3-david@gibson.dropbear.id.au> References: <20231106021709.603571-1-david@gibson.dropbear.id.au> <20231106021709.603571-3-david@gibson.dropbear.id.au> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: LRQI46Q3K5J3QFYOBF24HZDDKJIQ45U2 X-Message-ID-Hash: LRQI46Q3K5J3QFYOBF24HZDDKJIQ45U2 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, bugs.passt.top@bitsbetwixt.com, Chris Kuhn 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 Mon, 6 Nov 2023 13:17:09 +1100 David Gibson 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 Reported-by: Jay > Signed-off-by: David Gibson > --- > 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