2024-05-06, 03:16:20 +0200, Antonio Quartulli wrote:

An ovpn_peer object holds the whole status of a remote peer
(regardless whether it is a server or a client).

This includes status for crypto, tx/rx buffers, napi, etc.

Only support for one peer is introduced (P2P mode).
Multi peer support is introduced with a later patch.

Along with the ovpn_peer, also the ovpn_bind object is introcued

                                                         ^
typo: "introduced"

as the two are strictly related.
An ovpn_bind object wraps a sockaddr representing the local
coordinates being used to talk to a specific peer.

Isn't that just kfree_rcu? (note kfree_rcu doesn't need the NULL check)

+}

[...]

+static inline bool ovpn_bind_skb_src_match(const struct ovpn_bind *bind,
+					   struct sk_buff *skb)

nit: I think skb can also be const here

I'm not convinced this will get freed consistently if
register_netdevice fails early (before ndo_init).  After talking to
Paolo, it seems this should be moved into a new ->ndo_init instead.

Is the flush needed? I'm not an expert on workqueues, but from a quick
look at destroy_workqueue it calls drain_workqueue, which would take
care of flushing the queue?

+	rcu_barrier();
 }
 

[...]

nit: the comment isn't really needed since you have kdoc saying the same thing

[...]

+/**
+ * ovpn_peer_free - release private members and free peer object
+ * @peer: the peer to free
+ */
+static void ovpn_peer_free(struct ovpn_peer *peer)
+{
+	ovpn_bind_reset(peer, NULL);
+
+	WARN_ON(!__ptr_ring_empty(&peer->tx_ring));

Could you pass a destructor to ptr_ring_cleanup instead of all these WARNs?

+	ptr_ring_cleanup(&peer->tx_ring, NULL);
+	WARN_ON(!__ptr_ring_empty(&peer->rx_ring));
+	ptr_ring_cleanup(&peer->rx_ring, NULL);
+	WARN_ON(!__ptr_ring_empty(&peer->netif_rx_ring));
+	ptr_ring_cleanup(&peer->netif_rx_ring, NULL);
+
+	dst_cache_destroy(&peer->dst_cache);
+
+	dev_put(peer->ovpn->dev);
+
+	kfree(peer);
+}

[...]

+void ovpn_peer_release(struct ovpn_peer *peer)
+{
+	call_rcu(&peer->rcu, ovpn_peer_release_rcu);
+}
+
+/**
+ * ovpn_peer_delete_work - work scheduled to release peer in process context
+ * @work: the work object
+ */
+static void ovpn_peer_delete_work(struct work_struct *work)
+{
+	struct ovpn_peer *peer = container_of(work, struct ovpn_peer,
+					      delete_work);
+	ovpn_peer_release(peer);

Does call_rcu really need to run in process context?

+}

[...]

+/**
+ * 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(struct ovpn_peer *peer,
+				   struct sockaddr_storage *ss)
+{

[...]

+	case AF_INET6:
+		sa6 = (struct sockaddr_in6 *)ss;
+		if (memcmp(&sa6->sin6_addr, &bind->sa.in6.sin6_addr,
+			   sizeof(struct in6_addr)))

ipv6_addr_equal?

+			return false;
+		if (sa6->sin6_port != bind->sa.in6.sin6_port)
+			return false;
+		break;

[...]

+struct ovpn_peer *ovpn_peer_get_by_id(struct ovpn_struct *ovpn, u32 peer_id)
+{
+	struct ovpn_peer *peer = NULL;
+
+	if (ovpn->mode == OVPN_MODE_P2P)
+		peer = ovpn_peer_get_by_id_p2p(ovpn, peer_id);
+
+	return peer;
+}
+
+/**
+ * ovpn_peer_add_p2p - add per to related tables in a P2P instance

                              ^
typo: peer?


[...]

+/**
+ * ovpn_peer_del_p2p - delete peer from related tables in a P2P instance
+ * @peer: the peer to delete
+ * @reason: reason why the peer was deleted (sent to userspace)
+ *
+ * Return: 0 on success or a negative error code otherwise
+ */
+static int ovpn_peer_del_p2p(struct ovpn_peer *peer,
+			     enum ovpn_del_peer_reason reason)
+{
+	struct ovpn_peer *tmp;
+	int ret = -ENOENT;
+
+	spin_lock_bh(&peer->ovpn->lock);
+	tmp = rcu_dereference(peer->ovpn->peer);
+	if (tmp != peer)
+		goto unlock;

How do we recover if all those objects got out of sync? Are we stuck
with a broken peer?

And if this happens during interface deletion, aren't we leaking the
peer memory here?

+	ovpn_peer_put(tmp);
+	tmp->delete_reason = reason;
+	RCU_INIT_POINTER(peer->ovpn->peer, NULL);
+	ret = 0;
+
+unlock:
+	spin_unlock_bh(&peer->ovpn->lock);
+
+	return ret;
+}

[...]

[...]

+/**
+ * struct ovpn_peer - the main remote peer object
+ * @ovpn: main openvpn instance this peer belongs to
+ * @id: unique identifier
+ * @vpn_addrs.ipv4: IPv4 assigned to peer on the tunnel
+ * @vpn_addrs.ipv6: IPv6 assigned to peer on the tunnel
+ * @tx_ring: queue of outgoing poackets to this peer
+ * @rx_ring: queue of incoming packets from this peer
+ * @netif_rx_ring: queue of packets to be sent to the netdevice via NAPI
+ * @dst_cache: cache for dst_entry used to send to peer
+ * @bind: remote peer binding
+ * @halt: true if ovpn_peer_mark_delete was called
+ * @delete_reason: why peer was deleted (i.e. timeout, transport error, ..)
+ * @lock: protects binding to peer (bind)
+ * @refcount: reference counter
+ * @rcu: used to free peer in an RCU safe way
+ * @delete_work: deferred cleanup work, used to notify userspace
+ */
+struct ovpn_peer {
+	struct ovpn_struct *ovpn;
+	u32 id;
+	struct {
+		struct in_addr ipv4;
+		struct in6_addr ipv6;
+	} vpn_addrs;
+	struct ptr_ring tx_ring;
+	struct ptr_ring rx_ring;
+	struct ptr_ring netif_rx_ring;
+	struct dst_cache dst_cache;
+	struct ovpn_bind __rcu *bind;
+	bool halt;
+	enum ovpn_del_peer_reason delete_reason;
+	spinlock_t lock; /* protects bind */

nit: the comment isn't really needed, it's redundant with kdoc.

+	struct kref refcount;
+	struct rcu_head rcu;
+	struct work_struct delete_work;
+};

-- 
Sabrina