netdev - Re: [PATCH net-next 2/4] ovpn: ensure sk is still valid during cleanup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d9cbe73c-1895-42d7-8c21-70487773c94f@redhat.com>
Date: Thu, 29 May 2025 11:44:46 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Antonio Quartulli <antonio@...nvpn.net>, netdev@...r.kernel.org
Cc: Sabrina Dubroca <sd@...asysnail.net>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Oleksandr Natalenko <oleksandr@...alenko.name>,
 Qingfang Deng <dqfext@...il.com>, Gert Doering <gert@...enie.muc.de>
Subject: Re: [PATCH net-next 2/4] ovpn: ensure sk is still valid during
 cleanup

On 5/27/25 3:46 PM, Antonio Quartulli wrote:
> 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.

> @@ -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