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(a)gibson.dropbear.id.au> wrote:Good point. Note that at present we're not bind()ing to an address either.On Sun, Jan 14, 2024 at 01:07:55PM -0500, Jon Maloy wrote: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.[...] + + 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.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).Hmm, why? From getsockname(2): getsockname() returns the current address to which the socket sockfd is bound [...]+ 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.-- 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/~dgibsonRight, 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[...] +/** 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().> [...]