Re: [PATCH v2 08/32] tcp: Add connection union type

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 18 Nov 2022 12:10:47 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <limits.h>
> >  #include <stdint.h>
> >  #include <stdbool.h>
> > +#include <stddef.h>
> >  #include <string.h>
> >  #include <time.h>
> >  #include <unistd.h>
> > @@ -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