Re: [PATCH net-next v5 08/25] ovpn: introduce the ovpn_peer object

On 03/07/2024 23:37, Sabrina Dubroca wrote:

2024-06-27, 15:08:26 +0200, Antonio Quartulli wrote:

quoted

+/**
+ * struct ovpn_sockaddr - basic transport layer address
+ * @in4: IPv4 address
+ * @in6: IPv6 address
+ */
+struct ovpn_sockaddr {
+	union {
+		struct sockaddr_in in4;
+		struct sockaddr_in6 in6;
+	};
+};

nit: wrapping the anonymous union in a struct that contains nothing
else is not that useful.

yeah, I guess I can just turn ovpn_sockaddr in a union.

quoted

+/**
+ * struct ovpn_bind - remote peer binding
+ * @sa: the remote peer sockaddress
+ * @local: local endpoint used to talk to the peer
+ * @local.ipv4: local IPv4 used to talk to the peer
+ * @local.ipv6: local IPv6 used to talk to the peer
+ * @rcu: used to schedule RCU cleanup job
+ */
+struct ovpn_bind {
+	struct ovpn_sockaddr sa;  /* remote sockaddr */

nit: then maybe call it "peer" or "remote" instead of sa?

yap, makes sense. I will call it "remote".

quoted

+	union {
+		struct in_addr ipv4;
+		struct in6_addr ipv6;
+	} local;
+
+	struct rcu_head rcu;
+};
+

[...]

quoted

+struct ovpn_peer *ovpn_peer_new(struct ovpn_struct *ovpn, u32 id)
+{
+	struct ovpn_peer *peer;
+	int ret;
+
+	/* alloc and init peer object */
+	peer = kzalloc(sizeof(*peer), GFP_KERNEL);
+	if (!peer)
+		return ERR_PTR(-ENOMEM);
+
+	peer->id = id;
+	peer->halt = false;
+	peer->ovpn = ovpn;
+
+	peer->vpn_addrs.ipv4.s_addr = htonl(INADDR_ANY);
+	peer->vpn_addrs.ipv6 = in6addr_any;
+
+	RCU_INIT_POINTER(peer->bind, NULL);
+	spin_lock_init(&peer->lock);
+	kref_init(&peer->refcount);
+
+	ret = dst_cache_init(&peer->dst_cache, GFP_KERNEL);
+	if (ret < 0) {
+		netdev_err(ovpn->dev, "%s: cannot initialize dst cache\n",
+			   __func__);
+		kfree(peer);
+		return ERR_PTR(ret);
+	}
+
+	netdev_hold(ovpn->dev, NULL, GFP_KERNEL);

It would be good to add a tracker to help debug refcount issues.

Ok, will do!

quoted

+
+	return peer;
+}
+
+#define ovpn_peer_index(_tbl, _key, _key_len)		\
+	(jhash(_key, _key_len, 0) % HASH_SIZE(_tbl))	\

nit: not used in this patch, and even removed by patch 16 as you
convert from index to buckets (that conversion should be squashed into
patch 15)

You're correct. Will merge all these pieces in patch 15.

quoted

+/**
+ * ovpn_peer_transp_match - check if sockaddr and peer binding match
+ * @peer: the peer to get the binding from
+ * @ss: the sockaddr to match
+ *
+ * Return: true if sockaddr and binding match or false otherwise
+ */
+static bool ovpn_peer_transp_match(const struct ovpn_peer *peer,
+				   const struct sockaddr_storage *ss)
+{

AFAICT ovpn_peer_transp_match is only called with ss from
ovpn_peer_skb_to_sockaddr, so it's pretty much ovpn_bind_skb_src_match
but using peer->bind. You can probably avoid the code duplication
(ovpn_peer_transp_match and ovpn_bind_skb_src_match are very similar).

mhh it is not called in ovpn_peer_skb_to_sockaddr, but I guess your 
comment still applies: ovpn_peer_transp_match and 
ovpn_bind_skb_src_match are very similar.

However in one we have a sockaddr_storage while in the other we have an 
skb. How do we combine the two?
The only way I see is to create an ss out of the skb and then always use
ovpn_peer_transp_match. Is this what you were alluding to?


Thanks!

-- 
Antonio Quartulli
OpenVPN Inc.