2025-01-06, 00:27:28 +0100, Antonio Quartulli wrote:
> Hi Sabrina,
>
> On 03/01/2025 18:00, Sabrina Dubroca wrote:
> > Hello Antonio,
> >
> > 2024-12-19, 02:42:01 +0100, Antonio Quartulli wrote:
> > > +static void ovpn_socket_release_kref(struct kref *kref)
> > > + __releases(sock->sock->sk)
> > > +{
> > > + struct ovpn_socket *sock = container_of(kref, struct ovpn_socket,
> > > + refcount);
> > > +
> >
> > [extend with bits of patch 9]
> > > /* UDP sockets are detached in this kref callback because
> > > * we now know for sure that all concurrent users have
> > > * finally gone (refcounter dropped to 0).
> > > *
> > > * Moreover, detachment is performed under lock to prevent
> > > * a concurrent ovpn_socket_new() call with the same socket
> > > * to find the socket still attached but with refcounter 0.
> >
> > I'm not convinced this really works, because ovpn_socket_new() doesn't
> > use the same lock. lock_sock and bh_lock_sock both "lock the socket"
> > in some sense, but they're not mutually exclusive (we talked about
> > that around the TCP patch).
>
> You're right - but what prevents us from always using bh_lock_sock?
TCP detach can sleep, and UDP attach as well (setup_udp_tunnel_sock ->
udp_tunnel_encap_enable -> udp_encap_enable -> static_branch_inc ->
static_key_slow_inc -> cpus_read_lock). UDP detach would also not work
under bh_lock_sock if it really disabled encap on the socket (we end
up in udp_tunnel_encap_enable but that doesn't do anything since encap
is already turned on -- but a "real" detach should disable the encap
and do static_branch_dec).
So attach/detach need to be under lock_sock, not bh_lock_sock.
> > Are you fundamentally opposed to making attach permanent? ie, once
> > a UDP or TCP socket is assigned to an ovpn instance, it can't be
> > detached and reused. I think it would be safer, simpler, and likely
> > sufficient (I don't know openvpn much, but I don't see a use case for
> > moving a socket from one ovpn instance to another, or using it without
> > encap).
>
> I hardly believe a socket will ever be moved to a different instance.
> There is no use case (and no userspace support) for that at the moment.
>
> >
> > Rough idea:
> > - ovpn_socket_new is pretty much unchanged (locking still needed to
> > protect against another simultaneous attach attempt, EALREADY case
> > becomes a bit easier)
> > - ovpn_peer_remove doesn't do anything socket-related
> > - use ->encap_destroy/ovpn_tcp_close() to clean up sk_user_data
> > - no more refcounting on ovpn_socket (since the encap can't be
> > removed, the lifetime to ovpn_socket is tied to its socket)
> >
> > What do you think?
>
> hmm how would that work with UDP?
> On a server all clients may disconnect, but the UDP socket is expected to
> still survive and be re-used for new clients (userspace will keep it alive
> and keep listening for new clients).
>
> Or you're saying that the socket will remain "attached" (i.e. sk_user_data
> set to the ovpn_priv*) even when no more clients are connected?
Yes. Once attached, it stays attached.
> >
> > I'm trying to poke holes into this idea now. close() vs attach worries
> > me a bit.
>
> Can that truly happen?
Actually it can't, so this isn't a concern.
> If a socket is going through close(), there should be some way to mark it as
> "non-attachable".
>
> Actually, do we even need to clean up sk_user_data? The socket is being
> destroyed - why clean that up at all?
If we allocated some memory to store per-socket info, we need to free
it when we detach or close. There's no generic mechanism to free
sk_user_data since the core can't know where it came from, maybe
kfree() isn't appropriate.
--
Sabrina
