| Message ID | 20241029-b4-ovpn-v11-17-de4698c73a25@openvpn.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
2024-10-29, 11:47:30 +0100, Antonio Quartulli wrote: > +static int ovpn_peer_reset_sockaddr(struct ovpn_peer *peer, > + const struct sockaddr_storage *ss, > + const u8 *local_ip) > + __must_hold(&peer->lock) > +{ > + struct ovpn_bind *bind; > + size_t ip_len; > + > + /* create new ovpn_bind object */ > + bind = ovpn_bind_from_sockaddr(ss); > + if (IS_ERR(bind)) > + return PTR_ERR(bind); > + > + if (local_ip) { > + if (ss->ss_family == AF_INET) { > + ip_len = sizeof(struct in_addr); > + } else if (ss->ss_family == AF_INET6) { > + ip_len = sizeof(struct in6_addr); > + } else { > + netdev_dbg(peer->ovpn->dev, "%s: invalid family for remote endpoint\n", > + __func__); ratelimited since that can be triggered from packet processing? [...] > +void ovpn_peer_float(struct ovpn_peer *peer, struct sk_buff *skb) > +{ [...] > + > + switch (family) { > + case AF_INET: > + sa = (struct sockaddr_in *)&ss; > + sa->sin_family = AF_INET; > + sa->sin_addr.s_addr = ip_hdr(skb)->saddr; > + sa->sin_port = udp_hdr(skb)->source; > + salen = sizeof(*sa); > + break; > + case AF_INET6: > + sa6 = (struct sockaddr_in6 *)&ss; > + sa6->sin6_family = AF_INET6; > + sa6->sin6_addr = ipv6_hdr(skb)->saddr; > + sa6->sin6_port = udp_hdr(skb)->source; > + sa6->sin6_scope_id = ipv6_iface_scope_id(&ipv6_hdr(skb)->saddr, > + skb->skb_iif); > + salen = sizeof(*sa6); > + break; > + default: > + goto unlock; > + } > + > + netdev_dbg(peer->ovpn->dev, "%s: peer %d floated to %pIScp", __func__, %u for peer->id? and ratelimited too, probably. (also in ovpn_peer_update_local_endpoint in the previous patch) > + peer->id, &ss); > + ovpn_peer_reset_sockaddr(peer, (struct sockaddr_storage *)&ss, > + local_ip); skip the rehash if this fails? peer->bind will still be the old one so moving it to the new hash chain won't help (the lookup will fail).
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? 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) > + /* 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. > +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) > + if (likely(ovpn_bind_skb_src_match(bind, skb))) > + goto unlock; > + > + family = skb_protocol_to_family(skb); > +
On 04/11/2024 12:24, Sabrina Dubroca wrote: > 2024-10-29, 11:47:30 +0100, Antonio Quartulli wrote: >> +static int ovpn_peer_reset_sockaddr(struct ovpn_peer *peer, >> + const struct sockaddr_storage *ss, >> + const u8 *local_ip) >> + __must_hold(&peer->lock) >> +{ >> + struct ovpn_bind *bind; >> + size_t ip_len; >> + >> + /* create new ovpn_bind object */ >> + bind = ovpn_bind_from_sockaddr(ss); >> + if (IS_ERR(bind)) >> + return PTR_ERR(bind); >> + >> + if (local_ip) { >> + if (ss->ss_family == AF_INET) { >> + ip_len = sizeof(struct in_addr); >> + } else if (ss->ss_family == AF_INET6) { >> + ip_len = sizeof(struct in6_addr); >> + } else { >> + netdev_dbg(peer->ovpn->dev, "%s: invalid family for remote endpoint\n", >> + __func__); > > ratelimited since that can be triggered from packet processing? ACK > > > [...] >> +void ovpn_peer_float(struct ovpn_peer *peer, struct sk_buff *skb) >> +{ > [...] >> + >> + switch (family) { >> + case AF_INET: >> + sa = (struct sockaddr_in *)&ss; >> + sa->sin_family = AF_INET; >> + sa->sin_addr.s_addr = ip_hdr(skb)->saddr; >> + sa->sin_port = udp_hdr(skb)->source; >> + salen = sizeof(*sa); >> + break; >> + case AF_INET6: >> + sa6 = (struct sockaddr_in6 *)&ss; >> + sa6->sin6_family = AF_INET6; >> + sa6->sin6_addr = ipv6_hdr(skb)->saddr; >> + sa6->sin6_port = udp_hdr(skb)->source; >> + sa6->sin6_scope_id = ipv6_iface_scope_id(&ipv6_hdr(skb)->saddr, >> + skb->skb_iif); >> + salen = sizeof(*sa6); >> + break; >> + default: >> + goto unlock; >> + } >> + >> + netdev_dbg(peer->ovpn->dev, "%s: peer %d floated to %pIScp", __func__, > > %u for peer->id? > > and ratelimited too, probably. > > (also in ovpn_peer_update_local_endpoint in the previous patch) Technically we don't expect that frequent float/endpoint updates, but should they happen..better to be protected. ACK > >> + peer->id, &ss); >> + ovpn_peer_reset_sockaddr(peer, (struct sockaddr_storage *)&ss, >> + local_ip); > > skip the rehash if this fails? peer->bind will still be the old one so > moving it to the new hash chain won't help (the lookup will fail). Yeah, it makes sense. Thanks a lot. Regards,
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); >> + >
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?
On 13/11/2024 12:25, Sabrina Dubroca wrote: > 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? At the moment userspace won't try to do that, but I can foresee some future use cases: i.e. a peer that switches to a different interface and needs to open a new socket to keep sending data. Moreover, in userspace we're currently working on multisocket support (theoretically server side only), therefore I can imagine a peer floating from one socket to the other while keeping the session alive. This is all work in progress, but not that far in the future. For TCP, you're right, although at some point we may even implement transport reconnections without losing the VPN state (this is not even planned, just a brain dump). > >> 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? I agree with you. I'll make the socket unmutable again and I'll work on this later on. Thanks a lot for digging with me into this. Regards,
diff --git a/drivers/net/ovpn/bind.c b/drivers/net/ovpn/bind.c index b4d2ccec2ceddf43bc445b489cc62a578ef0ad0a..d17d078c5730bf4336dc87f45cdba3f6b8cad770 100644 --- a/drivers/net/ovpn/bind.c +++ b/drivers/net/ovpn/bind.c @@ -47,12 +47,8 @@ struct ovpn_bind *ovpn_bind_from_sockaddr(const struct sockaddr_storage *ss) * @new: the new bind to assign */ void ovpn_bind_reset(struct ovpn_peer *peer, struct ovpn_bind *new) + __must_hold(&peer->lock) { - struct ovpn_bind *old; - - spin_lock_bh(&peer->lock); - old = rcu_replace_pointer(peer->bind, new, true); - spin_unlock_bh(&peer->lock); - - kfree_rcu(old, rcu); + kfree_rcu(rcu_replace_pointer(peer->bind, new, + lockdep_is_held(&peer->lock)), rcu); } 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) { + /* 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); + } + /* point to encapsulated IP packet */ __skb_pull(skb, payload_offset); diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c index 3f67d200e283213fcb732d10f9edeb53e0a0e9ee..da6215bbb643592e4567e61e4b4976d367ed109c 100644 --- a/drivers/net/ovpn/peer.c +++ b/drivers/net/ovpn/peer.c @@ -94,6 +94,131 @@ struct ovpn_peer *ovpn_peer_new(struct ovpn_struct *ovpn, u32 id) return peer; } +/** + * ovpn_peer_reset_sockaddr - recreate binding for peer + * @peer: peer to recreate the binding for + * @ss: sockaddr to use as remote endpoint for the binding + * @local_ip: local IP for the binding + * + * Return: 0 on success or a negative error code otherwise + */ +static int ovpn_peer_reset_sockaddr(struct ovpn_peer *peer, + const struct sockaddr_storage *ss, + const u8 *local_ip) + __must_hold(&peer->lock) +{ + struct ovpn_bind *bind; + size_t ip_len; + + /* create new ovpn_bind object */ + bind = ovpn_bind_from_sockaddr(ss); + if (IS_ERR(bind)) + return PTR_ERR(bind); + + if (local_ip) { + if (ss->ss_family == AF_INET) { + ip_len = sizeof(struct in_addr); + } else if (ss->ss_family == AF_INET6) { + ip_len = sizeof(struct in6_addr); + } else { + netdev_dbg(peer->ovpn->dev, "%s: invalid family for remote endpoint\n", + __func__); + kfree(bind); + return -EINVAL; + } + + memcpy(&bind->local, local_ip, ip_len); + } + + /* set binding */ + ovpn_bind_reset(peer, bind); + + return 0; +} + +#define ovpn_get_hash_head(_tbl, _key, _key_len) ({ \ + typeof(_tbl) *__tbl = &(_tbl); \ + (&(*__tbl)[jhash(_key, _key_len, 0) % HASH_SIZE(*__tbl)]); }) \ + +/** + * ovpn_peer_float - update remote endpoint for peer + * @peer: peer to update the remote endpoint for + * @skb: incoming packet to retrieve the source address (remote) from + */ +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); + if (likely(ovpn_bind_skb_src_match(bind, skb))) + goto unlock; + + family = skb_protocol_to_family(skb); + + if (bind->remote.in4.sin_family == family) + local_ip = (u8 *)&bind->local; + + switch (family) { + case AF_INET: + sa = (struct sockaddr_in *)&ss; + sa->sin_family = AF_INET; + sa->sin_addr.s_addr = ip_hdr(skb)->saddr; + sa->sin_port = udp_hdr(skb)->source; + salen = sizeof(*sa); + break; + case AF_INET6: + sa6 = (struct sockaddr_in6 *)&ss; + sa6->sin6_family = AF_INET6; + sa6->sin6_addr = ipv6_hdr(skb)->saddr; + sa6->sin6_port = udp_hdr(skb)->source; + sa6->sin6_scope_id = ipv6_iface_scope_id(&ipv6_hdr(skb)->saddr, + skb->skb_iif); + salen = sizeof(*sa6); + break; + default: + goto unlock; + } + + netdev_dbg(peer->ovpn->dev, "%s: peer %d floated to %pIScp", __func__, + peer->id, &ss); + ovpn_peer_reset_sockaddr(peer, (struct sockaddr_storage *)&ss, + local_ip); + spin_unlock_bh(&peer->lock); + rcu_read_unlock(); + + /* rehashing is required only in MP mode as P2P has one peer + * only and thus there is no hashtable + */ + if (peer->ovpn->mode == OVPN_MODE_MP) { + spin_lock_bh(&peer->ovpn->peers->lock); + /* remove old hashing */ + hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr); + /* re-add with new transport address */ + nhead = ovpn_get_hash_head(peer->ovpn->peers->by_transp_addr, + &ss, salen); + hlist_nulls_add_head_rcu(&peer->hash_entry_transp_addr, nhead); + spin_unlock_bh(&peer->ovpn->peers->lock); + } + return; +unlock: + spin_unlock_bh(&peer->lock); + rcu_read_unlock(); +} + static void ovpn_peer_release(struct ovpn_peer *peer) { if (peer->sock) @@ -186,10 +311,6 @@ static struct in6_addr ovpn_nexthop_from_skb6(struct sk_buff *skb) return rt->rt6i_gateway; } -#define ovpn_get_hash_head(_tbl, _key, _key_len) ({ \ - typeof(_tbl) *__tbl = &(_tbl); \ - (&(*__tbl)[jhash(_key, _key_len, 0) % HASH_SIZE(*__tbl)]); }) \ - /** * ovpn_peer_get_by_vpn_addr4 - retrieve peer by its VPN IPv4 address * @ovpn: the openvpn instance to search diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h index 1a8638d266b11a4a80ee2f088394d47a7798c3af..940cea5372ec0375cfe3e673154a1e0248978409 100644 --- a/drivers/net/ovpn/peer.h +++ b/drivers/net/ovpn/peer.h @@ -155,4 +155,6 @@ void ovpn_peer_keepalive_work(struct work_struct *work); void ovpn_peer_update_local_endpoint(struct ovpn_peer *peer, struct sk_buff *skb); +void ovpn_peer_float(struct ovpn_peer *peer, struct sk_buff *skb); + #endif /* _NET_OVPN_OVPNPEER_H_ */
A peer connected via UDP may change its IP address without reconnecting (float). Add support for detecting and updating the new peer IP/port in case of floating. Signed-off-by: Antonio Quartulli <antonio@openvpn.net> --- drivers/net/ovpn/bind.c | 10 ++-- drivers/net/ovpn/io.c | 9 ++++ drivers/net/ovpn/peer.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++-- drivers/net/ovpn/peer.h | 2 + 4 files changed, 139 insertions(+), 11 deletions(-)