On Fri, Jan 19, 2024 at 11:45:05AM +0100, Stefano Brivio wrote:On Fri, 19 Jan 2024 11:05:02 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Huh. I never realised you could use 0.0.0.0 like that. I guess that makes it work, though I still feel like explicitly using localhost would be clearer.On Thu, Jan 18, 2024 at 05:23:26PM +0100, Stefano Brivio wrote:Hah, "cromulent" just embiggened my dictionary! Why not, though? From RFC 6890, 2.2.2: +----------------------+----------------------------+ | Attribute | Value | +----------------------+----------------------------+ | Address Block | 0.0.0.0/8 | | Name | "This host on this network"|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: > > [...] > > + > + 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.But we've only bound ourselves to 0.0.0.0, which while perfectly cromulent for a listening socket, is no good for connect().> + 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 [...]and: $ strace -e connect ./pkt_selfie --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=891325, si_uid=1000, si_status=0, si_utime=0, si_stime=0} --- connect(5, {sa_family=AF_INET, sin_port=htons(51155), sin_addr=inet_addr("0.0.0.0")}, 16) = -1 EINPROGRESS (Operation now in progress) MSG_PEEK with offset not supported +++ exited with 0 +++ with pkt_selfie.c from review of v1: https://archives.passt.top/passt-dev/20231206160808.3d312733@elisabeth/-- 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