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

[Antonio Quartulli <antonio@openvpn.net>]

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.

> 
> [...]
> 
>>>> +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..

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

Regards,


> 
>>>> +        DEBUG_NET_WARN_ON_ONCE(1);
>>>> +        return -EINVAL;
>>>> +    }
> 
> -- 
> Sergey

-- 
Antonio Quartulli
OpenVPN Inc.