Re: [PATCH 2/2] conf, icmp, tcp, udp: Add options to bind to outbound address and interface

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 9 Mar 2023 11:18:42 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Mar 09, 2023 at 12:33:20AM +0100, Stefano Brivio wrote:
> > On Thu, 9 Mar 2023 09:02:57 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Wed, Mar 08, 2023 at 08:34:49AM +0100, Stefano Brivio wrote:  
> > > > I didn't notice earlier: libslirp (and slirp4netns) supports binding
> > > > outbound sockets to specific IPv4 and IPv6 addresses, to force the
> > > > source addresse selection. If we want to claim feature parity, we
> > > > should implement that as well.
> > > > 
> > > > Further, Podman supports specifying outbound interfaces as well, but
> > > > this is simply done by resolving the primary address for an interface
> > > > when the network back-end is started. However, since kernel version
> > > > 5.7, commit c427bfec18f2 ("net: core: enable SO_BINDTODEVICE for
> > > > non-root users"), we can actually bind to a specific interface name,
> > > > which doesn't need to be validated in advance.
> > > > 
> > > > Implement -o / --outbound ADDR to bind to IPv4 and IPv6 addresses,
> > > > and --outbound-ip4 and --outbound-ip6 to bind IPv4 and IPv6 sockets
> > > > to given interfaces.    
> > > 
> > > You have 'outbound-ip' here but 'outbound-if' in the code, I think you
> > > intended the latter.  
> > 
> > Oops, right.
> >   
> > > > For ICMP and UDP, we call sock_l4() to open outbound sockets, as we
> > > > already needed to bind to given ports or echo identifiers, and we
> > > > can bind() a socket only once: there, pass address (if any) and
> > > > interface (if any) for the existing bind() and setsockopt() calls.
> > > > 
> > > > For TCP, in general, we wouldn't otherwise bind sockets. Add a
> > > > specific helper to do that.
> > > > 
> > > > For UDP outbound sockets, we need to know if the final destination
> > > > of the socket is a loopback address, before we decide whether it
> > > > makes sense to bind the socket at all: move the block mangling the
> > > > address destination before the creation of the socket in the IPv4
> > > > path. This was already the case for the IPv6 path.
> > > > 
> > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > > ---
> > > >  conf.c  | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
> > > >  icmp.c  | 24 +++++++++++++++---
> > > >  passt.1 | 19 ++++++++++++++
> > > >  passt.h | 10 ++++++++
> > > >  tcp.c   | 60 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  udp.c   | 54 ++++++++++++++++++++++++++-------------
> > > >  6 files changed, 219 insertions(+), 26 deletions(-)
> > > > 
> > > > diff --git a/conf.c b/conf.c
> > > > index 3aa3314..15506ec 100644
> > > > --- a/conf.c
> > > > +++ b/conf.c
> > > > @@ -776,6 +776,13 @@ static void usage(const char *name)
> > > >  	info(   "    default: gateway from interface with default route");
> > > >  	info(   "  -i, --interface NAME	Interface for addresses and routes");
> > > >  	info(   "    default: interface with first default route");    
> > > 
> > > So, I think the outbound IP and the IP we advertise to the guest
> > > should be the same.  Certainly by default, and I'm not sure I can even
> > > think of any case where it would be useful for them to be different.
> > > That fits with the "only NAT when we really have to" goal.  
> > 
> > Sure, that's a goal, but users might want to do NAT for whatever
> > reason, even just for slirp4netns compatibility (which we should
> > *really* support to play along nicely with Podman).
> > 
> > At the moment, it's already enough to pass '-a 10.200.0.2' and the
> > outbound address will be (in general) different from what we advertise.  
> 
> Yes, I know, but I'm saying I think that's kind of undesirable.  The
> more prominent option should change both outbound and advertised
> address to match.  Separately overriding the advertised address should
> be the special case.

So, summing up from offline discussion: ideally, yes, but we don't
really want to break "-a <private address>" at this point. This covers
a few point below, and I'll answer/summarise the rest later, except for
this:

> Normal mode:
> 	-a sets both the outbound and guest address
> 	--if4 and --if6 set the outbound interface.  If there's no -a,
> 	both outbound and guest address are derived from this as well.

...which is pretty much done in v4 -- the rest can most likely be
addressed later on without this series being a substantial obstacle.

-- 
Stefano