Re: [PATCH 7/8] udp: Decide whether to "splice" per datagram rather than per socket

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3466 bytes --]

On Tue, Dec 13, 2022 at 11:49:18PM +0100, Stefano Brivio wrote:
> On Mon,  5 Dec 2022 19:14:24 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Currently we have special sockets for receiving datagrams from locahost
> > which can use the optimized "splice" path rather than going across the tap
> > interface.
> > 
> > We want to loosen this so that sockets can receive sockets that will be
> > forwarded by both the spliced and non-spliced paths.  To do this, we alter
> > the meaning of the @splice bit in the reference to mean that packets
> > receieved on this socket *can* be spliced, not that they *will* be spliced.
> > They'll only actually be spliced if they come from 127.0.0.1 or ::1.
> > 
> > We can't (for now) remove the splice bit entirely, unlike with TCP.  Our
> > gateway mapping means that if the ns initiates communication to the gw
> > address, we'll translate that to target 127.0.0.1 on the host side.  Reply
> > packets will therefore have source address 127.0.0.1 when received on the
> > host, but these need to go via the tap path where that will be translated
> > back to the gateway address.  We need the @splice bit to distinguish that
> > case from packets going from localhost to a port mapped explicitly with
> > -u which should be spliced.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  udp.c | 54 +++++++++++++++++++++++++++++++++++-------------------
> >  udp.h |  2 +-
> >  2 files changed, 36 insertions(+), 20 deletions(-)
> > 
> > diff --git a/udp.c b/udp.c
> > index 6ccfe8c..011a157 100644
> > --- a/udp.c
> > +++ b/udp.c
> > @@ -513,16 +513,27 @@ static int udp_splice_new_ns(void *arg)
> >  }
> >  
> >  /**
> > - * sa_port() - Determine port from a sockaddr_in or sockaddr_in6
> > + * udp_mmh_splice_port() - Is source address of message suitable for splicing?
> >   * @v6:		Is @sa a sockaddr_in6 (otherwise sockaddr_in)?
> > - * @sa:		Pointer to either sockaddr_in or sockaddr_in6
> > + * @mmh:	mmsghdr of incoming message
> > + *
> > + * Return: if @sa refers to localhost (127.0.0.1 or ::1) the port from
> > + *         @sa, otherwise 0.
> > + *
> > + * NOTE: this relies on the fact that it's not valid to use UDP port 0
> 
> The port is reserved by IANA indeed, but... it can actually be used. On
> Linux, you can bind() it and you can connect() to it. As far as I can
> tell from the new version of udp_sock_handler() we would actually
> misdirect packets in that case.

Hm, ok.  Given the IANA reservation, I think it would be acceptable to
simply drop such packets - but if we were to make that choice we
should do so explicitly, rather than misdirecting them.

> How bad would it be to use an int here?

Pretty straightforward.  Just means we have to use the somewhat
abtruse "if (port <= USHRT_MAX)" or "if (port >= 0)" or something
instead of just "if (port)".  Should I go ahead and make that change?

> By the way, I think the comment should also mention that the port is
> returned in host order.

Ok, easily done.  Generally I try to keep the endianness associated
with the type, rather than attempting to document it for each variable
(or even worse, each point in time for each variable).

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]