Re: [PATCH v3 2/9] rv: add generic uprobe infrastructure for RV monitors

[Gabriele Monaco <gmonaco@redhat.com>]

On Mon, 2026-06-08 at 00:13 +0800, wen.yang@linux.dev wrote:
> From: Wen Yang <wen.yang@linux.dev>
> 
> Introduce rv_uprobe, a thin wrapper around uprobe_consumer providing
> rv_uprobe_attach_path(), rv_uprobe_attach(), and rv_uprobe_detach()
> for RV monitors.  An opaque priv pointer is forwarded unchanged to
> entry/return handlers so monitors can carry per-binding state (e.g. a
> latency threshold) to the hot path without any global lookup.
> 
> rv_uprobe_detach() is fully synchronous (nosync + sync + path_put +
> kfree), closing the use-after-free window present in open-coded
> patterns where kfree() precedes uprobe_unregister_sync().
> 
> Suggested-by: Gabriele Monaco <gmonaco@redhat.com>
> Signed-off-by: Wen Yang <wen.yang@linux.dev>

I find your implementation solid, but I wonder if it isn't a bit
overdoing it. It's good to have helper functions abstracting away the
common uprobes code, but I find the number of structures used to indirect/hide
the process a bit confusing.

Users of this headers still know they're using uprobes, so we don't
really need to hide it that much, plus the double function pointers are
an additional performance hit we could easily skip.

As I get it, we want to pass a private pointer (essentially to get the
threshold in tlob) and the uprobe infrastructure doesn't allow that
transparently. So let's keep on using struct embedding but a bit more
cut to the bone:

	struct rv_uprobe {
		struct uprobe_consumer	uc;
		struct uprobe		*uprobe;
		void			*priv;
	};

Then handlers would use container_of() appropriately to derive the data they
need.

	static int tlob_uprobe_entry_handler(struct uprobe_consumer *self, struct pt_regs *regs, __u64 *data)
	{
		struct rv_uprobe *p = container_of(self, struct rv_uprobe, uc);
		struct tlob_uprobe_binding *b = p->priv;

		...
	}

You could even define the struct directly inside your private data and save one
step (and no void *):

	struct rv_uprobe {
		struct uprobe_consumer	uc;
		struct uprobe		*uprobe;
	};

	#define DECLARE_RV_UPROBE(name) struct rv_uprobe name

	// And then

	struct tlob_uprobe_binding {
		u64 threshold_ns;
		...
		DECLARE_RV_UPROBE(start);
		DECLARE_RV_UPROBE(stop);
	}

	static int tlob_uprobe_entry_handler(struct uprobe_consumer *self, struct pt_regs *regs, __u64 *data)
	{
		struct tlob_uprobe_binding *b = container_of(self, struct tlob_uprobe_binding, start.uc);

		...
	}


