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.
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. Does it make sense?
Or possibly reading the error value that ovpn_socket_new can return before peer->sock is reset to NULL, just noticed this in ovpn_nl_peer_modify:
if (attrs[OVPN_A_PEER_SOCKET]) { // ... peer->sock = ovpn_socket_new(sock, peer); if (IS_ERR(peer->sock)) { // ... peer->sock = NULL;
(ovpn_encrypt_post has a similar check on peer->sock->sock->sk->sk_protocol that I don't think is safe either)
Yap, agreed.
/* check if this peer changed it's IP address and update* state*/ovpn_peer_float(peer, skb);/* update source endpoint for this peer */ovpn_peer_update_local_endpoint(peer, skb);Why not do both in the same function? They're not called anywhere else (at least in this version of the series). They both modify peer->bind depending on skb_protocol_to_family(skb), and operate under peer->lock.
I never considered to do so as I just always assumed the two to be two separate features/routines.
I think it's a good idea and I would get rid of a few common instructions (along with acquiring the lock twice). Thanks!
+void ovpn_peer_float(struct ovpn_peer *peer, struct sk_buff *skb) +{
- struct hlist_nulls_head *nhead;
- struct sockaddr_storage ss;
- const u8 *local_ip = NULL;
- struct sockaddr_in6 *sa6;
- struct sockaddr_in *sa;
- struct ovpn_bind *bind;
- sa_family_t family;
- size_t salen;
- rcu_read_lock();
- bind = rcu_dereference(peer->bind);
- if (unlikely(!bind)) {
rcu_read_unlock();return;- }
- spin_lock_bh(&peer->lock);
You could take the lock from the start, instead of using rcu_read_lock to get peer->bind. It would guarantee that the bind we got isn't already being replaced just as we wait to update it. And same in ovpn_peer_update_local_endpoint, it would make sure we're updating the local IP for the active bind.
(sorry I didn't think about that last time we discussed this)
no worries :) and I like the idea. will do that, thanks.
- if (likely(ovpn_bind_skb_src_match(bind, skb)))
goto unlock;- family = skb_protocol_to_family(skb);