netstack: gonet.Conn.Read() is merging UDP messages

[majek]

In this code: https://github.com/majek/slirpnetstack/blob/master/routing.go#L32-L39

I'm trying to catch udp packets sent in the direction of netstack. I do this by:

• setting up ForwarderRequest
• On new packet creating CreateEndpoint
• then creating gonet.NewConn object

This object serves me well, and I can read/write packets to it. The problem is that it has no notion on whether the underlying flow is tcp or udp, and it assumes the former. This means that when there are two frames on the wire, send in rapid succession, like "foo" and "bar", the gonet.Conn.Read() will return "foobar".

This is not correct, with regard to semantics of udp.

Additionally, I wonder if it would make sense to provide a helper NewPacketConn, like here: https://github.com/majek/gvisor/commit/21bc6697b02c3aadf72ab2d073b9f0f879af8c99

The problem there is that we have a connected socket in my context, so stuff like RemoteAddr() actually make sense.

[hbhasker]

@iangudger could you take a look.

[majek]

Ok, couple of ways forward:

(1) Merge my NewPacketConn code: https://github.com/majek/gvisor/commit/21bc6697b02c3aadf72ab2d073b9f0f879af8c99 This will alleviate the problem, by allowing me to use the gonet.PacketConn for udp forwarding. To my understanding PacketConn.ReadFrom has correct semantics. Although this is not technically pretty - the underlying abstraction is connected, while generally PacketConn is about unconnected sockets, this will work for me. I just need to preserve conn.RemoteAddr() somehow... I'll manage

(2) Fix gonet.Conn.Read(), to have proper semantics on udp sockets. Not sure how much work it is.

(3) I could rewrite my code to not use gonet abstractions, and instead manage my own conntrack table (putting flow 5-tuples in a table, managing the lifetime of these, etc). I don't want to do that because I'm afraid pushing all the packets through udp.ForwarderRequest could hamper throughput.

[iangudger]

gonet.PacketConn implements net.Conn in addition to net.PacketConn, and as a result, can represent connected UDP endpoints.

[majek]

Ok, I submitted the minimal NewPacketConn function, which is sufficient to fix my particular problem https://github.com/google/gvisor/pull/1676

The problem of gonet.Conn read() semantics remain; I just won't use gonet.Conn with UDP.

[majek]

Thanks, with NewPacketConn exposed I'm now happy https://github.com/majek/slirpnetstack/commit/3e20584a669aad3f5dfe9b333fc98a1ba6f05d25

I think we can close this bug; we probably should rename gonet.Conn to gonet.TCPConn, or at least make it clear in docs that it's intended for tcp/SOCK_STREAM;