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):
> 
> On Thu, 18 Jan 2024 14:05:38 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Sun, Jan 14, 2024 at 01:07:55PM -0500, Jon Maloy wrote:
> > >
> > > [...]
> > >
> > > +
> > > +	s[0] = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> > > +	s[1] = 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))) {  
> > 
> > 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.
> 
> 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.
> 
> 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 <= connect(s[1], &a, sizeof(a)) || errno != EINPROGRESS) {
> > > +		perror("Temporary probe socket connect() failed\n");
> > > +		goto out;
> > > +	}  
> > 
> > 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.
> 
> Hmm, why? From getsockname(2):
> 
>        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()  
> > 
> > I believe we prefer the explicit foo(void) for declarations of
> > functions with no parameters, rather than just foo().
> 
> 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).
> 
> Look:
> 
> $ cat prototypes.c
> int a(void) { ; }
> int b() { ; }
> int main(char **argv) { a(); a(1); b(); b(1); }
> 
> $ gcc prototypes.c
> prototypes.c: In function ‘main’:
> prototypes.c:3:30: error: too many arguments to function ‘a’
>     3 | int main(char **argv) { a(); a(1); b(); b(1); }
>       |                              ^
> prototypes.c:1:5: note: declared here
>     1 | int a(void) { ; }
>       |     ^
> 
> note that calling b() with any number and type of arguments is fine. And:
> 
> $ gcc -Wstrict-prototypes -Werror -Wfatal-errors prototypes.c
> prototypes.c:2:5: error: function declaration isn’t a prototype [-Werror=strict-prototypes]
>     2 | int b() { ; }
>       |     ^
> compilation terminated due to -Wfatal-errors.
> cc1: all warnings being treated as errors
> 
> > > [...]
> 

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