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

Antonio Quartulli
2024-05-15T20:37:45Z

On 15/05/2024 22:35, Sabrina Dubroca wrote:
> 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).

absolutely, will do.

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

ACK

--
Antonio Quartulli
OpenVPN Inc.