Re: [PATCH net-next v3 13/24] ovpn: implement TCP transport

[Sabrina Dubroca <sd@queasysnail.net>]

2024-05-15, 21:44:44 +0200, Antonio Quartulli wrote:
> On 15/05/2024 16:55, Sabrina Dubroca wrote:
> > 2024-05-15, 14:54:49 +0200, Antonio Quartulli wrote:
> > > On 15/05/2024 12:19, Sabrina Dubroca wrote:
> > > > 2024-05-15, 00:11:28 +0200, Antonio Quartulli wrote:
> > > > > On 14/05/2024 10:58, Sabrina Dubroca wrote:
> > > > > > > > The UDP code differentiates "socket already owned by this interface"
> > > > > > > > from "already taken by other user". That doesn't apply to TCP?
> > > > > > > 
> > > > > > > This makes me wonder: how safe it is to interpret the user data as an object
> > > > > > > of type ovpn_socket?
> > > > > > > 
> > > > > > > When we find the user data already assigned, we don't know what was really
> > > > > > > stored in there, right?
> > > > > > > Technically this socket could have gone through another module which
> > > > > > > assigned its own state.
> > > > > > > 
> > > > > > > Therefore I think that what UDP does [ dereferencing ((struct ovpn_socket
> > > > > > > *)user_data)->ovpn ] is probably not safe. Would you agree?
> > > > > > 
> > > > > > Hmmm, yeah, I think you're right. If you checked encap_type ==
> > > > > > UDP_ENCAP_OVPNINUDP before (sk_prot for TCP), then you'd know it's
> > > > > > really your data. Basically call ovpn_from_udp_sock during attach if
> > > > > > you want to check something beyond EBUSY.
> > > > > 
> > > > > right. Maybe we can leave with simply reporting EBUSY and be done with it,
> > > > > without adding extra checks and what not.
> > > > 
> > > > I don't know. What was the reason for the EALREADY handling in udp.c
> > > > and the corresponding refcount increase in ovpn_socket_new?
> > > 
> > > it's just me that likes to be verbose when doing error reporting.
> > 
> > With the "already owned by this interface" message? Sure, I get that.
> > 
> > > But eventually the exact error is ignored and we release the reference. From
> > > netlink.c:
> > > 
> > > 342                 peer->sock = ovpn_socket_new(sock, peer);
> > > 343                 if (IS_ERR(peer->sock)) {
> > > 344                         sockfd_put(sock);
> > > 345                         peer->sock = NULL;
> > > 346                         ret = -ENOTSOCK;
> > > 
> > > so no added value in distinguishing the two cases.
> > 
> > But ovpn_socket_new currently turns EALREADY into a valid result, so
> > we won't go through the error hanadling here. That's the part I'm
> > unclear about.
> 
> you're right. I had forgotten a little but important detail.
> 
> With UDP OpenVPN creates one socket and uses it for all peers.
> With TCP we forcefully need one socket per client.
> 
> Consequently, when a UDP socket is found to be used by our own instance,  we
> can happily increase the refcounter and use it as if it was free (we are
> just attaching it to yet another peer).
> 
> In TCP this is not possible, so the socket must be unused, otherwise we
> can't attach it.
> 
> I hope it makes sense.

Yes, thanks. This behavior should be documented (for example, by
putting exactly what you just wrote in a comment above
ovpn_socket_new).

So for TCP you just need the existing check and EBUSY return. For UDP,
you need the EALREADY check, but with an extra encap_type test before
looking at the contents of the sk_user_data.

-- 
Sabrina