On 10/02/2026 01:07, Antonio Quartulli wrote:
On 09/02/2026 17:34, Sabrina Dubroca wrote:
2026-02-09, 16:36:55 +0100, Antonio Quartulli wrote:
On 09/02/2026 16:32, Antonio Quartulli wrote:
What if we go the other way around: we have ovpn_tcp_close() check if
the peer was already gone from the ovpn hash or not.
If it did, we know the peer is going through its own lifecycle and we
skip the ovpn_peer_del() call.
To make sure I understand: the bug we're trying to fix here is that we
end up calling ovpn_tcp_close() too early, so by the time
ovpn_tcp_socket_detach() is called, sk->sk_socket is already NULL so
"sk->sk_socket->ops = ..." crashes the kernel. Ralf's patch makes sure
ovpn_socket_release() -> ... -> ovpn_tcp_socket_detach() has been
called by the time we're done with close(). Is that correct?
Then I'm confused by how this would solve the issue. We could still be
done with ovpn_tcp_close() before ovpn_peer_keepalive_work() ->
unlock_ovpn -> ovpn_socket_release has completed?
As we have already stated, the crux of the issue is the moment
tcp_close() sets sk->sk_socket to NULL.
This happens in:
1. tcp_close(sk, timeout);
2. __tcp_close(sk, timeout);
3. sock_orphan(sk);
4. sk_set_socket(sk, NULL);
When we later/concurrently invoke ovpn_tcp_socket_release() we
dereference sk->sk_socket, which is now NULL.
sock_orphan()is actually pretty small:
2123 static inline void sock_orphan(struct sock *sk)
2124 {
2125 write_lock_bh(&sk->sk_callback_lock);
2126 sock_set_flag(sk, SOCK_DEAD);
2127 sk_set_socket(sk, NULL);
2128 sk->sk_wq = NULL;
2129 write_unlock_bh(&sk->sk_callback_lock);
2130 }
and as we can see it holds and then releases sk_callback_lock.
I remember that we were originally using this lock when setting the
callbacks, but we eventually dropped it (around v3 or v4 of the original
patchset, but I couldn't find the reason).
Maybe we thought there was no need to hold that lock?
Now, if we actually acquire this lock in ovpn_tcp_socket_detach(), check
that sk_socket is non-NULL, restores the CB and release it, wouldn't we
solve our problem?
Cheers,
--
Antonio Quartulli
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
