Message-ID: <ZpflHPMK5tDlfXQw@hog>
Date: Wed, 17 Jul 2024 17:37:00 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.net>, kuba@...nel.org,
	pabeni@...hat.com, edumazet@...gle.com
Cc: netdev@...r.kernel.org, ryazanov.s.a@...il.com, andrew@...n.ch
Subject: Re: [PATCH net-next v5 20/25] ovpn: implement peer add/dump/delete
 via netlink

2024-07-17, 16:04:25 +0200, Antonio Quartulli wrote:
> On 16/07/2024 15:41, Sabrina Dubroca wrote:
> > 2024-06-27, 15:08:38 +0200, Antonio Quartulli wrote:
> > > +	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);
> > > +			ret = -ENOTSOCK;
> > > +			goto peer_release;
> > > +		}
> > > +
> > > +		if (peer->sock)
> > > +			ovpn_socket_put(peer->sock);
> > > +
> > > +		peer->sock = ovpn_socket_new(sock, peer);
> > > +		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;
> > 
> > Is there any value for the client in keeping the old peer->sock
> > assigned if we fail here?
> > 
> > ie something like:
> > 
> >      tmp = ovpn_socket_new(sock, peer);
> >      if (IS_ERR(tmp)) {
> >          ...
> >          goto peer_release;
> >      }
> >      if (peer->sock)
> >          ovpn_socket_put(peer->sock);
> >      peer->sock = tmp;
> > 
> > 
> > But if it's just going to get rid of the old socket and the whole
> > association/peer on failure, probably not.
> 
> Right. if attaching the new socket fails, we are entering some broken status
> which is not worth keeping around.

Ok, then the current code is fine, thanks.


> > > +	/* 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.
> > 
> > (that should also work with UDP "connected" sockets)
> 
> True, but those are not used in openvpn. In case of UDP, userspace just
> creates one socket and uses it for all peers.
> I will add a note about 'connected UDP socket' in the comment, to clear this
> out.

If you want. I was being pedantic, I don't think it's really necessary
to mention this.


> > > +				goto peer_release;
> > > +			}
> > > +		}
> > > +
> > > +		/* 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);
> > > +			goto peer_release;
> > > +		}
> > > +	}
> > 
> > I would reject OVPN_A_PEER_SOCKADDR_REMOTE for a non-UDP socket.
> 
> judging from the comments below, it seems you prefer to reject unneeded
> attributes. OTOH I took the opposite approach (just ignore those).

Yes.

> However, I was actually looking for some preference/indication regarding
> this point and I now I got one :-)

I don't think there's an established rule, and a lot of the old code
is very tolerant. That's my preference (in part because I think
refusing bogus combinations allows to enable them in the future with a
new behavior), but maybe the maintainers have a different opinion?
OTOH ignoring those attributes can let a modern client run on an old
kernel (possibly without some features, depending on what the
attribute is).

(leaving a few other examples of stricter validation for context:)

> I will be strict and return -EINVAL when unneded attributes are present.
> 
> > 
> > 
> > > +	/* VPN IPs cannot be updated, because they are hashed */
> > 
> > Then I think there should be something like
> > 
> >      if (!new_peer && (attrs[OVPN_A_PEER_VPN_IPV4] || attrs[OVPN_A_PEER_VPN_IPV6])) {
> >          NL_SET_ERR_MSG_FMT_MOD(... "can't update ip");
> >          ret = -EINVAL;
> >          goto peer_release;
> >      }
> > 
> > (just after getting the peer, before any changes have actually been
> > made)
> 
> ACK
> 
> > 
> > And if they are only used in MP mode, I would maybe also reject
> > requests where mode==P2P and OVPN_A_PEER_VPN_IPV* is provided.
> 
> yup, like I commented above.
> 
> > 
> > 
> > > +	if (new_peer && attrs[OVPN_A_PEER_VPN_IPV4])
> > > +		peer->vpn_addrs.ipv4.s_addr =
> > > +			nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]);
> > > +
> > > +	/* VPN IPs cannot be updated, because they are hashed */
> > > +	if (new_peer && attrs[OVPN_A_PEER_VPN_IPV6])
> > > +		peer->vpn_addrs.ipv6 =
> > > +			nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]);
> > > +
> > > +	/* when setting the keepalive, both parameters have to be configured */
> > 
> > Then I would also reject a config where only one is set (also before any
> > changes have been made).
> 
> ok


[...]
> > > +	if (attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] &&
> > > +	    attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]) {
> > > +		keepalive_set = true;
> > > +		interv = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL]);
> > > +		timeout = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]);
> > > +	}
> > > +
> > > +	if (keepalive_set)
> > > +		ovpn_peer_keepalive_set(peer, interv, timeout);
> > 
> > Why not skip the bool and just do this in the previous block?
> 
> I am pretty sure there was a reason...but it may have faded away after the
> 95-th rebase hehe. Thanks for spotting this!

:)

> 
> > 
> > > +	netdev_dbg(ovpn->dev,
> > > +		   "%s: %s peer with endpoint=%pIScp/%s id=%u VPN-IPv4=%pI4 VPN-IPv6=%pI6c\n",
> > > +		   __func__, (new_peer ? "adding" : "modifying"), ss,
> > > +		   peer->sock->sock->sk->sk_prot_creator->name, peer->id,
> > > +		   &peer->vpn_addrs.ipv4.s_addr, &peer->vpn_addrs.ipv6);
> > > +
> > > +	if (new_peer) {
> > > +		ret = ovpn_peer_add(ovpn, peer);
> > > +		if (ret < 0) {
> > > +			NL_SET_ERR_MSG_FMT_MOD(info->extack,
> > > +					       "cannot add new peer (id=%u) to hashtable: %d\n",
> > > +					       peer->id, ret);
> > > +			goto peer_release;
> > > +		}
> > > +	} else {
> > > +		ovpn_peer_put(peer);
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +peer_release:
> > > +	if (new_peer) {
> > > +		/* release right away because peer is not really used in any
> > > +		 * context
> > > +		 */
> > > +		ovpn_peer_release(peer);
> > > +		kfree(peer);
> > 
> > I don't think that's correct, the new peer was created with
> > ovpn_peer_new, so it took a reference on the netdevice
> > (netdev_hold(ovpn->dev, ...)), which isn't released by
> > ovpn_peer_release. Why not just go through ovpn_peer_put?
> 
> Because then we would send the notification to userspace, but it is not
> correct to do so, because the new() is just about to return an error.

Oh, right.

> I presume I should just move netdev_put(peer->ovpn->dev, NULL); to
> ovpn_peer_release(). That will take care of this case too.

Ok.


-- 
Sabrina

Powered by blists - more mailing lists