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

[Sabrina Dubroca <sd@queasysnail.net>]

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.

-- 
Sabrina