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?
[...]
+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? And who is giving guarantee
that the code will be copied together with the check?
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?
+ DEBUG_NET_WARN_ON_ONCE(1);
+ return -EINVAL;
+ }
--
Sergey
