Re: [PATCH net-next 2/4] ovpn: ensure sk is still valid during cleanup
flat view
From: Paolo Abeni <pabeni@redhat.com>
Date: 2025-05-29 09:44:53
On 5/27/25 3:46 PM, Antonio Quartulli wrote:
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c index bea03913bfb1..1dd7e763c168 100644 --- a/drivers/net/ovpn/netlink.c +++ b/drivers/net/ovpn/netlink.c@@ -423,9 +423,14 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info) ovpn_sock = ovpn_socket_new(sock, peer); /* at this point we unconditionally drop the reference to the socket: * - in case of error, the socket has to be dropped - * - if case of success, the socket is configured and let + * - if case of success, the socket is configured and we let * userspace own the reference, so that the latter can - * trigger the final close() + * trigger the final close(). + * + * NOTE: at this point ovpn_socket_new() has acquired a reference + * to sock->sk. That's needed especially to avoid race conditions + * during cleanup, where sock may still be alive, but sock->sk may be + * getting released concurrently.
This comment duplicate some wording from commit message contents and don't add much value IMHO. It could be dropped.
quoted hunk ↗ jump to hunk
@@ -192,19 +189,30 @@ struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer) rcu_read_unlock(); } + /* increase sk refcounter as we'll store a reference in + * ovpn_socket. + * ovpn_socket_release() will decrement the refcounter. + */ + if (!refcount_inc_not_zero(&sk->sk_refcnt)) {
How could sk_refcnt be zero here? likely just sock_hold() is sufficient. Also I think the reference could be acquired a little later, avoiding at least the following chunk. Also IMHO the comment is not very clear. I think it should state explicitly which entity is retaining the reference (AFAICS the peer hash table). /P