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 617915A005E for ; Sat, 19 Nov 2022 09:39:48 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1668847187; 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=M0//m3qWJtnWriufK81fNuO5iPtiTvCQHuZd8j/k/v4=; b=IT+L3q+SrVQhW1Kqs7xSrBBzH51N7v8+X2njuA751S5BnQDFND9BF3197jtEvvNE1GYlMd tJAsXv0KkziSS9uEVro2V6HarFJGfh/mWePyCqc3hcd2avTeZ5o+82rHtUfjSkeD9a9PDv sXPfAeX5xF30Drvr+EMYEDggdQY1or0= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-471-htEEzbqHMS-tHWFqMloq4Q-1; Sat, 19 Nov 2022 03:39:41 -0500 X-MC-Unique: htEEzbqHMS-tHWFqMloq4Q-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 7BA878027EC; Sat, 19 Nov 2022 08:39:41 +0000 (UTC) Received: from maya.cloud.tilaa.com (ovpn-208-8.brq.redhat.com [10.40.208.8]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 244F240C6EC3; Sat, 19 Nov 2022 08:39:40 +0000 (UTC) Date: Sat, 19 Nov 2022 09:39:38 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v2 08/32] tcp: Add connection union type Message-ID: <20221119093938.4b5b4b73@elisabeth> In-Reply-To: References: <20221117055908.2782981-1-david@gibson.dropbear.id.au> <20221117055908.2782981-9-david@gibson.dropbear.id.au> <20221118012518.49a46b94@elisabeth> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.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: AQIKTLO4VRPPJ6WLK5AZ3OZCSVLELMY6 X-Message-ID-Hash: AQIKTLO4VRPPJ6WLK5AZ3OZCSVLELMY6 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 X-Mailman-Version: 3.3.3 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 Fri, 18 Nov 2022 12:10:47 +1100 David Gibson wrote: > On Fri, Nov 18, 2022 at 01:25:18AM +0100, Stefano Brivio wrote: > > > > [...] > > > > It also confuses Coverity Scan, because in tcp_table_compact() we have: > > > > memset(hole, 0, sizeof(*hole)); > > > > and while the prototype is: > > > > void tcp_table_compact(struct ctx *c, union tcp_conn *hole) > > > > it sees that we're passing, from tcp_splice_destroy(), something > > smaller than that (48 bytes), but we're zeroing the whole thing. > > Bother. > > > Of course, it's not a real issue, that space is reserved for a > > connection slot anyway, but given there are no other issues reported, > > I'd try to keep Coverity happy if possible. > > > > First try, failed: check hole->c.spliced and, if set, zero only > > sizeof(struct tcp_splice_conn) bytes. This looks like a false > > positive. > > > > Another try, which should probably work (I just hit the daily build > > submission quota, grr): explicitly pass the union tcp_conn containing > > our struct tcp_splice_conn. This patch does it: > > > > --- > > diff --git a/tcp.c b/tcp.c > > index 8874789..d635a8e 100644 > > --- a/tcp.c > > +++ b/tcp.c > > @@ -591,7 +591,7 @@ static size_t tcp6_l2_flags_buf_bytes; > > union tcp_conn tc[TCP_MAX_CONNS]; > > > > #define CONN(index) (&tc[(index)].tap) > > -#define CONN_IDX(conn) ((union tcp_conn *)(conn) - tc) > > +#define CONN_IDX(conn) (TCP_TAP_TO_COMMON(conn) - tc) > > > > /** conn_at_idx() - Find a connection by index, if present > > * @index: Index of connection to lookup > > @@ -1385,7 +1385,7 @@ static void tcp_conn_destroy(struct ctx *c, struct tcp_tap_conn *conn) > > close(conn->timer); > > > > tcp_hash_remove(c, conn); > > - tcp_table_compact(c, (union tcp_conn *)conn); > > + tcp_table_compact(c, TCP_TAP_TO_COMMON(conn)); > > } > > > > static void tcp_rst_do(struct ctx *c, struct tcp_tap_conn *conn); > > diff --git a/tcp_conn.h b/tcp_conn.h > > index 4a8be29..fa407ad 100644 > > --- a/tcp_conn.h > > +++ b/tcp_conn.h > > @@ -176,6 +176,12 @@ union tcp_conn { > > struct tcp_splice_conn splice; > > }; > > > > +#define TCP_TAP_TO_COMMON(x) \ > > + ((union tcp_conn *)((char *)(x) - offsetof(union tcp_conn, tap))) > > + > > +#define TCP_SPLICE_TO_COMMON(x) \ > > + ((union tcp_conn *)((char *)(x) - offsetof(union tcp_conn, splice))) > > + > > /* TCP connections */ > > extern union tcp_conn tc[]; > > > > diff --git a/tcp_splice.c b/tcp_splice.c > > index e2f0ce1..7d3f17e 100644 > > --- a/tcp_splice.c > > +++ b/tcp_splice.c > > @@ -37,6 +37,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -74,7 +75,7 @@ static int splice_pipe_pool [TCP_SPLICE_PIPE_POOL_SIZE][2][2]; > > #define CONN_V4(x) (!CONN_V6(x)) > > #define CONN_HAS(conn, set) ((conn->events & (set)) == (set)) > > #define CONN(index) (&tc[(index)].splice) > > -#define CONN_IDX(conn) ((union tcp_conn *)(conn) - tc) > > +#define CONN_IDX(conn) (TCP_SPLICE_TO_COMMON(conn) - tc) > > > > /* Display strings for connection events */ > > static const char *tcp_splice_event_str[] __attribute((__unused__)) = { > > @@ -283,7 +284,7 @@ void tcp_splice_destroy(struct ctx *c, struct tcp_splice_conn *conn) > > debug("TCP (spliced): index %li, CLOSED", CONN_IDX(conn)); > > > > c->tcp.splice_conn_count--; > > - tcp_table_compact(c, (union tcp_conn *)conn); > > + tcp_table_compact(c, TCP_SPLICE_TO_COMMON(conn)); > > } > > Hmm.. so TAP_TO_COMMON and SPLICE_TO_COMMON don't actually change the > pointer any more than the cast does, but you're hoping that will be > enough to convince coverity? I'm a bit skeptical, but I guess we can > try it. Right, that didn't actually work. > I wonder if an explicit coverity lint override might be a better > option here. Or, we could add padding to tcp_splice_conn so it has > the same size as tcp_tap_conn. I think that would probably convince > coverity. Padding needs to be calculated manually, and a specific override might need some specific maintenance, with references to proprietary code. I'm not really fond of either. Given that we already have the pointers to the container union in the callers, we could just pass those instead, and then in splice or "tap" specific logic, and just there, use struct tcp_{tap,splice}_conn. I just posted a patch doing this. -- Stefano