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

Antonio Quartulli
2024-05-15T19:43:25Z

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.

>

--
Antonio Quartulli
OpenVPN Inc.