From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 9E6965A026D for ; Fri, 19 Jan 2024 01:26:37 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1705623993; bh=1P3ibgggPMw3+JrJWOygYSzzAxFS8lFiZd8QPDrZntA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=TfnFGG1gAPqpSecTs6XdjtcbVv2sqa6efWVtIyw8a3gS59nWO34UV2KdQOjDE8K7g lPAfw99K4x5gDtXHWidcOBDjoeZ3/m2V3iiDyrO7bq0oyQ1+1J0UMMsTJVAmWaHpDa BefY7kZk+a6SIykP9SLYUd6NOFdsdasHeFSy5kn7l4QtdLxxErSzGVDJgofbyyGYRk lSK6E6l6PKqma6mtoL2iunPyjUm4a7F5j2PHXhUpsYwOle5u3Kd+PhvWQwiRCsHMWH 5mt8pXRaY02r7qBXTQvicsiDVYZEsOSrubvHdu1dZEmlyIGgaZn754InZ6TTClavFQ izn1yDDnOfVGQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4TGL5P6L1Tz4xPY; Fri, 19 Jan 2024 11:26:33 +1100 (AEDT) Date: Fri, 19 Jan 2024 11:05:02 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v2] tcp.c: leverage MSG_PEEK with offset kernel capability when available Message-ID: References: <20240114180755.1008481-1-jmaloy@redhat.com> <20240118172326.73b6f4ba@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="EYoa8EzJijikW+Y5" Content-Disposition: inline In-Reply-To: <20240118172326.73b6f4ba@elisabeth> Message-ID-Hash: GICJFQVGNUFS5JLSIVXR4WKHNPRBTGIQ X-Message-ID-Hash: GICJFQVGNUFS5JLSIVXR4WKHNPRBTGIQ X-MailFrom: dgibson@gandalf.ozlabs.org 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: Jon Maloy , passt-dev@passt.top, lvivier@redhat.com, dgibson@redhat.com 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: --EYoa8EzJijikW+Y5 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jan 18, 2024 at 05:23:26PM +0100, Stefano Brivio wrote: > Not a full review, but a couple of comments, mostly about stuff I also > had in pkt_selfie.c (review of v1): >=20 > On Thu, 18 Jan 2024 14:05:38 +1100 > David Gibson wrote: >=20 > > On Sun, Jan 14, 2024 at 01:07:55PM -0500, Jon Maloy wrote: > > > > > > [...] > > > > > > + > > > + s[0] =3D socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); > > > + s[1] =3D socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, IPPROTO_TCP); > > > + if (s[0] < 0 || s[1] < 0) { > > > + perror("Temporary probe socket creation failed\n"); > > > + goto out; > > > + } > > > + if (0 > bind(s[0], &a, sizeof(a))) { =20 > >=20 > > Since the socket address is unspecified, why do you need to bind at > > all? It might be clearer to explicitly set a to localhost + a > > specific port - because you're in a temporary namespace, you can rely > > on every port being available. >=20 > There are two advantages of bind() without port, and then getsockname(): > first, ip_unprivileged_port_start might have whatever value in our new > namespace (we don't touch it), and I wouldn't take for granted we'll > have CAP_SYS_ADMIN in it for all the possible start-up combinations. >=20 > Second, there's no need for a magic value. Good point. Note that at present we're not bind()ing to an address either. > > > + perror("Temporary probe socket bind() failed\n"); > > > + goto out; > > > + } > > > + if (0 > getsockname(s[0], &a, &((socklen_t) { sizeof(a) }))) { > > > + perror("Temporary probe socket getsockname() failed\n"); > > > + goto out; > > > + } > > > + if (0 > listen(s[0], 0)) { > > > + perror("Temporary probe socket listen() failed\n"); > > > + goto out; > > > + } > > > + if (0 <=3D connect(s[1], &a, sizeof(a)) || errno !=3D EINPROGRESS) { > > > + perror("Temporary probe socket connect() failed\n"); > > > + goto out; > > > + } =20 > >=20 > > This is assuming that a will now contain the correct address to > > connect to. Although it will have the right port, I think the address > > may still be unspecified for the listening socket. >=20 > Hmm, why? From getsockname(2): >=20 > getsockname() returns the current address to which the socket > sockfd is bound [...] But we've only bound ourselves to 0.0.0.0, which while perfectly cromulent for a listening socket, is no good for connect(). If we accept(), then the accepted socket will have a specific bound address, but the listening socket won't (I'm pretty sure I've actually tested this). > > > [...] > > > > > > +/** tcp_probe_msg_peek_offset_cap() - Probe kernel for MSG_PEEK with= offset support > > > + */ > > > +static bool tcp_probe_msg_peek_offset_cap() =20 > >=20 > > I believe we prefer the explicit foo(void) for declarations of > > functions with no parameters, rather than just foo(). >=20 > Right, because foo() isn't a prototype, while foo(void) is. Perhaps at > some point we should enable -Wstrict-prototypes in CFLAGS (it's not in > -Wextra, I just realised). >=20 > Look: >=20 > $ cat prototypes.c > int a(void) { ; } > int b() { ; } > int main(char **argv) { a(); a(1); b(); b(1); } >=20 > $ gcc prototypes.c > prototypes.c: In function =E2=80=98main=E2=80=99: > prototypes.c:3:30: error: too many arguments to function =E2=80=98a=E2=80= =99 > 3 | int main(char **argv) { a(); a(1); b(); b(1); } > | ^ > prototypes.c:1:5: note: declared here > 1 | int a(void) { ; } > | ^ >=20 > note that calling b() with any number and type of arguments is fine. And: >=20 > $ gcc -Wstrict-prototypes -Werror -Wfatal-errors prototypes.c > prototypes.c:2:5: error: function declaration isn=E2=80=99t a prototype [= -Werror=3Dstrict-prototypes] > 2 | int b() { ; } > | ^ > compilation terminated due to -Wfatal-errors. > cc1: all warnings being treated as errors >=20 > > > [...] >=20 --=20 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 --EYoa8EzJijikW+Y5 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmWpvK0ACgkQzQJF27ox 2Gfalw/+KkPo2kme5SAyCktfKMa/oKJChmmMpIgBDR2Hsj4YtN7CtlgGOtZgpsuw ELk7T4hA0ExGdkiGBwg2XfGRg5pBbd/sp8dXMN/k6x7xPcm/yF1+6eBO2B3PMClP pzBy69eX3G0OVuAYcHTtbyQfFcAUtCD7UHR6pbsf8Ggyx4J6JheFf1T8QbjTs7j/ 68Xi2q/rr4uYmHi+R12PxwUScfmQoM9FdVCsoVOkZpX0hPaYyXvkF64UkQOIRuoH E8MP1PL4qgc6eNkLciJW0gPtTPs9+5g9pkkzxo7Ffk/ntu0p5zKmEwSK8ZAu7ZoP ePPb6TqSuRn2EbrQjF7rp6NWO1NDee+oBjL6e4+mPdFVcNyGi+HeT/8ddwDOqECI F4MDYhSRqobiX1SS57zq92kzLs4CPdYszFW/pP7WgVsEkqAtgTXe0y4wecIQ1o3X RDyEOy5p/SCLsga7tCeEPE3hvklVCyLSCL3v/Fg5lR6NpaZ+fM1wP2Gb884fWtLd HZGAlqzMN2mgZGlaD6l5ueO3WPFnDh72/0wF3CG7d1eNK1CQxz/b79k6Rv0X9tbS iJp9D7XngmFLkFilE/u0Xantgsa7QjM/7VcZSbPcEAmn7I/1XhGU/FQVwbpPfww6 P1Q356GGASvVm/ORUTfYugD7tgP9nUd+FhJsPMFcPAJQBjgAlRQ= =7mxk -----END PGP SIGNATURE----- --EYoa8EzJijikW+Y5--