linux-kernel - Re: [PATCH net-next v15 11/22] 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: <dcb961a8-e0ba-49ea-b1ef-f52439713588@openvpn.net>
Date: Mon, 16 Dec 2024 15:09:17 +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>,
 dsahern@...nel.org
Subject: Re: [PATCH net-next v15 11/22] ovpn: implement TCP transport

On 16/12/2024 14:59, Sabrina Dubroca wrote:
> 2024-12-11, 22:15:15 +0100, Antonio Quartulli wrote:
>> @@ -42,6 +56,31 @@ struct ovpn_peer {
>>   		struct in6_addr ipv6;
>>   	} vpn_addrs;
>>   	struct ovpn_socket *sock;
>> +
>> +	/* state of the TCP reading. Needed to keep track of how much of a
>> +	 * single packet has already been read from the stream and how much is
>> +	 * missing
>> +	 */
> 
> nit: not so accurate since the switch to strp, can probably be dropped
> since @tcp has a kdoc entry

right - dropping it.

> 
>> +	struct {
>> +		struct strparser strp;
>> +		struct work_struct tx_work;
>> +		struct sk_buff_head user_queue;
>> +		struct sk_buff_head out_queue;
>> +		bool tx_in_progress;
>> +
>> +		struct {
>> +			struct sk_buff *skb;
>> +			int offset;
>> +			int len;
>> +		} out_msg;
>> +
>> +		struct {
>> +			void (*sk_data_ready)(struct sock *sk);
>> +			void (*sk_write_space)(struct sock *sk);
>> +			struct proto *prot;
>> +			const struct proto_ops *ops;
>> +		} sk_cb;
>> +	} tcp;
> 
> [...]
>> +static void ovpn_tcp_send_sock_skb(struct ovpn_peer *peer, struct sk_buff *skb)
>> +{
>> +	if (peer->tcp.out_msg.skb)
>> +		ovpn_tcp_send_sock(peer);
>> +
>> +	if (peer->tcp.out_msg.skb) {
>> +		dev_core_stats_rx_dropped_inc(peer->ovpn->dev);
> 
> tx_dropped?

ACK

> 
>> +		kfree_skb(skb);
>> +		return;
>> +	}
>> +
>> +	peer->tcp.out_msg.skb = skb;
>> +	peer->tcp.out_msg.len = skb->len;
>> +	peer->tcp.out_msg.offset = 0;
>> +	ovpn_tcp_send_sock(peer);
>> +}
>> +
>> +void ovpn_tcp_send_skb(struct ovpn_peer *peer, struct sk_buff *skb)
>> +{
>> +	u16 len = skb->len;
>> +
>> +	*(__be16 *)__skb_push(skb, sizeof(u16)) = htons(len);
>> +
>> +	bh_lock_sock(peer->sock->sock->sk);
>> +	if (sock_owned_by_user(peer->sock->sock->sk)) {
>> +		if (skb_queue_len(&peer->tcp.out_queue) >=
>> +		    READ_ONCE(net_hotdata.max_backlog)) {
>> +			dev_core_stats_rx_dropped_inc(peer->ovpn->dev);
> 
> tx_dropped?

ACK

> 
>> +			kfree_skb(skb);
>> +			goto unlock;
>> +		}
>> +		__skb_queue_tail(&peer->tcp.out_queue, skb);
>> +	} else {
>> +		ovpn_tcp_send_sock_skb(peer, skb);
>> +	}
>> +unlock:
>> +	bh_unlock_sock(peer->sock->sock->sk);
>> +}
> 
> [...]
>> +static void ovpn_tcp_close(struct sock *sk, long timeout)
>> +{
>> +	struct ovpn_socket *sock;
>> +
>> +	rcu_read_lock();
> 
> [can't sleep until unlock]
> 
>> +	sock = rcu_dereference_sk_user_data(sk);
>> +
>> +	strp_stop(&sock->peer->tcp.strp);
>> +
>> +	tcp_close(sk, timeout);
> 
> 
>      void tcp_close(struct sock *sk, long timeout)
>      {
>      	lock_sock(sk);
> 
> but this can sleep.

Ouch.. I wonder why I have never seen the might_sleep() trigger this, 
but probably that's due to the fact that we hardly hit this cb in the 
classic use case.

> 
> Is there anything that prevents delaying tcp_close until after
> ovpn_peer_del and rcu_read_unlock?

not really.

> 
>> +	ovpn_peer_del(sock->peer, OVPN_DEL_PEER_REASON_TRANSPORT_ERROR);
>> +	rcu_read_unlock();

I will move the tcp_close() here.

>> +}
> 
> [...]
>> +void __init ovpn_tcp_init(void)
>> +{
>> +	ovpn_tcp_build_protos(&ovpn_tcp_prot, &ovpn_tcp_ops, &tcp_prot,
>> +			      &inet_stream_ops);
>> +
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +	ovpn_tcp_build_protos(&ovpn_tcp6_prot, &ovpn_tcp6_ops, &tcpv6_prot,
>> +			      &inet6_stream_ops);
> 
> I don't think that works for CONFIG_OVPN=y and CONFIG_IPV6=m. You can
> either go back to the ugly thing espintcp and tls do, or use the
> traditional Kconfig hack:
> 
> 	depends on IPV6 || !IPV6
> 
> (you can find it sprinkled in various places of drivers/net/Kconfig
> and net/)

I'll go for the Kconfig hack. Hopefully one day IPV6 will become bool..

Thanks!

Regards,


-- 
Antonio Quartulli
OpenVPN Inc.