Re: [PATCH 1/8] tcp: Fix address type for tcp_sock_init_af()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3138 bytes --]

On Wed, Dec 27, 2023 at 09:25:06PM +0100, Stefano Brivio wrote:
> On Fri,  8 Dec 2023 01:31:33 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > This takes a struct in_addr * (i.e. an IPv4 address), although it's
> > explicitly supposed to handle IPv6 as well.  Both its caller and sock_l4()
> > which it calls use a void * for the address, which can be either an in_addr
> > or an in6_addr.
> > 
> > We get away with this, because we don't do anything with the pointer other
> > than transfer it from the caller to sock_l4(), but it's misleading.  And
> > quite possibly technically UB, because C is like that.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  tcp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tcp.c b/tcp.c
> > index f506cfd..bda95b2 100644
> > --- a/tcp.c
> > +++ b/tcp.c
> > @@ -2905,7 +2905,7 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events)
> >   * Return: fd for the new listening socket, negative error code on failure
> >   */
> >  static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
> > -			    const struct in_addr *addr, const char *ifname)
> > +			    const void *addr, const char *ifname)
> 
> This is obviously correct.
> 
> However, after a lot of thinking: (gcc) optimisations based on
> Type-Based Alias Analysis, which we don't disable on this path, could,
> now, happily defer filling 'addr' with inet_pton() in conf_ports() to a
> point *after* the tcp_sock_init() call.

Hrm... possibly.  The fact that the addr variable in conf_ports() is a
char array, not a struct in*_addr might save us.  I think replacing it
with a union of an in_addr and in6_addr would also be ok.

> Without this patch, at least 32 bits must be updated before the call.

I'm not sure that's correct.  If the compiler is allowed to assume
that a char[] and a void * aren't aliased (even if they clearly are),
then I'd expect it to also be allowed to assume that a char[] and a
struct in_addr * aren't aliased.

> It might sound like a joke because... it actually is. But look at what
> we had to do for the functions in checksum.c. We pass const void *buf,
> and anything that buf points to can be updated (with TBAA) after the
> call.
> 
> I don't see any conceptual difference between this case and those
> functions.
> 
> Anyway, that won't reasonably happen here, and in any case this would
> have been broken for IPv6, so I'll go ahead and apply this.
> 
> But, eventually, I think we should switch all these usages to union
> inany_addr *.

So, we may be able to use union inany_addr in some places, but that's
not the same thing as this: inany_addr carries IPv4 addresses as
mapped IPv6 addresses, it's not switched on a separate af parameter.
We could, of course, define a new type as a simple union of in_addr
and in6_addr.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]