Re: [PATCH v2 01/16] udp: Also bind() connected ports for "splice" forwarding

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

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

On Fri, Nov 25, 2022 at 02:47:51AM +0100, Stefano Brivio wrote:
> On Thu, 24 Nov 2022 12:16:44 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > pasta handles "spliced" port forwarding by resending datagrams received on
> > a bound socket in the init namespace to a connected socket in the guest
> > namespace.  This means there are actually three ports associated with each
> > "connection".  First there's the source and destination ports of the
> > originating datagram.  That's also the destination port of the forwarded
> > datagram, but the source port of the forwarded datagram is the kernel
> > allocated bound address of the connected socket.
> > 
> > However, by bind()ing as well as connect()ing the forwarding socket we can
> > choose the source port of the forwarded datagrams.  By choosing it to match
> > the original source port we remove that surprising third port number and
> > no longer need to store port numbers in struct udp_splice_port.
> 
> If you wondered, I think the whole connect() with getsockname() thing
> without a bind() came from the fundamental misconception I had that you
> couldn't connect() a bound socket -- and I didn't quite think of
> dropping connect() as you do in 3/16 anyway.
> 
> There's one minor problem this introduces: the source port of the
> originating datagram now needs to be free in the init namespace. It's
> still better than the alternative problem you fix in 16/16, though.

You mean in the target namespace?  Which could be init or otherwise
depending on direction.  That did occur to me, however, I believe the
only way it would be not free is if we have a fixed port mapping
occupying that port - in which case the problem goes away again in
8/16.

> I'm wondering if we could, _once you're done with all this_ (it already
> looks complicated enough), revisit the 'goto fail' in
> udp_splice_connect() (now udp_splice_new()) when bind() fails, and just
> proceed with an ephemeral port then.

Oof... I'd rather not, because then we'd have to have somewhere to
keep track of the original source port just for that unlikely edge
case.

> Also, I haven't tried, but I'm not sure if this introduces some kind of
> DoS possibility: even if pasta forwards a single port, it should be
> possible for a remote host to make pasta bind to a large amount of
> non-ephemeral ports.

Hm, yes, that's a point.

> Maybe it would make sense to think of a limit on how many ports a
> single peer could cause pasta to bind.

Maybe, yes.

> I'm not sure yet how we could track peers without a separate address
> storage (even though keeping an LRU array should be feasible) -- the
> simpler alternative, limiting bound ports by destination port, would
> offer an even more convenient way to a DoS.
> 
> On the other hand, this is exceedingly minor I guess. We're binding
> ports in the namespace after all, and we can reuse bound sockets.

Right.  I think this is something to address when and if it proves to
be a problem in practice.

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