Now I intentionally skipped offset, as I don't really see why carry it around,
and path, as we could live without it by relying on inodes (uprobes
reference-track them but you'd need to save them somewhere).

Mind, I only build tested this idea (appended after). This was supported by an
LLM which did a few more changes like standardising names and return values of
registration functions (a bit more consistent with other stuff, you don't have
to commit with that though).

By the way, do we really need the duplicate check? Would it break if a user
defines it twice? If not, we could simplify the tlob_add_uprobe() even further
and doing all path related checks in the library, since those aren't quite tlob
specific.

Thoughts?

Thanks,
Gabriele

**Suggested simplification:**

---
diff --git a/include/rv/rv_uprobe.h b/include/rv/rv_uprobe.h
index 9106c5c927..4fb3f50a63 100644
--- a/include/rv/rv_uprobe.h
+++ b/include/rv/rv_uprobe.h
@@ -7,83 +7,56 @@
 #ifndef _RV_UPROBE_H
 #define _RV_UPROBE_H
 
-#include <linux/path.h>
 #include <linux/types.h>
+#include <linux/uprobes.h>
 
 struct pt_regs;
+struct inode;
 
 /**
  * struct rv_uprobe - a single uprobe registered on behalf of an RV monitor
  *
- * @offset:   byte offset within the ELF binary where the probe is installed
- * @priv:     monitor-private pointer; set at attach time, never touched by
- *            this layer; passed unchanged to entry_fn / ret_fn
- * @path:     resolved path of the probed binary (read-only after attach);
- *            callers may use path.dentry for identity comparisons
- *
- * The implementation fields (uprobe_consumer, uprobe handle, callbacks) are
- * private to rv_uprobe.c and are not exposed here; monitors must not access
- * them directly.
+ * @uc:       underlying uprobe consumer (publicly visible)
+ * @uprobe:   active uprobe structure handle
+ * @inode:    inode of the target binary (read-only after registration)
  */
 struct rv_uprobe {
-	/* public: read-only after rv_uprobe_attach*() */
-	loff_t		 offset;
-	void		*priv;
-	struct path	 path;
+	struct uprobe_consumer	uc;
+	struct uprobe		*uprobe;
+	struct inode		*inode;
 };
 
-/**
- * rv_uprobe_attach_path - register an uprobe given an already-resolved path
- * @path:     path of the target binary; rv_uprobe takes its own reference
- * @offset:   byte offset within the binary
- * @entry_fn: called on probe hit (entry); may be NULL
- * @ret_fn:   called on function return (uretprobe); may be NULL
- * @priv:     opaque pointer forwarded to callbacks unchanged
- *
- * Use this variant when the caller has already resolved the path (e.g. to
- * register multiple probes on the same binary with a single kern_path call).
- * The inode is derived internally via d_real_inode(), so inode and path are
- * always consistent.
- *
- * Returns a pointer to the new rv_uprobe on success, ERR_PTR on failure.
- */
-struct rv_uprobe *rv_uprobe_attach_path(struct path *path, loff_t offset,
-	int (*entry_fn)(struct rv_uprobe *p, struct pt_regs *regs, __u64 *data),
-	int (*ret_fn)(struct rv_uprobe *p, unsigned long func,
-			struct pt_regs *regs, __u64 *data),
-	void *priv);
+/* Seamless inline declaration of a named uprobe inside user structs */
+#define DECLARE_RV_UPROBE(name) \
+	struct rv_uprobe name
 
 /**
- * rv_uprobe_attach - resolve binpath and register an uprobe
- * @binpath:  absolute path to the target binary
- * @offset:   byte offset within the binary
- * @entry_fn: called on probe hit (entry); may be NULL
- * @ret_fn:   called on function return (uretprobe); may be NULL
- * @priv:     opaque pointer forwarded to callbacks unchanged
+ * rv_uprobe_register - resolve binpath and register an uprobe
+ * @binpath:     absolute path to the target binary
+ * @offset:      byte offset within the binary
+ * @p:           pointer to the allocated/embedded rv_uprobe structure
+ * @handler:     called on probe hit (entry); may be NULL
+ * @ret_handler: called on function return (uretprobe); may be NULL
  *
- * Resolves binpath via kern_path(), then delegates to rv_uprobe_attach_path().
+ * Resolves binpath via kern_path(), registers the uprobe directly using the
+ * embedded `uprobe_consumer`, and immediately releases the path reference.
  *
- * Returns a pointer to the new rv_uprobe on success, ERR_PTR on failure.
+ * Returns 0 on success, or a negative error code on failure.
  */
-struct rv_uprobe *rv_uprobe_attach(const char *binpath, loff_t offset,
-	int (*entry_fn)(struct rv_uprobe *p, struct pt_regs *regs, __u64 *data),
-	int (*ret_fn)(struct rv_uprobe *p, unsigned long func,
-			struct pt_regs *regs, __u64 *data),
-	void *priv);
+int rv_uprobe_register(const char *binpath, loff_t offset, struct rv_uprobe *p,
+	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs, __u64 *data),
+	int (*ret_handler)(struct uprobe_consumer *self, unsigned long func,
+			   struct pt_regs *regs, __u64 *data));
 
 /**
- * rv_uprobe_detach - synchronously unregister an uprobe and free it
- * @p:  probe to detach; may be NULL (no-op)
- *
- * Calls uprobe_unregister_nosync(), then uprobe_unregister_sync() to wait
- * for any in-progress handler to finish, then releases the path reference
- * and frees the rv_uprobe struct.  The caller's priv data is NOT freed.
+ * rv_uprobe_unregister - synchronously unregister a uprobe
+ * @p:  probe to unregister; may be NULL (no-op)
  *
- * When removing a single probe, prefer this over the three-phase API.
- * Safe to call from process context only (uprobe_unregister_sync() may
- * schedule).
+ * Dequeues the uprobe and waits synchronously for all in-flight handlers
+ * to complete.
  */
