Re: [PATCH net-next v11 07/23] ovpn: introduce the ovpn_socket object

[Sergey Ryazanov <ryazanov.s.a@gmail.com>]

On November 21, 2024 11:36:19 PM, Antonio Quartulli <antonio@openvpn.net> wrote:
>On 21/11/2024 00:58, Sergey Ryazanov wrote:
>> On 15.11.2024 16:28, Antonio Quartulli wrote:
>>> On 10/11/2024 19:26, Sergey Ryazanov wrote:
>>>> On 29.10.2024 12:47, Antonio Quartulli wrote:
>> 
>> [...]
>> 
>>>>> +static bool ovpn_socket_hold(struct ovpn_socket *sock)
>>>>> +{
>>>>> +    return kref_get_unless_zero(&sock->refcount);
>>>> 
>>>> Why do we need to wrap this kref acquiring call into the function. Why we cannot simply call kref_get_unless_zero() from ovpn_socket_get()?
>>> 
>>> Generally I prefer to keep the API among objects consistent.
>>> In this specific case, it means having hold() and put() helpers in order to avoid calling kref_* functions directly in the code.
>>> 
>>> This is a pretty simple case because hold() is called only once, but I still like to be consistent.
>> 
>> Make sense. The counterpart ovpn_socket_hold() function declared in the header file. Probably that's why I missed it. Shall we move the holding routine there as well?
>
>I prefer not to, because that function is used only in socket.c. Moving/declaring it in socket.h would export a symbols that is not used anywhere else.
>
>The _put() variant is instead use in peer.c, thus it is exported.

Technically, inline function is not exported. On another hand, it makes sense to keep header file clean. Agree.

[...]
 
>>>>> +int ovpn_udp_socket_attach(struct socket *sock, struct ovpn_struct *ovpn)
>>>>> +{
>>>>> +    struct ovpn_socket *old_data;
>>>>> +    int ret = 0;
>>>>> +
>>>>> +    /* sanity check */
>>>>> +    if (sock->sk->sk_protocol != IPPROTO_UDP) {
>>>> 
>>>> The function will be called only for a UDP socket. The caller makes sure this is truth. So, why do we need this check?
>>> 
>>> To avoid this function being copied/called somewhere else in the future and we forget about this critical assumption.
>> 
>> Shall we do the same for all other functions in this file? E.g. ovpn_udp_socket_detach/ovpn_udp_send_skb?
>
>Those functions work on a socket that is already owned, thus it already passed this precheck, while _attach() is the one seeing the new socket for the first time.
>
>If this check is triggered it would only be due to a bug.
>Hence the DEBUG_NET_WARN_ON_ONCE().
>
>> And who is giving guarantee that the code will be copied together with the check?
>
>No guarantee is given :)
>
>> 
>>> Indeed it's a just sanity check.
>> 
>> Shall we check for pointers validity before dereferencing them?
>> 
>> if (!ovpn || !sock || !sock->sk || !sock->sk->sk_protocol != IPPROTO_UDP) {
>> 
>> With the above questions I would like to show that it's endless number of possible mistakes. And no matter how much do we check, a creative engineer will find a way to ruin the kernel.
>> 
>> So, is it worth to spend code lines for checking socket for being UDP inside a function that has '_udp_' in its name and is called only inside the module?
>
>Are you suggesting we should drop any kind of check for functions called only within the module? I am not sure I follow..

Sanity checks in the internal functions, yes. I'm afraid, they give a false feel of safety. Short a clear code for me is more preferable, especially when I know in advance who and how going to call a function.

>Anyway, I am dropping the check at the beginning in the function.