linux-kernel - Re: [PATCH net-next v11 17/23] ovpn: add support for peer floating

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZzSMrgFmbxX9NtIp@hog>
Date: Wed, 13 Nov 2024 12:25:34 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: 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@...n.ch>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v11 17/23] ovpn: add support for peer floating

2024-11-12, 15:03:00 +0100, Antonio Quartulli wrote:
> On 12/11/2024 11:56, Sabrina Dubroca wrote:
> > 2024-10-29, 11:47:30 +0100, Antonio Quartulli wrote:
> > > diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
> > > index 63c140138bf98e5d1df79a2565b666d86513323d..0e8a6f2c76bc7b2ccc287ad1187cf50f033bf261 100644
> > > --- a/drivers/net/ovpn/io.c
> > > +++ b/drivers/net/ovpn/io.c
> > > @@ -135,6 +135,15 @@ void ovpn_decrypt_post(void *data, int ret)
> > >   	/* keep track of last received authenticated packet for keepalive */
> > >   	peer->last_recv = ktime_get_real_seconds();
> > > +	if (peer->sock->sock->sk->sk_protocol == IPPROTO_UDP) {
> > 
> > What prevents peer->sock from being replaced and released
> > concurrently?
> 
> Technically nothing.
> Userspace currently does not even support updating a peer socket at runtime,
> but I wanted ovpn to be flexible enough from the beginning.

Is there a reason to do that? With TCP the peer would have to
reconnect, and I guess fully restart the whole process (become a new
peer with a new ID etc). With UDP, do you need to replace the socket?

> One approach might be to go back to peer->sock being unmutable and forget
> about this.
> 
> OTOH, if we want to keep this flexibility (which I think is nice), I think I
> should make peer->sock an RCU pointer and access it accordingly.

You already use kfree_rcu for ovpn_socket, so the only difference
would be the __rcu annotation and helpers? (+ rcu_read_lock/unlock in
a few places)

Adding rcu_read_lock for peer->sock in ovpn_tcp_tx_work looks
painful... (another place that I missed where things could go bad if
the socket was updated in the current implementation, btw)

Maybe save that for later since you don't have a use case for it yet?

-- 
Sabrina