-void rv_uprobe_detach(struct rv_uprobe *p);
+void rv_uprobe_unregister(struct rv_uprobe *p);
 
 /**
  * rv_uprobe_unregister_nosync - dequeue an uprobe without waiting
@@ -91,9 +64,7 @@ void rv_uprobe_detach(struct rv_uprobe *p);
  *
  * Removes the uprobe from the uprobe subsystem but does NOT wait for
  * in-flight handlers to complete.  The caller must call rv_uprobe_sync()
- * before calling rv_uprobe_free() on the same probe.
- *
- * Use this to batch multiple deregistrations before a single rv_uprobe_sync().
+ * before freeing any container holding this probe.
  */
 void rv_uprobe_unregister_nosync(struct rv_uprobe *p);
 
@@ -101,19 +72,8 @@ void rv_uprobe_unregister_nosync(struct rv_uprobe *p);
  * rv_uprobe_sync - wait for all in-flight uprobe handlers to complete
  *
  * Global barrier: waits for every in-flight uprobe handler across the system
- * to finish.  Call once after a batch of rv_uprobe_unregister_nosync() calls
- * and before any rv_uprobe_free() call.
+ * to finish.
  */
 void rv_uprobe_sync(void);
 
-/**
- * rv_uprobe_free - release resources of a previously deregistered probe
- * @p:  probe to free; may be NULL (no-op)
- *
- * Releases the path reference and frees the rv_uprobe struct.  Must only
- * be called after rv_uprobe_sync() has returned.  The caller's priv data
- * is NOT freed.
- */
-void rv_uprobe_free(struct rv_uprobe *p);
-
 #endif /* _RV_UPROBE_H */
diff --git a/kernel/trace/rv/monitors/tlob/tlob.c b/kernel/trace/rv/monitors/tlob/tlob.c
index d8e0c47947..28a6c740c7 100644
--- a/kernel/trace/rv/monitors/tlob/tlob.c
+++ b/kernel/trace/rv/monitors/tlob/tlob.c
@@ -252,8 +252,8 @@ struct tlob_uprobe_binding {
 	char			binpath[TLOB_MAX_PATH];
 	loff_t			offset_start;
 	loff_t			offset_stop;
-	struct rv_uprobe	*start_probe;
-	struct rv_uprobe	*stop_probe;
+	DECLARE_RV_UPROBE(start_probe);
+	DECLARE_RV_UPROBE(stop_probe);
 };
 
 /* RCU callback: free the slab once no readers remain. */
@@ -512,16 +512,16 @@ int tlob_stop_task(struct task_struct *task)
 EXPORT_SYMBOL_GPL(tlob_stop_task);
 
 
