netdev - Re: [PATCH net-next v20 12/25] ovpn: implement TCP transport

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: <2a677967-da28-430e-b794-26c5c17e9963@openvpn.net>
Date: Sun, 2 Mar 2025 21:59:43 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Donald Hunter <donald.hunter@...il.com>, Shuah Khan <shuah@...nel.org>,
 ryazanov.s.a@...il.com, Andrew Lunn <andrew+netdev@...n.ch>,
 Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org, Xiao Liang <shaw.leon@...il.com>,
 David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH net-next v20 12/25] ovpn: implement TCP transport

On 02/03/2025 19:59, Sabrina Dubroca wrote:
> 2025-02-27, 02:21:37 +0100, Antonio Quartulli wrote:
>> Moreover export tcp_release_cb by means of EXPORT_SYMBOL instead of
>> EXPORT_IPV6_MOD, so that other modules can use it, even if IPV6 is
>> not compiled in.
> 
> Is that really needed? You're saving tcp.sk_cb.prot, so you could just
> call peer->tcp.sk_cb.prot->release_cb? (with a bit of care since it's
> called after peer_put)
> 
> [I don't know what the maintainers' preference is wrt "re-exporting"
> symbols that got moved to EXPORT_IPV6_MOD]

Mh yeah, I like your suggestion as I don't have to change the EXPORT and 
also I don't have to make assumptions on the original value of 
->release_cb as I can just call it.

Will follow your suggestion. Thanks!

> 
> 
> [...]
>> +static void ovpn_tcp_send_sock(struct ovpn_peer *peer, struct sock *sk)
>> +{
>> +	struct sk_buff *skb = peer->tcp.out_msg.skb;
>> +
>> +	if (!skb)
>> +		return;
>> +
>> +	if (peer->tcp.tx_in_progress)
>> +		return;
>> +
>> +	peer->tcp.tx_in_progress = true;
>> +
>> +	do {
>> +		int ret = skb_send_sock_locked(sk, skb,
>> +					       peer->tcp.out_msg.offset,
>> +					       peer->tcp.out_msg.len);
>> +		if (unlikely(ret < 0)) {
>> +			if (ret == -EAGAIN)
>> +				goto out;
>> +
>> +			net_warn_ratelimited("%s: TCP error to peer %u: %d\n",
>> +					     netdev_name(peer->ovpn->dev),
>> +					     peer->id, ret);
>> +
>> +			/* in case of TCP error we can't recover the VPN
>> +			 * stream therefore we abort the connection
>> +			 */
>> +			ovpn_peer_del(peer,
>> +				      OVPN_DEL_PEER_REASON_TRANSPORT_ERROR);
> 
> I don't think this works:
> 
> ovpn_peer_del -> unlock_ovpn -> ovpn_socket_release -> might_sleep
> 
> but we can get to ovpn_tcp_send_sock in a few contexts that are not
> allowed to sleep:
> 
> ovpn_tcp_send_skb -> ovpn_tcp_send_sock_skb -> ovpn_tcp_send_sock
> __sk_flush_backlog -> release_cb = ovpn_tcp_release -> ovpn_tcp_send_sock_skb
> release_sock       -> release_cb = ovpn_tcp_release -> ovpn_tcp_send_sock_skb
> 
> 
> (I checked all other paths leading to unlock_ovpn/ovpn_socket_release,
> this is the only one I could find that is not allowed to sleep. So it
> would likely be easier to push this peer_del (or even just the
> handling of release_list) into some other sleepable context than
> trying to reshuffle all the other paths)

ACK (and thanks for double checking all other paths!)
I will do some magic on this peer_del() in order to make it sleep 
without issues.

Thanks!

Regards,

> 

-- 
Antonio Quartulli
OpenVPN Inc.