Re: [PATCH net-next v16 07/26] ovpn: introduce the ovpn_socket object

[Sabrina Dubroca <sd@queasysnail.net>]

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