-static int tlob_uprobe_entry_handler(struct rv_uprobe *p, struct pt_regs *regs,
+static int tlob_uprobe_entry_handler(struct uprobe_consumer *self, struct pt_regs *regs,
 				     __u64 *data)
 {
-	struct tlob_uprobe_binding *b = p->priv;
+	struct tlob_uprobe_binding *b = container_of(self, struct tlob_uprobe_binding, start_probe.uc);
 
 	tlob_start_task(current, b->threshold_ns);
 	return 0;
 }
 
-static int tlob_uprobe_stop_handler(struct rv_uprobe *p, struct pt_regs *regs,
+static int tlob_uprobe_stop_handler(struct uprobe_consumer *self, struct pt_regs *regs,
 				    __u64 *data)
 {
 	tlob_stop_task(current);
@@ -537,6 +537,7 @@ static int tlob_add_uprobe(u64 threshold_ns, const char *binpath,
 {
 	struct tlob_uprobe_binding *b, *tmp_b;
 	char pathbuf[TLOB_MAX_PATH];
+	struct inode *inode;
 	struct path path;
 	char *canon;
 	int ret;
@@ -561,10 +562,12 @@ static int tlob_add_uprobe(u64 threshold_ns, const char *binpath,
 		goto err_path;
 	}
 
-	/* Reject duplicate start offset for the same binary. */
+	inode = d_real_inode(path.dentry);
+
+	/* Reject duplicate start offset for the same binary inode. */
 	list_for_each_entry(tmp_b, &tlob_uprobe_list, list) {
 		if (tmp_b->offset_start == offset_start &&
-		    tmp_b->start_probe->path.dentry == path.dentry) {
+		    tmp_b->start_probe.inode == inode) {
 			ret = -EEXIST;
 			goto err_path;
 		}
@@ -577,29 +580,25 @@ static int tlob_add_uprobe(u64 threshold_ns, const char *binpath,
 	}
 	strscpy(b->binpath, canon, sizeof(b->binpath));
 
-	/* Both probes share b (priv) and path; attach_path refs path itself. */
-	b->start_probe = rv_uprobe_attach_path(&path, offset_start,
-					       tlob_uprobe_entry_handler, NULL, b);
-	if (IS_ERR(b->start_probe)) {
-		ret = PTR_ERR(b->start_probe);
-		b->start_probe = NULL;
-		goto err_path;
-	}
+	path_put(&path);
+
+	/* Both probes are registered directly on the embedded fields */
+	ret = rv_uprobe_register(b->binpath, offset_start, &b->start_probe,
+				 tlob_uprobe_entry_handler, NULL);
+	if (ret)
+		goto err_free;
 
-	b->stop_probe = rv_uprobe_attach_path(&path, offset_stop,
-					      tlob_uprobe_stop_handler, NULL, b);
-	if (IS_ERR(b->stop_probe)) {
-		ret = PTR_ERR(b->stop_probe);
-		b->stop_probe = NULL;
+	ret = rv_uprobe_register(b->binpath, offset_stop, &b->stop_probe,
+				 tlob_uprobe_stop_handler, NULL);
+	if (ret)
 		goto err_start;
-	}
 
-	path_put(&path);
 	list_add_tail(&b->list, &tlob_uprobe_list);
 	return 0;
 
 err_start:
-	rv_uprobe_detach(b->start_probe);
+	rv_uprobe_unregister(&b->start_probe);
+	goto err_free;
 err_path:
 	path_put(&path);
 err_free:
@@ -611,21 +610,24 @@ static int tlob_remove_uprobe_by_key(loff_t offset_start, const char *binpath)
 {
 	struct tlob_uprobe_binding *b, *tmp;
 	struct path remove_path;
+	struct inode *inode;
 	int ret;
 
 	ret = kern_path(binpath, LOOKUP_FOLLOW, &remove_path);
 	if (ret)
 		return ret;
 
+	inode = d_real_inode(remove_path.dentry);
+
 	ret = -ENOENT;
 	list_for_each_entry_safe(b, tmp, &tlob_uprobe_list, list) {
 		if (b->offset_start != offset_start)
 			continue;
-		if (b->start_probe->path.dentry != remove_path.dentry)
+		if (b->start_probe.inode != inode)
 			continue;
 		list_del(&b->list);
-		rv_uprobe_detach(b->start_probe);
-		rv_uprobe_detach(b->stop_probe);
+		rv_uprobe_unregister(&b->start_probe);
+		rv_uprobe_unregister(&b->stop_probe);
 		kfree(b);
 		ret = 0;
 		break;
@@ -643,8 +645,8 @@ static void tlob_remove_all_uprobes(void)
 	mutex_lock(&tlob_uprobe_mutex);
 	list_for_each_entry_safe(b, tmp, &tlob_uprobe_list, list) {
 		list_move(&b->list, &pending);
-		rv_uprobe_unregister_nosync(b->start_probe);
-		rv_uprobe_unregister_nosync(b->stop_probe);
+		rv_uprobe_unregister_nosync(&b->start_probe);
+		rv_uprobe_unregister_nosync(&b->stop_probe);
 	}
 	mutex_unlock(&tlob_uprobe_mutex);
 
@@ -658,8 +660,6 @@ static void tlob_remove_all_uprobes(void)
 	rv_uprobe_sync();
 
 	list_for_each_entry_safe(b, tmp, &pending, list) {
-		rv_uprobe_free(b->start_probe);
-		rv_uprobe_free(b->stop_probe);
 		kfree(b);
 	}
 }
diff --git a/kernel/trace/rv/rv_uprobe.c b/kernel/trace/rv/rv_uprobe.c
index 3d8b764dde..69b8b0c27e 100644
--- a/kernel/trace/rv/rv_uprobe.c
+++ b/kernel/trace/rv/rv_uprobe.c
@@ -10,149 +10,74 @@
 #include <linux/uprobes.h>
 #include <rv/rv_uprobe.h>
 
-/*
- * Private extension of struct rv_uprobe.  Allocated by rv_uprobe_attach*()
- * and returned to callers as &impl->pub.
- */
-struct rv_uprobe_impl {
-	struct rv_uprobe	pub;	/* must be first; callers hold &pub */
-	struct uprobe_consumer	uc;
-	struct uprobe		*uprobe;
-	int (*entry_fn)(struct rv_uprobe *p, struct pt_regs *regs, __u64 *data);
-	int (*ret_fn)(struct rv_uprobe *p, unsigned long func,
-			struct pt_regs *regs, __u64 *data);
-};
-
-static int rv_uprobe_handler(struct uprobe_consumer *uc,
-			     struct pt_regs *regs, __u64 *data)
-{
-	struct rv_uprobe_impl *impl = container_of(uc, struct rv_uprobe_impl, uc);
-
-	if (impl->entry_fn)
-		return impl->entry_fn(&impl->pub, regs, data);
-	return 0;
-}
-
-static int rv_uprobe_ret_handler(struct uprobe_consumer *uc,
-				 unsigned long func,
-				 struct pt_regs *regs, __u64 *data)
-{
-	struct rv_uprobe_impl *impl = container_of(uc, struct rv_uprobe_impl, uc);
-
-	if (impl->ret_fn)
-		return impl->ret_fn(&impl->pub, func, regs, data);
-	return 0;
-}
-
-static struct rv_uprobe *
-__rv_uprobe_attach(struct inode *inode, struct path *path, loff_t offset,
-		   int (*entry_fn)(struct rv_uprobe *p, struct pt_regs *regs, __u64 *data),
-		   int (*ret_fn)(struct rv_uprobe *p, unsigned long func,
-				   struct pt_regs *regs, __u64 *data),
-		   void *priv)
-{
-	struct rv_uprobe_impl *impl;
-	int ret;
-
-	if (!entry_fn && !ret_fn)
-		return ERR_PTR(-EINVAL);
-
-	impl = kzalloc_obj(*impl, GFP_KERNEL);
-	if (!impl)
-		return ERR_PTR(-ENOMEM);
-
-	impl->pub.offset = offset;
-	impl->pub.priv   = priv;
-	impl->entry_fn   = entry_fn;
-	impl->ret_fn     = ret_fn;
-	path_get(path);
-	impl->pub.path   = *path;
-
-	if (entry_fn)
-		impl->uc.handler     = rv_uprobe_handler;
-	if (ret_fn)
-		impl->uc.ret_handler = rv_uprobe_ret_handler;
-
-	impl->uprobe = uprobe_register(inode, offset, 0, &impl->uc);
-	if (IS_ERR(impl->uprobe)) {
-		ret = PTR_ERR(impl->uprobe);
-		path_put(&impl->pub.path);
-		kfree(impl);
-		return ERR_PTR(ret);
-	}
-
-	return &impl->pub;
-}
-
-/**
- * rv_uprobe_attach_path - register an uprobe given an already-resolved path
- */
-struct rv_uprobe *rv_uprobe_attach_path(struct path *path, loff_t offset,
-	int (*entry_fn)(struct rv_uprobe *p, struct pt_regs *regs, __u64 *data),
-	int (*ret_fn)(struct rv_uprobe *p, unsigned long func,
-			struct pt_regs *regs, __u64 *data),
-	void *priv)
-{
-	struct inode *inode = d_real_inode(path->dentry);
-
-	return __rv_uprobe_attach(inode, path, offset, entry_fn, ret_fn, priv);
-}
-EXPORT_SYMBOL_GPL(rv_uprobe_attach_path);
-
 /**
- * rv_uprobe_attach - resolve binpath and register an uprobe
+ * rv_uprobe_register - resolve binpath and register an uprobe
  */
-struct rv_uprobe *rv_uprobe_attach(const char *binpath, loff_t offset,
-	int (*entry_fn)(struct rv_uprobe *p, struct pt_regs *regs, __u64 *data),
-	int (*ret_fn)(struct rv_uprobe *p, unsigned long func,
-			struct pt_regs *regs, __u64 *data),
-	void *priv)
+int rv_uprobe_register(const char *binpath, loff_t offset, struct rv_uprobe *p,
+	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs, __u64 *data),
+	int (*ret_handler)(struct uprobe_consumer *self, unsigned long func,
+			   struct pt_regs *regs, __u64 *data))
 {
-	struct rv_uprobe *p;
+	struct inode *inode;
 	struct path path;
 	int ret;
 
+	if (!handler && !ret_handler)
+		return -EINVAL;
+
 	ret = kern_path(binpath, LOOKUP_FOLLOW, &path);
 	if (ret)
-		return ERR_PTR(ret);
+		return ret;
 
 	if (!d_is_reg(path.dentry)) {
 		path_put(&path);
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
 
-	p = rv_uprobe_attach_path(&path, offset, entry_fn, ret_fn, priv);
+	inode = d_real_inode(path.dentry);
+
+	p->uc.handler     = handler;
+	p->uc.ret_handler = ret_handler;
+	p->inode          = inode;
+
+	p->uprobe = uprobe_register(inode, offset, 0, &p->uc);
 	path_put(&path);
-	return p;
+
+	if (IS_ERR(p->uprobe)) {
+		ret = PTR_ERR(p->uprobe);
+		p->uprobe = NULL;
+		p->inode  = NULL;
+		return ret;
+	}
+
+	return 0;
 }
-EXPORT_SYMBOL_GPL(rv_uprobe_attach);
+EXPORT_SYMBOL_GPL(rv_uprobe_register);
 
 /**
- * rv_uprobe_detach - synchronously unregister an uprobe and free it
+ * rv_uprobe_unregister - synchronously unregister a uprobe
  */
-void rv_uprobe_detach(struct rv_uprobe *p)
+void rv_uprobe_unregister(struct rv_uprobe *p)
 {
-	if (!p)
+	if (!p || IS_ERR_OR_NULL(p->uprobe))
 		return;
 
 	rv_uprobe_unregister_nosync(p);
 	rv_uprobe_sync();
-	rv_uprobe_free(p);
 }
-EXPORT_SYMBOL_GPL(rv_uprobe_detach);
+EXPORT_SYMBOL_GPL(rv_uprobe_unregister);
 
 /**
  * rv_uprobe_unregister_nosync - dequeue an uprobe without waiting
  */
 void rv_uprobe_unregister_nosync(struct rv_uprobe *p)
 {
-	struct rv_uprobe_impl *impl;
-
-	if (!p)
+	if (!p || IS_ERR_OR_NULL(p->uprobe))
 		return;
 
-	impl = container_of(p, struct rv_uprobe_impl, pub);
-	uprobe_unregister_nosync(impl->uprobe, &impl->uc);
+	uprobe_unregister_nosync(p->uprobe, &p->uc);
+	p->uprobe = NULL;
+	p->inode  = NULL;
 }
 EXPORT_SYMBOL_GPL(rv_uprobe_unregister_nosync);
 
@@ -164,19 +89,3 @@ void rv_uprobe_sync(void)
 	uprobe_unregister_sync();
 }
 EXPORT_SYMBOL_GPL(rv_uprobe_sync);
-
-/**
- * rv_uprobe_free - release resources of a previously deregistered probe
- */
-void rv_uprobe_free(struct rv_uprobe *p)
-{
-	struct rv_uprobe_impl *impl;
-
-	if (!p)
-		return;
-
-	impl = container_of(p, struct rv_uprobe_impl, pub);
-	path_put(&p->path);
-	kfree(impl);
-}
-EXPORT_SYMBOL_GPL(rv_uprobe_free);