Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink
On 29/11/2024 18:00, Sabrina Dubroca wrote:
2024-11-14, 11:32:36 +0100, Antonio Quartulli wrote:
On 13/11/2024 12:05, Sabrina Dubroca wrote:
2024-11-12, 15:26:59 +0100, Antonio Quartulli wrote:
On 11/11/2024 16:41, Sabrina Dubroca wrote:
2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
+void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer)
+ __must_hold(&peer->ovpn->peers->lock)
Changes to peer->vpn_addrs are not protected by peers->lock, so those
could be getting updated while we're rehashing (and taking peer->lock
in ovpn_nl_peer_modify as I'm suggesting above also wouldn't prevent
that).
/me screams :-D
Sorry :)
Indeed peers->lock is only about protecting the lists, not the content of
the listed objects.
How about acquiring the peers->lock before calling ovpn_nl_peer_modify()?
It seems like it would work. Maybe a bit weird to have conditional
locking (MP mode only), but ok. You already have this lock ordering
(hold peers->lock before taking peer->lock) in
ovpn_peer_keepalive_work_mp, so there should be no deadlock from doing
the same thing in the netlink code.
Yeah.
Then I would also do that in ovpn_peer_float to protect that rehash.
I am not extremely comfortable with this, because it means acquiring
peers->lock on every packet (right now we do so only on peer->lock) and it
may defeat the advantage of the RCU locking on the hashtables.
Wouldn't you agree?
Hmpf, yeah. Then I think you could keep most of the current code,
except doing the rehash under both locks (peers + peer), and get
ss+sa_len for the rehash directly from peer->bind (instead of using
the ones we just defined locally in ovpn_peer_float, since they may
have changed while we released peer->lock to grab peers->lock). We may
end up "rehashing" twice into the same bucket if we have 2 concurrent
peer_float calls (call 1 sets remote r1, call 2 sets a new one r2,
call 1 hashes according to r2, call 2 also rehashes based on r2). That
should be ok (it can happen anyway that a "real" rehash lands in the
same bucket).
I think the double rehashing is ok. It's a double float happening so we
expect a double rehashing in any case.
peer_float {
spin_lock(peer)
match/update bind
spin_unlock(peer)
if (MP) {
spin_lock(peers)
spin_lock(peer)
rehash using peer->bind->remote rather than ss
spin_unlock(peer)
spin_unlock(peers)
}
}
Does that sound reasonable?
Yeah, not very elegant, but this is what we need :)
Thanks!
Regards,
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink
2024-11-14, 11:32:36 +0100, Antonio Quartulli wrote:
> On 13/11/2024 12:05, Sabrina Dubroca wrote:
> > 2024-11-12, 15:26:59 +0100, Antonio Quartulli wrote:
> > > On 11/11/2024 16:41, Sabrina Dubroca wrote:
> > > > 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
> > > > > +void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer)
> > > > > + __must_hold(&peer->ovpn->peers->lock)
> > > >
> > > > Changes to peer->vpn_addrs are not protected by peers->lock, so those
> > > > could be getting updated while we're rehashing (and taking peer->lock
> > > > in ovpn_nl_peer_modify as I'm suggesting above also wouldn't prevent
> > > > that).
> > > >
> > >
> > > /me screams :-D
> >
> > Sorry :)
> >
> > > Indeed peers->lock is only about protecting the lists, not the content of
> > > the listed objects.
> > >
> > > How about acquiring the peers->lock before calling ovpn_nl_peer_modify()?
> >
> > It seems like it would work. Maybe a bit weird to have conditional
> > locking (MP mode only), but ok. You already have this lock ordering
> > (hold peers->lock before taking peer->lock) in
> > ovpn_peer_keepalive_work_mp, so there should be no deadlock from doing
> > the same thing in the netlink code.
>
> Yeah.
>
> >
> > Then I would also do that in ovpn_peer_float to protect that rehash.
>
> I am not extremely comfortable with this, because it means acquiring
> peers->lock on every packet (right now we do so only on peer->lock) and it
> may defeat the advantage of the RCU locking on the hashtables.
> Wouldn't you agree?
Hmpf, yeah. Then I think you could keep most of the current code,
except doing the rehash under both locks (peers + peer), and get
ss+sa_len for the rehash directly from peer->bind (instead of using
the ones we just defined locally in ovpn_peer_float, since they may
have changed while we released peer->lock to grab peers->lock). We may
end up "rehashing" twice into the same bucket if we have 2 concurrent
peer_float calls (call 1 sets remote r1, call 2 sets a new one r2,
call 1 hashes according to r2, call 2 also rehashes based on r2). That
should be ok (it can happen anyway that a "real" rehash lands in the
same bucket).
peer_float {
spin_lock(peer)
match/update bind
spin_unlock(peer)
if (MP) {
spin_lock(peers)
spin_lock(peer)
rehash using peer->bind->remote rather than ss
spin_unlock(peer)
spin_unlock(peers)
}
}
Does that sound reasonable?
--
Sabrina
Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink
On 21/11/2024 17:02, Sabrina Dubroca wrote:
[I'm still thinking about the locking problems for ovpn_peer_float,
but just noticed this while staring at the rehash code]
2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
+void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer)
+ __must_hold(&peer->ovpn->peers->lock)
+{
+ struct hlist_nulls_head *nhead;
+
+ if (peer->vpn_addrs.ipv4.s_addr != htonl(INADDR_ANY)) {
+ /* remove potential old hashing */
+ hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr);
s/hash_entry_transp_addr/hash_entry_addr4/ ?
cr0p. very good catch!
Thanks
+ nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr,
+ &peer->vpn_addrs.ipv4,
+ sizeof(peer->vpn_addrs.ipv4));
+ hlist_nulls_add_head_rcu(&peer->hash_entry_addr4, nhead);
+ }
+
+ if (!ipv6_addr_any(&peer->vpn_addrs.ipv6)) {
+ /* remove potential old hashing */
+ hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr);
s/hash_entry_transp_addr/hash_entry_addr6/ ?
Thanks²
This is what happens when you copy/paste code around.
+ nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr,
+ &peer->vpn_addrs.ipv6,
+ sizeof(peer->vpn_addrs.ipv6));
+ hlist_nulls_add_head_rcu(&peer->hash_entry_addr6, nhead);
+ }
+}
Regards,
--
Antonio Quartulli
OpenVPN Inc.
Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink
[I'm still thinking about the locking problems for ovpn_peer_float,
but just noticed this while staring at the rehash code]
2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
> +void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer)
> + __must_hold(&peer->ovpn->peers->lock)
> +{
> + struct hlist_nulls_head *nhead;
> +
> + if (peer->vpn_addrs.ipv4.s_addr != htonl(INADDR_ANY)) {
> + /* remove potential old hashing */
> + hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr);
s/hash_entry_transp_addr/hash_entry_addr4/ ?
> + nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr,
> +&peer->vpn_addrs.ipv4,
> +sizeof(peer->vpn_addrs.ipv4));
> + hlist_nulls_add_head_rcu(&peer->hash_entry_addr4, nhead);
> + }
> +
> + if (!ipv6_addr_any(&peer->vpn_addrs.ipv6)) {
> + /* remove potential old hashing */
> + hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr);
s/hash_entry_transp_addr/hash_entry_addr6/ ?
> + nhead = ovpn_get_hash_head(peer->ovpn->peers->by_vpn_addr,
> +&peer->vpn_addrs.ipv6,
> +sizeof(peer->vpn_addrs.ipv6));
> + hlist_nulls_add_head_rcu(&peer->hash_entry_addr6, nhead);
> + }
> +}
--
Sabrina
Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink
2024-11-20, 12:34:08 +0100, Antonio Quartulli wrote:
> On 20/11/2024 12:12, Sabrina Dubroca wrote:
[...]
> > > > I don't know when userspace would use v4mapped addresses,
> > >
> > > It happens when listening on [::] with a v6 socket that has no
> > > "IPV6_V6ONLY"
> > > set to true (you can check ipv6(7) for more details).
> > > This socket can receive IPv4 connections, which are implemented using
> > > v4mapped addresses. In this case both remote and local are going to be
> > > v4mapped.
> >
> > I'm familiar with v4mapped addresses, but I wasn't sure the userspace
> > part would actually passed them as peer. But I guess it would when the
> > peer connects over ipv4 on an ipv6 socket.
> >
> > So the combination of PEER_IPV4 with LOCAL_IPV6(v4mapped) should never
> > happen? In that case I guess we just need to check that we got 2
> > attributes of the same type (both _IPV4 or both _IPV6) and if we got
> > _IPV6, that they're either both v4mapped or both not. Might be a tiny
> > bit simpler than what I was suggesting below.
>
> Exactly - this is what I was originally suggesting, but your solution is
> just a bit cleaner imho.
Ok.
> > > However, the sanity check should make sure nobody can inject bogus
> > > combinations.
> > >
> > > > but treating
> > > > a v4mapped address as a "proper" ipv4 address should work with the
> > > > rest of the code, since you already have the conversion in
> > > > ovpn_nl_attr_local_ip and ovpn_nl_attr_sockaddr_remote. So maybe you
> > > > could do something like (rough idea and completely untested):
> > > >
> > > > static int get_family(attr_v4, attr_v6)
> > > > {
> > > > if (attr_v4)
> > > > return AF_INET;
> > > > if (attr_v6) {
> > > > if (ipv6_addr_v4mapped(attr_v6)
> > > > return AF_INET;
> > > > return AF_INET6;
> > > > }
> > > > return AF_UNSPEC;
> > > > }
> > > >
> > > >
> > > > // in _precheck:
> > > > // keep the attrs[OVPN_A_PEER_REMOTE_IPV4] &&
> > > > attrs[OVPN_A_PEER_REMOTE_IPV6] check
> > > > // maybe add a similar one for LOCAL_IPV4 && LOCAL_IPV6
> > >
> > > the latter is already covered by:
> > >
> > > 192 if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
> > > 193 attrs[OVPN_A_PEER_LOCAL_IPV4]) {
> > > 194 NL_SET_ERR_MSG_MOD(info->extack,
> > > 195"cannot specify local IPv4
> > > address
> > > without remote");
> > > 196 return -EINVAL;
> > > 197 }
> > > 198
> > > 199 if (!attrs[OVPN_A_PEER_REMOTE_IPV6] &&
> > > 200 attrs[OVPN_A_PEER_LOCAL_IPV6]) {
> > > 201 NL_SET_ERR_MSG_MOD(info->extack,
> > > 202"cannot specify local IPV6
> > > address
> > > without remote");
> > > 203 return -EINVAL;
> > > 204 }
> >
> > LOCAL_IPV4 combined with REMOTE_IPV6 should be fine if the remote is
> > v4mapped. And conversely, LOCAL_IPV6 combined with REMOTE_IPV6 isn't
> > ok if remote is v4mapped. So those checks should go away and be
> > replaced with the "get_family" thing, but that requires at most one of
> > the _IPV4/_IPV6 attributes to be present to behave consistently.
>
> I don't expect to receive a mix of _IPV4 and _IPV6, because the assumption
> is that either both addresses are v4mapped or none.
>
> Userspace fetches the addresses from the received packet, so I presume they
> will both be exposed as v4mapped if we are in this special case.
>
> Hence, I don't truly want to allow combining them.
>
> Does it make sense?
Yup, thanks.
--
Sabrina
Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink
On 20/11/2024 12:12, Sabrina Dubroca wrote:
2024-11-14, 10:21:18 +0100, Antonio Quartulli wrote:
On 13/11/2024 17:56, Sabrina Dubroca wrote:
2024-11-12, 15:19:50 +0100, Antonio Quartulli wrote:
On 04/11/2024 16:14, Sabrina Dubroca wrote:
2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
+static int ovpn_nl_peer_precheck(struct ovpn_struct *ovpn,
+struct genl_info *info,
+struct nlattr **attrs)
+{
+ if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs,
+ OVPN_A_PEER_ID))
+ return -EINVAL;
+
+ if (attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6]) {
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "cannot specify both remote IPv4 or IPv6
address");
+ return -EINVAL;
+ }
+
+ if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
+ !attrs[OVPN_A_PEER_REMOTE_IPV6] && attrs[OVPN_A_PEER_REMOTE_PORT]) {
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "cannot specify remote port without IP
address");
+ return -EINVAL;
+ }
+
+ if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
+ attrs[OVPN_A_PEER_LOCAL_IPV4]) {
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "cannot specify local IPv4 address without
remote");
+ return -EINVAL;
+ }
+
+ if (!attrs[OVPN_A_PEER_REMOTE_IPV6] &&
+ attrs[OVPN_A_PEER_LOCAL_IPV6]) {
I think these consistency checks should account for v4mapped
addresses. With remote=v4mapped and local=v6 we'll end up with an
incorrect ipv4 "local" address (taken out of the ipv6 address's first
4B by ovpn_peer_reset_sockaddr). With remote=ipv6 and local=v4mapped,
we'll pass the last 4B of OVPN_A_PEER_LOCAL_IPV6 to
ovpn_peer_reset_sockaddr and try to read 16B (the full ipv6 address)
out of that.
Right, a v4mapped address would fool this check.
How about checking if both or none addresses are v4mapped? This way we
should prevent such cases.
I don't know when userspace would use v4mapped addresses,
It happens when listening on [::] with a v6 socket that has no "IPV6_V6ONLY"
set to true (you can check ipv6(7) for more details).
This socket can receive IPv4 connections, which are implemented using
v4mapped addresses. In this case both remote and local are going to be
v4mapped.
I'm familiar with v4mapped addresses, but I wasn't sure the userspace
part would actually passed them as peer. But I guess it would when the
peer connects over ipv4 on an ipv6 socket.
So the combination of PEER_IPV4 with LOCAL_IPV6(v4mapped) should never
happen? In that case I guess we just need to check that we got 2
attributes of the same type (both _IPV4 or both _IPV6) and if we got
_IPV6, that they're either both v4mapped or both not. Might be a tiny
bit simpler than what I was suggesting below.
Exactly - this is what I was originally suggesting, but your solution is
just a bit cleaner imho.
However, the sanity check should make sure nobody can inject bogus
combinations.
but treating
a v4mapped address as a "proper" ipv4 address should work with the
rest of the code, since you already have the conversion in
ovpn_nl_attr_local_ip and ovpn_nl_attr_sockaddr_remote. So maybe you
could do something like (rough idea and completely untested):
static int get_family(attr_v4, attr_v6)
{
if (attr_v4)
return AF_INET;
if (attr_v6) {
if (ipv6_addr_v4mapped(attr_v6)
return AF_INET;
return AF_INET6;
}
return AF_UNSPEC;
}
// in _precheck:
// keep the attrs[OVPN_A_PEER_REMOTE_IPV4] &&
attrs[OVPN_A_PEER_REMOTE_IPV6] check
// maybe add a similar one for LOCAL_IPV4 && LOCAL_IPV6
the latter is already covered by:
192 if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
193 attrs[OVPN_A_PEER_LOCAL_IPV4]) {
194 NL_SET_ERR_MSG_MOD(info->extack,
195"cannot specify local IPv4 address
without remote");
196 return -EINVAL;
197 }
198
199 if (!attrs[OVPN_A_PEER_REMOTE_IPV6] &&
200 attrs[OVPN_A_PEER_LOCAL_IPV6]) {
201 NL_SET_ERR_MSG_MOD(info->extack,
202"cannot specify local IPV6 address
without remote");
203 return -EINVAL;
204 }
LOCAL_IPV4 combined with REMOTE_IPV6 should be fine if the remote is
v4mapped. And conversely, LOCAL_IPV6 combined with REMOTE_IPV6 isn't
ok if remote is v4mapped. So those checks should go away and be
replaced with the "get_family" thing, but that requires at most one of
the _IPV4/_IPV6 attributes to be present to behave consistently.
I don't expect to receive a mix of _IPV4 and _IPV6, because the
assumption is that either both addresses
Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink
2024-11-14, 10:21:18 +0100, Antonio Quartulli wrote:
> On 13/11/2024 17:56, Sabrina Dubroca wrote:
> > 2024-11-12, 15:19:50 +0100, Antonio Quartulli wrote:
> > > On 04/11/2024 16:14, Sabrina Dubroca wrote:
> > > > 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
> > > > > +static int ovpn_nl_peer_precheck(struct ovpn_struct *ovpn,
> > > > > + struct genl_info *info,
> > > > > + struct nlattr **attrs)
> > > > > +{
> > > > > + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER],
> > > > > attrs,
> > > > > + OVPN_A_PEER_ID))
> > > > > + return -EINVAL;
> > > > > +
> > > > > + if (attrs[OVPN_A_PEER_REMOTE_IPV4] &&
> > > > > attrs[OVPN_A_PEER_REMOTE_IPV6]) {
> > > > > + NL_SET_ERR_MSG_MOD(info->extack,
> > > > > +"cannot specify both remote IPv4 or
> > > > > IPv6 address");
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
> > > > > + !attrs[OVPN_A_PEER_REMOTE_IPV6] &&
> > > > > attrs[OVPN_A_PEER_REMOTE_PORT]) {
> > > > > + NL_SET_ERR_MSG_MOD(info->extack,
> > > > > +"cannot specify remote port without
> > > > > IP address");
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
> > > > > + attrs[OVPN_A_PEER_LOCAL_IPV4]) {
> > > > > + NL_SET_ERR_MSG_MOD(info->extack,
> > > > > +"cannot specify local IPv4 address
> > > > > without remote");
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + if (!attrs[OVPN_A_PEER_REMOTE_IPV6] &&
> > > > > + attrs[OVPN_A_PEER_LOCAL_IPV6]) {
> > > >
> > > > I think these consistency checks should account for v4mapped
> > > > addresses. With remote=v4mapped and local=v6 we'll end up with an
> > > > incorrect ipv4 "local" address (taken out of the ipv6 address's first
> > > > 4B by ovpn_peer_reset_sockaddr). With remote=ipv6 and local=v4mapped,
> > > > we'll pass the last 4B of OVPN_A_PEER_LOCAL_IPV6 to
> > > > ovpn_peer_reset_sockaddr and try to read 16B (the full ipv6 address)
> > > > out of that.
> > >
> > > Right, a v4mapped address would fool this check.
> > > How about checking if both or none addresses are v4mapped? This way we
> > > should prevent such cases.
> >
> > I don't know when userspace would use v4mapped addresses,
>
> It happens when listening on [::] with a v6 socket that has no "IPV6_V6ONLY"
> set to true (you can check ipv6(7) for more details).
> This socket can receive IPv4 connections, which are implemented using
> v4mapped addresses. In this case both remote and local are going to be
> v4mapped.
I'm familiar with v4mapped addresses, but I wasn't sure the userspace
part would actually passed them as peer. But I guess it would when the
peer connects over ipv4 on an ipv6 socket.
So the combination of PEER_IPV4 with LOCAL_IPV6(v4mapped) should never
happen? In that case I guess we just need to check that we got 2
attributes of the same type (both _IPV4 or both _IPV6) and if we got
_IPV6, that they're either both v4mapped or both not. Might be a tiny
bit simpler than what I was suggesting below.
> However, the sanity check should make sure nobody can inject bogus
> combinations.
>
> > but treating
> > a v4mapped address as a "proper" ipv4 address should work with the
> > rest of the code, since you already have the conversion in
> > ovpn_nl_attr_local_ip and ovpn_nl_attr_sockaddr_remote. So maybe you
> > could do something like (rough idea and completely untested):
> >
> > static int get_family(attr_v4, attr_v6)
> > {
> > if (attr_v4)
> > return AF_INET;
> > if (attr_v6) {
> > if (ipv6_addr_v4mapped(attr_v6)
> > return AF_INET;
> > return AF_INET6;
> > }
> > return AF_UNSPEC;
> > }
> >
> >
> > // in _precheck:
> > // keep the attrs[OVPN_A_PEER_REMOTE_IPV4] &&
> > attrs[OVPN_A_PEER_REMOTE_IPV6] check
> > // maybe add a similar one for LOCAL_IPV4 && LOCAL_IPV6
>
> the latter is already covered by:
>
> 192 if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
> 193 attrs[OVPN_A_PEER_LOCAL_IPV4]) {
> 194 NL_SET_ERR_MSG_MOD(info->extack,
> 195"cannot specify local IPv4 address
> without remote");
> 196 return -EINVAL;
> 197 }
> 198
> 199 if (!attrs[OVPN_A_PEER_REMOTE_IPV6] &&
> 200 attrs[OVPN_A_PEER_LOCAL_IPV6]) {
> 201 NL_SET_ERR_MSG_MOD(info->extack,
> 202"cannot specify local IPV6 address
> without remote");
> 203 return -EINVAL;
> 204 }
LOCAL_IPV4 combined with REMOTE_IPV6 should be fine if the r
Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink
On 13/11/2024 12:05, Sabrina Dubroca wrote: 2024-11-12, 15:26:59 +0100, Antonio Quartulli wrote: On 11/11/2024 16:41, Sabrina Dubroca wrote: 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote: +void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer) + __must_hold(&peer->ovpn->peers->lock) Changes to peer->vpn_addrs are not protected by peers->lock, so those could be getting updated while we're rehashing (and taking peer->lock in ovpn_nl_peer_modify as I'm suggesting above also wouldn't prevent that). /me screams :-D Sorry :) Indeed peers->lock is only about protecting the lists, not the content of the listed objects. How about acquiring the peers->lock before calling ovpn_nl_peer_modify()? It seems like it would work. Maybe a bit weird to have conditional locking (MP mode only), but ok. You already have this lock ordering (hold peers->lock before taking peer->lock) in ovpn_peer_keepalive_work_mp, so there should be no deadlock from doing the same thing in the netlink code. Yeah. Then I would also do that in ovpn_peer_float to protect that rehash. I am not extremely comfortable with this, because it means acquiring peers->lock on every packet (right now we do so only on peer->lock) and it may defeat the advantage of the RCU locking on the hashtables. Wouldn't you agree? An alternative would be to hold peer->lock for the entire function, but this will lead to dead locks...no go either. It feels like peers->lock is turning into a duplicate of ovpn->lock. ovpn->lock used for P2P mode, peers->lock used equivalently for MP mode. You might consider merging them (but I wouldn't see it as necessary for merging the series unless there's a locking issue with the current proposal). I agree: ovpn->lock was introduced to protect ovpn's fields, but actually the only one e protect is peer. They are truly the same and I could therefore get rid of ovpn->peers->lock and always use ovpn->lock. Will see how invasive this is and decide whether to commit it to v12 or not. Thanks! Regards, -- Antonio Quartulli OpenVPN Inc.
Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink
On 13/11/2024 17:56, Sabrina Dubroca wrote:
2024-11-12, 15:19:50 +0100, Antonio Quartulli wrote:
On 04/11/2024 16:14, Sabrina Dubroca wrote:
2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
+static int ovpn_nl_peer_precheck(struct ovpn_struct *ovpn,
+struct genl_info *info,
+struct nlattr **attrs)
+{
+ if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs,
+ OVPN_A_PEER_ID))
+ return -EINVAL;
+
+ if (attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6]) {
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "cannot specify both remote IPv4 or IPv6
address");
+ return -EINVAL;
+ }
+
+ if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
+ !attrs[OVPN_A_PEER_REMOTE_IPV6] && attrs[OVPN_A_PEER_REMOTE_PORT]) {
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "cannot specify remote port without IP
address");
+ return -EINVAL;
+ }
+
+ if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
+ attrs[OVPN_A_PEER_LOCAL_IPV4]) {
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "cannot specify local IPv4 address without
remote");
+ return -EINVAL;
+ }
+
+ if (!attrs[OVPN_A_PEER_REMOTE_IPV6] &&
+ attrs[OVPN_A_PEER_LOCAL_IPV6]) {
I think these consistency checks should account for v4mapped
addresses. With remote=v4mapped and local=v6 we'll end up with an
incorrect ipv4 "local" address (taken out of the ipv6 address's first
4B by ovpn_peer_reset_sockaddr). With remote=ipv6 and local=v4mapped,
we'll pass the last 4B of OVPN_A_PEER_LOCAL_IPV6 to
ovpn_peer_reset_sockaddr and try to read 16B (the full ipv6 address)
out of that.
Right, a v4mapped address would fool this check.
How about checking if both or none addresses are v4mapped? This way we
should prevent such cases.
I don't know when userspace would use v4mapped addresses,
It happens when listening on [::] with a v6 socket that has no
"IPV6_V6ONLY" set to true (you can check ipv6(7) for more details).
This socket can receive IPv4 connections, which are implemented using
v4mapped addresses. In this case both remote and local are going to be
v4mapped.
However, the sanity check should make sure nobody can inject bogus
combinations.
but treating
a v4mapped address as a "proper" ipv4 address should work with the
rest of the code, since you already have the conversion in
ovpn_nl_attr_local_ip and ovpn_nl_attr_sockaddr_remote. So maybe you
could do something like (rough idea and completely untested):
static int get_family(attr_v4, attr_v6)
{
if (attr_v4)
return AF_INET;
if (attr_v6) {
if (ipv6_addr_v4mapped(attr_v6)
return AF_INET;
return AF_INET6;
}
return AF_UNSPEC;
}
// in _precheck:
// keep the attrs[OVPN_A_PEER_REMOTE_IPV4] &&
attrs[OVPN_A_PEER_REMOTE_IPV6] check
// maybe add a similar one for LOCAL_IPV4 && LOCAL_IPV6
the latter is already covered by:
192 if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
193 attrs[OVPN_A_PEER_LOCAL_IPV4]) {
194 NL_SET_ERR_MSG_MOD(info->extack,
195"cannot specify local IPv4
address without remote");
196 return -EINVAL;
197 }
198
199 if (!attrs[OVPN_A_PEER_REMOTE_IPV6] &&
200 attrs[OVPN_A_PEER_LOCAL_IPV6]) {
201 NL_SET_ERR_MSG_MOD(info->extack,
202"cannot specify local IPV6
address without remote");
203 return -EINVAL;
204 }
remote_family = get_family(attrs[OVPN_A_PEER_REMOTE_IPV4],
attrs[OVPN_A_PEER_REMOTE_IPV6]);
local_family = get_family(attrs[OVPN_A_PEER_LOCAL_IPV4],
attrs[OVPN_A_PEER_LOCAL_IPV6]);
if (remote_family != local_family) {
extack "incompatible address families";
return -EINVAL;
}
That would mirror the conversion that
ovpn_nl_attr_local_ip/ovpn_nl_attr_sockaddr_remote do.
Yeah, pretty much what I was suggested, but in a more explicit manner.
I like it.
int ovpn_nl_peer_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
{
[...]
+ } else {
+ rcu_read_lock();
+ hash_for_each_rcu(ovpn->peers->by_id, bkt, peer,
+ hash_entry_id) {
+ /* skip already dumped peers that were dumped by
+* previous invocations
+*/
+ if (last_idx > 0) {
+ last_idx--;
+ continue;
+ }
If a peer that was dumped during a previous invocation is removed in
between, we'll miss one that's still pr
Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink
2024-11-12, 15:19:50 +0100, Antonio Quartulli wrote:
> On 04/11/2024 16:14, Sabrina Dubroca wrote:
> > 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
> > > +static int ovpn_nl_peer_precheck(struct ovpn_struct *ovpn,
> > > + struct genl_info *info,
> > > + struct nlattr **attrs)
> > > +{
> > > + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs,
> > > + OVPN_A_PEER_ID))
> > > + return -EINVAL;
> > > +
> > > + if (attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6]) {
> > > + NL_SET_ERR_MSG_MOD(info->extack,
> > > +"cannot specify both remote IPv4 or IPv6
> > > address");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
> > > + !attrs[OVPN_A_PEER_REMOTE_IPV6] && attrs[OVPN_A_PEER_REMOTE_PORT]) {
> > > + NL_SET_ERR_MSG_MOD(info->extack,
> > > +"cannot specify remote port without IP
> > > address");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
> > > + attrs[OVPN_A_PEER_LOCAL_IPV4]) {
> > > + NL_SET_ERR_MSG_MOD(info->extack,
> > > +"cannot specify local IPv4 address without
> > > remote");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (!attrs[OVPN_A_PEER_REMOTE_IPV6] &&
> > > + attrs[OVPN_A_PEER_LOCAL_IPV6]) {
> >
> > I think these consistency checks should account for v4mapped
> > addresses. With remote=v4mapped and local=v6 we'll end up with an
> > incorrect ipv4 "local" address (taken out of the ipv6 address's first
> > 4B by ovpn_peer_reset_sockaddr). With remote=ipv6 and local=v4mapped,
> > we'll pass the last 4B of OVPN_A_PEER_LOCAL_IPV6 to
> > ovpn_peer_reset_sockaddr and try to read 16B (the full ipv6 address)
> > out of that.
>
> Right, a v4mapped address would fool this check.
> How about checking if both or none addresses are v4mapped? This way we
> should prevent such cases.
I don't know when userspace would use v4mapped addresses, but treating
a v4mapped address as a "proper" ipv4 address should work with the
rest of the code, since you already have the conversion in
ovpn_nl_attr_local_ip and ovpn_nl_attr_sockaddr_remote. So maybe you
could do something like (rough idea and completely untested):
static int get_family(attr_v4, attr_v6)
{
if (attr_v4)
return AF_INET;
if (attr_v6) {
if (ipv6_addr_v4mapped(attr_v6)
return AF_INET;
return AF_INET6;
}
return AF_UNSPEC;
}
// in _precheck:
// keep the attrs[OVPN_A_PEER_REMOTE_IPV4] &&
attrs[OVPN_A_PEER_REMOTE_IPV6] check
// maybe add a similar one for LOCAL_IPV4 && LOCAL_IPV6
remote_family = get_family(attrs[OVPN_A_PEER_REMOTE_IPV4],
attrs[OVPN_A_PEER_REMOTE_IPV6]);
local_family = get_family(attrs[OVPN_A_PEER_LOCAL_IPV4],
attrs[OVPN_A_PEER_LOCAL_IPV6]);
if (remote_family != local_family) {
extack "incompatible address families";
return -EINVAL;
}
That would mirror the conversion that
ovpn_nl_attr_local_ip/ovpn_nl_attr_sockaddr_remote do.
> > > int ovpn_nl_peer_get_dumpit(struct sk_buff *skb, struct
> > > netlink_callback *cb)
> > > {
> > [...]
> > > + } else {
> > > + rcu_read_lock();
> > > + hash_for_each_rcu(ovpn->peers->by_id, bkt, peer,
> > > + hash_entry_id) {
> > > + /* skip already dumped peers that were dumped by
> > > + * previous invocations
> > > + */
> > > + if (last_idx > 0) {
> > > + last_idx--;
> > > + continue;
> > > + }
> >
> > If a peer that was dumped during a previous invocation is removed in
> > between, we'll miss one that's still present in the overall dump. I
> > don't know how much it matters (I guses it depends on how the results
> > of this dump are used by userspace), so I'll let you decide if this
> > needs to be fixed immediately or if it can be ignored for now.
>
> True, this is a risk I assumed.
> Not extremely important if you ask me, but do you have any suggestion how to
> avoid this in an elegant and lockless way?
No, inconsistent dumps are an old problem with netlink, so I'm just
mentioning it as something to be aware of. You can add
genl_dump_check_consistent to let userspace know that it may have
gotten incorrect information (you'll need to keep a counter and
increment it when a peer is added/removed). On a very busy server you
may never manage to get a consistent dump, if peers are going up and
down very fast.
There's been some progress for dumping netdevices in commit
759ab1edb56c ("net: store netdevs in an xarray"), but that can still
return incorrect data.
--
Sabrina
Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink
2024-11-12, 15:26:59 +0100, Antonio Quartulli wrote: > On 11/11/2024 16:41, Sabrina Dubroca wrote: > > 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote: > > > +void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer) > > > + __must_hold(&peer->ovpn->peers->lock) > > > > Changes to peer->vpn_addrs are not protected by peers->lock, so those > > could be getting updated while we're rehashing (and taking peer->lock > > in ovpn_nl_peer_modify as I'm suggesting above also wouldn't prevent > > that). > > > > /me screams :-D Sorry :) > Indeed peers->lock is only about protecting the lists, not the content of > the listed objects. > > How about acquiring the peers->lock before calling ovpn_nl_peer_modify()? It seems like it would work. Maybe a bit weird to have conditional locking (MP mode only), but ok. You already have this lock ordering (hold peers->lock before taking peer->lock) in ovpn_peer_keepalive_work_mp, so there should be no deadlock from doing the same thing in the netlink code. Then I would also do that in ovpn_peer_float to protect that rehash. It feels like peers->lock is turning into a duplicate of ovpn->lock. ovpn->lock used for P2P mode, peers->lock used equivalently for MP mode. You might consider merging them (but I wouldn't see it as necessary for merging the series unless there's a locking issue with the current proposal). -- Sabrina
Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink
On 11/11/2024 16:41, Sabrina Dubroca wrote:
2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
+static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info,
+ struct nlattr **attrs)
+{
+ struct sockaddr_storage ss = {};
+ u32 sockfd, interv, timeout;
+ struct socket *sock = NULL;
+ u8 *local_ip = NULL;
+ bool rehash = false;
+ int ret;
+
+ if (attrs[OVPN_A_PEER_SOCKET]) {
+ /* lookup the fd in the kernel table and extract the socket
+* object
+*/
+ sockfd = nla_get_u32(attrs[OVPN_A_PEER_SOCKET]);
+ /* sockfd_lookup() increases sock's refcounter */
+ sock = sockfd_lookup(sockfd, &ret);
+ if (!sock) {
+ NL_SET_ERR_MSG_FMT_MOD(info->extack,
+ "cannot lookup peer socket (fd=%u):
%d",
+ sockfd, ret);
+ return -ENOTSOCK;
+ }
+
+ /* Only when using UDP as transport protocol the remote endpoint
+* can be configured so that ovpn knows where to send packets
+* to.
+*
+* In case of TCP, the socket is connected to the peer and ovpn
+* will just send bytes over it, without the need to specify a
+* destination.
+*/
+ if (sock->sk->sk_protocol != IPPROTO_UDP &&
+ (attrs[OVPN_A_PEER_REMOTE_IPV4] ||
+attrs[OVPN_A_PEER_REMOTE_IPV6])) {
+ NL_SET_ERR_MSG_FMT_MOD(info->extack,
+ "unexpected remote IP address for non
UDP socket");
+ sockfd_put(sock);
+ return -EINVAL;
+ }
+
+ if (peer->sock)
+ ovpn_socket_put(peer->sock);
+
+ peer->sock = ovpn_socket_new(sock, peer);
I don't see anything preventing concurrent updates of peer->sock. I
think peer->lock should be taken from the start of
ovpn_nl_peer_modify. Concurrent changes to peer->vpn_addrs and
peer->keepalive_* are also not prevented with the current code.
Yeah, this came up to my mind as well when checking the keepalive worker
code.
I'll make sure all updates happen under lock.
+ if (IS_ERR(peer->sock)) {
+ NL_SET_ERR_MSG_FMT_MOD(info->extack,
+ "cannot encapsulate socket: %ld",
+ PTR_ERR(peer->sock));
+ sockfd_put(sock);
+ peer->sock = NULL;
+ return -ENOTSOCK;
+ }
+ }
+
+ if (ovpn_nl_attr_sockaddr_remote(attrs, &ss) != AF_UNSPEC) {
+ /* we carry the local IP in a generic container.
+* ovpn_peer_reset_sockaddr() will properly interpret it
+* based on ss.ss_family
+*/
+ local_ip = ovpn_nl_attr_local_ip(attrs);
+
+ spin_lock_bh(&peer->lock);
+ /* set peer sockaddr */
+ ret = ovpn_peer_reset_sockaddr(peer, &ss, local_ip);
+ if (ret < 0) {
+ NL_SET_ERR_MSG_FMT_MOD(info->extack,
+ "cannot set peer sockaddr: %d",
+ ret);
+ spin_unlock_bh(&peer->lock);
+ return ret;
+ }
+ spin_unlock_bh(&peer->lock);
+ }
+
+ if (attrs[OVPN_A_PEER_VPN_IPV4]) {
+ rehash = true;
+ peer->vpn_addrs.ipv4.s_addr =
+ nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]);
+ }
+
+ if (attrs[OVPN_A_PEER_VPN_IPV6]) {
+ rehash = true;
+ peer->vpn_addrs.ipv6 =
+ nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]);
+ }
+
+ /* when setting the keepalive, both parameters have to be configured */
+ if (attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] &&
+ attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]) {
+ interv = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL]);
+ timeout = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]);
+ ovpn_peer_keepalive_set(peer, interv, timeout);
+ }
+
+ netdev_dbg(peer->ovpn->dev,
+ "%s: peer id=%u endpoint=%pIScp/%s VPN-IPv4=%pI4
VPN-IPv6=%pI6c\n",
+ __func__, peer->id, &ss,
+ peer->sock->sock->sk->sk_prot_creator->name,
+ &peer->vpn_addrs.ipv4.s_addr, &peer->vpn_addrs.ipv6);
+
+ return rehash ? 1 : 0;
+}
+
[...]
+void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer)
+ __must_hold(&peer->ovpn->peers->lock)
Changes
Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink
On 04/11/2024 16:14, Sabrina Dubroca wrote:
2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
+static int ovpn_nl_peer_precheck(struct ovpn_struct *ovpn,
+struct genl_info *info,
+struct nlattr **attrs)
+{
+ if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs,
+ OVPN_A_PEER_ID))
+ return -EINVAL;
+
+ if (attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6]) {
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "cannot specify both remote IPv4 or IPv6
address");
+ return -EINVAL;
+ }
+
+ if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
+ !attrs[OVPN_A_PEER_REMOTE_IPV6] && attrs[OVPN_A_PEER_REMOTE_PORT]) {
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "cannot specify remote port without IP
address");
+ return -EINVAL;
+ }
+
+ if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
+ attrs[OVPN_A_PEER_LOCAL_IPV4]) {
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "cannot specify local IPv4 address without
remote");
+ return -EINVAL;
+ }
+
+ if (!attrs[OVPN_A_PEER_REMOTE_IPV6] &&
+ attrs[OVPN_A_PEER_LOCAL_IPV6]) {
I think these consistency checks should account for v4mapped
addresses. With remote=v4mapped and local=v6 we'll end up with an
incorrect ipv4 "local" address (taken out of the ipv6 address's first
4B by ovpn_peer_reset_sockaddr). With remote=ipv6 and local=v4mapped,
we'll pass the last 4B of OVPN_A_PEER_LOCAL_IPV6 to
ovpn_peer_reset_sockaddr and try to read 16B (the full ipv6 address)
out of that.
Right, a v4mapped address would fool this check.
How about checking if both or none addresses are v4mapped? This way we
should prevent such cases.
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "cannot specify local IPV6 address without
remote");
+ return -EINVAL;
+ }
[...]
int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info)
{
[...]
+ ret = ovpn_nl_peer_modify(peer, info, attrs);
+ if (ret < 0) {
+ ovpn_peer_put(peer);
+ return ret;
+ }
+
+ /* ret == 1 means that VPN IPv4/6 has been modified and rehashing
+* is required
+*/
+ if (ret > 0) {
&& mode == MP ?
I don't see ovpn_nl_peer_modify checking that before returning 1, and
in P2P mode ovpn->peers will be NULL.
Right.
I was wondering if it's better to add the check on the return statement
of ovpn_nl_peer_modify...but I think it's more functional to add it
here, as per your suggestion.
+ spin_lock_bh(&ovpn->peers->lock);
+ ovpn_peer_hash_vpn_ip(peer);
+ spin_unlock_bh(&ovpn->peers->lock);
+ }
+
+ ovpn_peer_put(peer);
+
+ return 0;
+}
int ovpn_nl_peer_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
{
[...]
+ } else {
+ rcu_read_lock();
+ hash_for_each_rcu(ovpn->peers->by_id, bkt, peer,
+ hash_entry_id) {
+ /* skip already dumped peers that were dumped by
+* previous invocations
+*/
+ if (last_idx > 0) {
+ last_idx--;
+ continue;
+ }
If a peer that was dumped during a previous invocation is removed in
between, we'll miss one that's still present in the overall dump. I
don't know how much it matters (I guses it depends on how the results
of this dump are used by userspace), so I'll let you decide if this
needs to be fixed immediately or if it can be ignored for now.
True, this is a risk I assumed.
Not extremely important if you ask me, but do you have any suggestion
how to avoid this in an elegant and lockless way?
IIRC I got inspired by the station dump in the mac80211 code, which
probably assumes the same risk.
+
+ if (ovpn_nl_send_peer(skb, info, peer,
+ NETLINK_CB(cb->skb).portid,
+ cb->nlh->nlmsg_seq,
+ NLM_F_MULTI) < 0)
+ break;
+
+ /* count peers being dumped during this invocation */
+ dumped++;
+ }
+ rcu_read_unlock();
+ }
+
+out:
+ netdev_put(ovpn->dev, &ovpn->dev_tracker);
+
+ /* sum up peers dumped in this message, so that at the next invocation
+* we can continue from where we left
+*/
+ cb->args[1] += dumped;
+ return skb->len;
}
int ovpn_nl_peer_del_doit(struct sk_buff *skb, struct genl_info *info)
Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink
2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
> +static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info
> *info,
> +struct nlattr **attrs)
> +{
> + struct sockaddr_storage ss = {};
> + u32 sockfd, interv, timeout;
> + struct socket *sock = NULL;
> + u8 *local_ip = NULL;
> + bool rehash = false;
> + int ret;
> +
> + if (attrs[OVPN_A_PEER_SOCKET]) {
> + /* lookup the fd in the kernel table and extract the socket
> + * object
> + */
> + sockfd = nla_get_u32(attrs[OVPN_A_PEER_SOCKET]);
> + /* sockfd_lookup() increases sock's refcounter */
> + sock = sockfd_lookup(sockfd, &ret);
> + if (!sock) {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +"cannot lookup peer socket
> (fd=%u): %d",
> +sockfd, ret);
> + return -ENOTSOCK;
> + }
> +
> + /* Only when using UDP as transport protocol the remote endpoint
> + * can be configured so that ovpn knows where to send packets
> + * to.
> + *
> + * In case of TCP, the socket is connected to the peer and ovpn
> + * will just send bytes over it, without the need to specify a
> + * destination.
> + */
> + if (sock->sk->sk_protocol != IPPROTO_UDP &&
> + (attrs[OVPN_A_PEER_REMOTE_IPV4] ||
> + attrs[OVPN_A_PEER_REMOTE_IPV6])) {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +"unexpected remote IP address
> for non UDP socket");
> + sockfd_put(sock);
> + return -EINVAL;
> + }
> +
> + if (peer->sock)
> + ovpn_socket_put(peer->sock);
> +
> + peer->sock = ovpn_socket_new(sock, peer);
I don't see anything preventing concurrent updates of peer->sock. I
think peer->lock should be taken from the start of
ovpn_nl_peer_modify. Concurrent changes to peer->vpn_addrs and
peer->keepalive_* are also not prevented with the current code.
> + if (IS_ERR(peer->sock)) {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +"cannot encapsulate socket: %ld",
> +PTR_ERR(peer->sock));
> + sockfd_put(sock);
> + peer->sock = NULL;
> + return -ENOTSOCK;
> + }
> + }
> +
> + if (ovpn_nl_attr_sockaddr_remote(attrs, &ss) != AF_UNSPEC) {
> + /* we carry the local IP in a generic container.
> + * ovpn_peer_reset_sockaddr() will properly interpret it
> + * based on ss.ss_family
> + */
> + local_ip = ovpn_nl_attr_local_ip(attrs);
> +
> + spin_lock_bh(&peer->lock);
> + /* set peer sockaddr */
> + ret = ovpn_peer_reset_sockaddr(peer, &ss, local_ip);
> + if (ret < 0) {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +"cannot set peer sockaddr: %d",
> +ret);
> + spin_unlock_bh(&peer->lock);
> + return ret;
> + }
> + spin_unlock_bh(&peer->lock);
> + }
> +
> + if (attrs[OVPN_A_PEER_VPN_IPV4]) {
> + rehash = true;
> + peer->vpn_addrs.ipv4.s_addr =
> + nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]);
> + }
> +
> + if (attrs[OVPN_A_PEER_VPN_IPV6]) {
> + rehash = true;
> + peer->vpn_addrs.ipv6 =
> + nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]);
> + }
> +
> + /* when setting the keepalive, both parameters have to be configured */
> + if (attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] &&
> + attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]) {
> + interv = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL]);
> + timeout = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]);
> + ovpn_peer_keepalive_set(peer, interv, timeout);
> + }
> +
> + netdev_dbg(peer->ovpn->dev,
> +"%s: peer id=%u endpoint=%pIScp/%s VPN-IPv4=%pI4
> VPN-IPv6=%pI6c\n",
> +__func__, peer->id, &ss,
> +peer->sock->sock->sk->sk_prot_creator->name,
> +&peer->vpn_addrs.ipv4.s_addr, &peer->vpn_addrs.ipv6);
> +
> + return rehash ? 1 : 0;
> +}
> +
[...]
> +void ovpn_peer_hash_vpn_ip(struct ovpn_peer *peer)
> + __must_hold(&peer->ovpn->peers->lock)
Changes to peer->vpn_addrs are not protected by peers->lock, so those
could be getting updated while we're rehashing (and taking peer->lock
in ovpn_nl
Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink
2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
> +static int ovpn_nl_peer_precheck(struct ovpn_struct *ovpn,
> + struct genl_info *info,
> + struct nlattr **attrs)
> +{
> + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs,
> + OVPN_A_PEER_ID))
> + return -EINVAL;
> +
> + if (attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6]) {
> + NL_SET_ERR_MSG_MOD(info->extack,
> +"cannot specify both remote IPv4 or IPv6
> address");
> + return -EINVAL;
> + }
> +
> + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
> + !attrs[OVPN_A_PEER_REMOTE_IPV6] && attrs[OVPN_A_PEER_REMOTE_PORT]) {
> + NL_SET_ERR_MSG_MOD(info->extack,
> +"cannot specify remote port without IP
> address");
> + return -EINVAL;
> + }
> +
> + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
> + attrs[OVPN_A_PEER_LOCAL_IPV4]) {
> + NL_SET_ERR_MSG_MOD(info->extack,
> +"cannot specify local IPv4 address without
> remote");
> + return -EINVAL;
> + }
> +
> + if (!attrs[OVPN_A_PEER_REMOTE_IPV6] &&
> + attrs[OVPN_A_PEER_LOCAL_IPV6]) {
I think these consistency checks should account for v4mapped
addresses. With remote=v4mapped and local=v6 we'll end up with an
incorrect ipv4 "local" address (taken out of the ipv6 address's first
4B by ovpn_peer_reset_sockaddr). With remote=ipv6 and local=v4mapped,
we'll pass the last 4B of OVPN_A_PEER_LOCAL_IPV6 to
ovpn_peer_reset_sockaddr and try to read 16B (the full ipv6 address)
out of that.
> + NL_SET_ERR_MSG_MOD(info->extack,
> +"cannot specify local IPV6 address without
> remote");
> + return -EINVAL;
> + }
[...]
> int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info)
> {
[...]
> + ret = ovpn_nl_peer_modify(peer, info, attrs);
> + if (ret < 0) {
> + ovpn_peer_put(peer);
> + return ret;
> + }
> +
> + /* ret == 1 means that VPN IPv4/6 has been modified and rehashing
> + * is required
> + */
> + if (ret > 0) {
&& mode == MP ?
I don't see ovpn_nl_peer_modify checking that before returning 1, and
in P2P mode ovpn->peers will be NULL.
> + spin_lock_bh(&ovpn->peers->lock);
> + ovpn_peer_hash_vpn_ip(peer);
> + spin_unlock_bh(&ovpn->peers->lock);
> + }
> +
> + ovpn_peer_put(peer);
> +
> + return 0;
> +}
> int ovpn_nl_peer_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> {
[...]
> + } else {
> + rcu_read_lock();
> + hash_for_each_rcu(ovpn->peers->by_id, bkt, peer,
> + hash_entry_id) {
> + /* skip already dumped peers that were dumped by
> + * previous invocations
> + */
> + if (last_idx > 0) {
> + last_idx--;
> + continue;
> + }
If a peer that was dumped during a previous invocation is removed in
between, we'll miss one that's still present in the overall dump. I
don't know how much it matters (I guses it depends on how the results
of this dump are used by userspace), so I'll let you decide if this
needs to be fixed immediately or if it can be ignored for now.
> +
> + if (ovpn_nl_send_peer(skb, info, peer,
> + NETLINK_CB(cb->skb).portid,
> + cb->nlh->nlmsg_seq,
> + NLM_F_MULTI) < 0)
> + break;
> +
> + /* count peers being dumped during this invocation */
> + dumped++;
> + }
> + rcu_read_unlock();
> + }
> +
> +out:
> + netdev_put(ovpn->dev, &ovpn->dev_tracker);
> +
> + /* sum up peers dumped in this message, so that at the next invocation
> + * we can continue from where we left
> + */
> + cb->args[1] += dumped;
> + return skb->len;
> }
>
> int ovpn_nl_peer_del_doit(struct sk_buff *skb, struct genl_info *info)
> {
> - return -EOPNOTSUPP;
> + struct nlattr *attrs[OVPN_A_PEER_MAX + 1];
> + struct ovpn_struct *ovpn = info->user_ptr[0];
> + struct ovpn_peer *peer;
> + u32 peer_id;
> + int ret;
> +
> + if (GENL_REQ_ATTR_CHECK(info, OVPN_A_PEER))
> + return -EINVAL;
> +
> + ret = nla_parse_nested(attrs, OVPN_A_PEER_MAX, info->attrs[OVPN_A_PEER],
> +ovpn_peer_nl_policy, info->extack);
> + if (ret)
> + return ret;
> +
> + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs,
> + OVPN_A_PEE
