Re: [PATCH v2] tcp.c: leverage MSG_PEEK with offset kernel capability when available

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 19 Jan 2024 11:05:02 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> 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().

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

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/

-- 
Stefano