From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=YVomm2Vy; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id ABC0E5A061E for ; Thu, 13 Nov 2025 07:33:11 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1763015590; 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=MNTUgnKJgXrcpt7iYJot3NyqYWaL80fwoUGceTeI+DE=; b=YVomm2VyCcNP7dyzKzXliND0ggDJva5YTldfO5gzOWYkLFyGNkhGybhOL2mKXnmlIKeqs6 sWElqh8YPDd2OF0/KLai0mGFvYbxNSxTATCrSkZioNSNCg+2tllotbdjGnrazKd8GG3pIf hfaSJTryPzaw35vRu43T8BOXSK+bahc= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-500-gQHvDHsANgKpsJntJ_nqtQ-1; Thu, 13 Nov 2025 01:33:09 -0500 X-MC-Unique: gQHvDHsANgKpsJntJ_nqtQ-1 X-Mimecast-MFC-AGG-ID: gQHvDHsANgKpsJntJ_nqtQ_1763015588 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-47106720618so8390605e9.1 for ; Wed, 12 Nov 2025 22:33:09 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1763015587; x=1763620387; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=MNTUgnKJgXrcpt7iYJot3NyqYWaL80fwoUGceTeI+DE=; b=vaFmYVWeYEls9W7feKQ7FrjcH/EhaMyz4dpcvbt7Pj0L9RT7TGYeMP28jC8dzg045J WKySeo1LSyr3q+o5BsZJ5oSlbYCV+a/bwiem4YymeG+mE0VTTht+m9DhGfj7gISidXxZ nu6HqEGPxeAopZH6cRsjaaoo71be3UPwLuPl/jnYtwJIsVHySX0TTXoNjeAkiDVLQ6RH cVgRLMdEMpmZMKe5q0CI1vStb0zBXriUc7ArH3/5e+86vYGuQ2oKNftNVnyJfWcjyLjJ VHoaGTFu7raRIrJ1vvk4gz8isSeDH/3F6Zom6/TaLr/m1e4kvQN7DvwW8TVZyB2E+25T mHew== X-Gm-Message-State: AOJu0YxESUsWlw2ZTVrYwg3cHvc+aynRiMNYTeBWaWXKdSczXrejFOQ5 tkWJyG8QpNeKOACu7AesjJIvih8+XGtliSNRwMPqmCw8pigye7VVURlFFWyEI/71g2OtvctCfk1 H6YrN1r1JlgF5cmmMyUF3IR1wgfBJYSA84++usiaH+VSlgsQLrRWxWJ32LqQrpQ== X-Gm-Gg: ASbGncu41Lp9ZcRNX44XnK6jp+0Affv1bnYiQzFt+xud/hLg1kO2+SSzIkEb5MUqNB3 kTZTQaHbbjTUBj4K3zkZy6NLz4JlInvsIPs8HEaBxojNan0x951eiKdA7K18PEYjUDWfpxuu3rx GiLbFCwN3i49XyxnwvMVDKwep6uIPWp6AEJ+Vz2bz/ngy0T46NXfR+fGWpUtYR43xx0A2hLsiwC UzF8IfmWE1eV7/siT7fkvA+ecb2EiRViigfWULSEKGDNqo1EFguKh9ION5x4zQvpkZvkjG20liB 9m9kCYAW1FRfps0T0gSu0PHLLJ9U0OYFV5KVDMSJeJnwsRbh7u0pp+DnmEal35drTSaBprN0oVJ OLkCTPCWILtO9cAUBVKvuJ6Z0MwM= X-Received: by 2002:a05:600c:3546:b0:46e:32a5:bd8d with SMTP id 5b1f17b1804b1-4778703e738mr59141835e9.3.1763015587114; Wed, 12 Nov 2025 22:33:07 -0800 (PST) X-Google-Smtp-Source: AGHT+IFL7Mdr12vbNLpMaeMjKHUTWVgwbgPzEqjbbkFu49QR4T6puKnVWFy/UOEeNeDhF8+6pqZBKg== X-Received: by 2002:a05:600c:3546:b0:46e:32a5:bd8d with SMTP id 5b1f17b1804b1-4778703e738mr59141285e9.3.1763015586196; Wed, 12 Nov 2025 22:33:06 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-47787e8e6acsm74585545e9.9.2025.11.12.22.33.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 12 Nov 2025 22:33:05 -0800 (PST) Date: Thu, 13 Nov 2025 07:33:04 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v3 1/8] inany: Let length of sockaddr_inany be implicit from the family Message-ID: <20251113073304.56859bae@elisabeth> In-Reply-To: <20251029062628.1647051-2-david@gibson.dropbear.id.au> References: <20251029062628.1647051-1-david@gibson.dropbear.id.au> <20251029062628.1647051-2-david@gibson.dropbear.id.au> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: w82soo9iHBAKsqjh6rDr0DKRaupoL2t-sHA79lsQ9Ss_1763015588 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: A3J6EVRY6C75R3JXFN72GICSFD26ZTO7 X-Message-ID-Hash: A3J6EVRY6C75R3JXFN72GICSFD26ZTO7 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.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: Apologies for the very substantial delay. I have to admit I procrastinated around 3/8 and 4/8 quite a bit as they look scary (but much needed, of course). Nits only, here: On Wed, 29 Oct 2025 17:26:21 +1100 David Gibson wrote: > sockaddr_inany can contain either an IPv4 or IPv6 socket address, so the > relevant length for bind() or connect() can vary. In pif_sockaddr() we > return that length, and in sock_l4_sa() we take it as an extra parameter. > > However, sockaddr_inany always contains exactly a sockaddr_in or a > sockaddr_in6 each with a fixed size. Therefore we can derive the relevant > length from the family, and don't need to pass it around separately. > > Make a tiny helper to get the relevant address length, and update all > interfaces to use that approach instead. > > In the process, fix a buglet in tcp_flow_repair_bind(): we passed > sizeof(union sockaddr_inany) to bind() instead of the specific length for > the address family. Since the sizeof() is always longer than the specific > length, this is probably fine, but not theoretically correct. (Whoops.) > Signed-off-by: David Gibson > --- > flow.c | 17 +++++++---------- > icmp.c | 3 ++- > inany.h | 18 ++++++++++++++++++ > pif.c | 14 ++++---------- > pif.h | 2 +- > tcp.c | 20 +++++++++----------- > tcp_splice.c | 5 ++--- > udp.c | 8 +++----- > util.c | 9 ++++----- > util.h | 5 +++-- > 10 files changed, 53 insertions(+), 48 deletions(-) > > diff --git a/flow.c b/flow.c > index feefda3c..9926f408 100644 > --- a/flow.c > +++ b/flow.c > @@ -170,8 +170,7 @@ struct flowside_sock_args { > int fd; > int err; > enum epoll_type type; > - const struct sockaddr *sa; > - socklen_t sl; This should be dropped from the struct documentation. > + const union sockaddr_inany *sa; > const char *path; > uint32_t data; Note: you'll get a trivial conflict here with "util: Move epoll registration out of sock_l4_sa()" if you rebase. > }; > @@ -187,7 +186,7 @@ static int flowside_sock_splice(void *arg) > > ns_enter(a->c); > > - a->fd = sock_l4_sa(a->c, a->type, a->sa, a->sl, NULL, > + a->fd = sock_l4_sa(a->c, a->type, a->sa, NULL, > a->sa->sa_family == AF_INET6, a->data); > a->err = errno; > > @@ -209,11 +208,10 @@ int flowside_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif, > { > const char *ifname = NULL; > union sockaddr_inany sa; > - socklen_t sl; > > ASSERT(pif_is_socket(pif)); > > - pif_sockaddr(c, &sa, &sl, pif, &tgt->oaddr, tgt->oport); > + pif_sockaddr(c, &sa, pif, &tgt->oaddr, tgt->oport); > > switch (pif) { > case PIF_HOST: > @@ -224,13 +222,13 @@ int flowside_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif, > else if (sa.sa_family == AF_INET6) > ifname = c->ip6.ifname_out; > > - return sock_l4_sa(c, type, &sa, sl, ifname, > + return sock_l4_sa(c, type, &sa, ifname, > sa.sa_family == AF_INET6, data); > > case PIF_SPLICE: { > struct flowside_sock_args args = { > .c = c, .type = type, > - .sa = &sa.sa, .sl = sl, .data = data, > + .sa = &sa, .data = data, > }; > NS_CALL(flowside_sock_splice, &args); > errno = args.err; > @@ -259,10 +257,9 @@ int flowside_connect(const struct ctx *c, int s, > uint8_t pif, const struct flowside *tgt) > { > union sockaddr_inany sa; > - socklen_t sl; > > - pif_sockaddr(c, &sa, &sl, pif, &tgt->eaddr, tgt->eport); > - return connect(s, &sa.sa, sl); > + pif_sockaddr(c, &sa, pif, &tgt->eaddr, tgt->eport); > + return connect(s, &sa.sa, socklen_inany(&sa)); > } > > /** flow_log_ - Log flow-related message > diff --git a/icmp.c b/icmp.c > index 6dffafb0..62b3bed8 100644 > --- a/icmp.c > +++ b/icmp.c > @@ -301,8 +301,9 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af, > ASSERT(flow_proto[pingf->f.type] == proto); > pingf->ts = now->tv_sec; > > - pif_sockaddr(c, &sa, &msh.msg_namelen, PIF_HOST, &tgt->eaddr, 0); > + pif_sockaddr(c, &sa, PIF_HOST, &tgt->eaddr, 0); > msh.msg_name = &sa; > + msh.msg_namelen = socklen_inany(&sa); > msh.msg_iov = iov; > msh.msg_iovlen = cnt; > msh.msg_control = NULL; > diff --git a/inany.h b/inany.h > index 7ca5cbd3..e3cae2a8 100644 > --- a/inany.h > +++ b/inany.h > @@ -67,6 +67,24 @@ union sockaddr_inany { > struct sockaddr_in6 sa6; > }; > > +/** socklen_inany() - Return relevant size of an sockaddr_inany 'of a' ...but the fact that it returns that sounds kind of pleonastic. What about: /** socklen_inany() - Get relevant address length for sockaddr_inany address > + * @sa: sockaddr_iany socket address sockaddr_inany > + * > + * Returns the correct socket address length to pass to bind() or connect() with > + * @sa, based on whether it is an IPv4 or IPv6 address. Same here, I guess we obviously want it to be correct, and we have a somewhat standard syntax for return values in documentation, what about: * Return: socket address length for bind() or connect(), from IP family in @sa ? > + */ > +static inline socklen_t socklen_inany(const union sockaddr_inany *sa) > +{ > + switch (sa->sa_family) { > + case AF_INET: > + return sizeof(sa->sa4); Wouldn't it be more obvious to return sizeof(struct sockaddr_in), and sizeof(struct sockaddr_in6) below, instead? I actually had to check that sa->sa4 matched that (I didn't remember if that was a union). Not a strong preference from my side, both are obviously correct anyway. > + case AF_INET6: > + return sizeof(sa->sa6); > + default: > + ASSERT(0); > + } > +} -- Stefano