[PATCH] rethook: Use __rcu pointer for rethook::handler

[PATCH] rethook: Use __rcu pointer for rethook::handler

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Since the rethook::handler is an RCU-maganged pointer so that it will
notice readers the rethook is stopped (unregistered) or not, it should
be an __rcu pointer and use appropriate functions to be accessed. This
will use appropriate memory barrier when accessing it. OTOH, rethook::data
is never changed, so we don't need to check it in get_kretprobe().

Fixes: 54ecbe6f1ed5 ("rethook: Add a generic return hook")
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 include/linux/kprobes.h |    6 ++----
 include/linux/rethook.h |    2 +-
 kernel/trace/rethook.c  |   21 ++++++++++++---------
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 64672bace560..0ff44d6633e3 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -197,10 +197,8 @@ extern int arch_trampoline_kprobe(struct kprobe *p);
 #ifdef CONFIG_KRETPROBE_ON_RETHOOK
 static nokprobe_inline struct kretprobe *get_kretprobe(struct kretprobe_instance *ri)
 {
-	RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
-		"Kretprobe is accessed from instance under preemptive context");
-
-	return (struct kretprobe *)READ_ONCE(ri->node.rethook->data);
+	/* rethook::data is non-changed field, so that you can access it freely. */
+	return (struct kretprobe *)ri->node.rethook->data;
 }
 static nokprobe_inline unsigned long get_kretprobe_retaddr(struct kretprobe_instance *ri)
 {
diff --git a/include/linux/rethook.h b/include/linux/rethook.h
index ce69b2b7bc35..164cd32c25cd 100644
--- a/include/linux/rethook.h
+++ b/include/linux/rethook.h
@@ -28,7 +28,7 @@ typedef void (*rethook_handler_t) (struct rethook_node *, void *, unsigned long,
  */
 struct rethook {
 	void			*data;
-	rethook_handler_t	handler;
+	rethook_handler_t __rcu handler;
 	struct objpool_head	pool;
 	struct rcu_head		rcu;
 };
diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
index 6fd7d4ecbbc6..77f0e987bbff 100644
--- a/kernel/trace/rethook.c
+++ b/kernel/trace/rethook.c
@@ -48,7 +48,7 @@ static void rethook_free_rcu(struct rcu_head *head)
  */
 void rethook_stop(struct rethook *rh)
 {
-	WRITE_ONCE(rh->handler, NULL);
+	rcu_assign_pointer(rh->handler, NULL);
 }
 
 /**
@@ -63,7 +63,7 @@ void rethook_stop(struct rethook *rh)
  */
 void rethook_free(struct rethook *rh)
 {
-	WRITE_ONCE(rh->handler, NULL);
+	rcu_assign_pointer(rh->handler, NULL);
 
 	call_rcu(&rh->rcu, rethook_free_rcu);
 }
@@ -107,7 +107,7 @@ struct rethook *rethook_alloc(void *data, rethook_handler_t handler,
 		return ERR_PTR(-ENOMEM);
 
 	rh->data = data;
-	rh->handler = handler;
+	rcu_assign_pointer(rh->handler, handler);
 
 	/* initialize the objpool for rethook nodes */
 	if (objpool_init(&rh->pool, num, size, GFP_KERNEL, rh,
@@ -135,9 +135,12 @@ static void free_rethook_node_rcu(struct rcu_head *head)
  */
 void rethook_recycle(struct rethook_node *node)
 {
-	lockdep_assert_preemption_disabled();
+	rethook_handler_t handler;
+
+	handler = rcu_dereference_check(node->rethook->handler,
+					rcu_read_lock_any_held());
 
-	if (likely(READ_ONCE(node->rethook->handler)))
+	if (likely(handler))
 		objpool_push(node, &node->rethook->pool);
 	else
 		call_rcu(&node->rcu, free_rethook_node_rcu);
@@ -153,10 +156,9 @@ NOKPROBE_SYMBOL(rethook_recycle);
  */
 struct rethook_node *rethook_try_get(struct rethook *rh)
 {
-	rethook_handler_t handler = READ_ONCE(rh->handler);
-
-	lockdep_assert_preemption_disabled();
+	rethook_handler_t handler;
 
+	handler = rcu_dereference_check(rh->handler, rcu_read_lock_any_held());
 	/* Check whether @rh is going to be freed. */
 	if (unlikely(!handler))
 		return NULL;
@@ -300,7 +302,8 @@ unsigned long rethook_trampoline_handler(struct pt_regs *regs,
 		rhn = container_of(first, struct rethook_node, llist);
 		if (WARN_ON_ONCE(rhn->frame != frame))
 			break;
-		handler = READ_ONCE(rhn->rethook->handler);
+		handler = rcu_dereference_check(rhn->rethook->handler,
+						rcu_read_lock_any_held());
 		if (handler)
 			handler(rhn, rhn->rethook->data,
 				correct_ret_addr, regs);

Re: [PATCH] rethook: Use __rcu pointer for rethook::handler

[kernel test robot]

Hi Masami,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.7-rc2 next-20231124]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Masami-Hiramatsu-Google/rethook-Use-__rcu-pointer-for-rethook-handler/20231124-090634
base:   linus/master
patch link:    https://lore.kernel.org/r/170078778632.209874.7893551840863388753.stgit%40devnote2
patch subject: [PATCH] rethook: Use __rcu pointer for rethook::handler
config: x86_64-randconfig-r113-20231124 (https://download.01.org/0day-ci/archive/20231124/202311241808.rv9ceuAh-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231124/202311241808.rv9ceuAh-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311241808.rv9ceuAh-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> kernel/trace/rethook.c:51:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
>> kernel/trace/rethook.c:51:9: sparse:    void ( [noderef] __rcu * )( ... )
>> kernel/trace/rethook.c:51:9: sparse:    void ( * )( ... )
   kernel/trace/rethook.c:66:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
   kernel/trace/rethook.c:66:9: sparse:    void ( [noderef] __rcu * )( ... )
   kernel/trace/rethook.c:66:9: sparse:    void ( * )( ... )
   kernel/trace/rethook.c:110:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
   kernel/trace/rethook.c:110:9: sparse:    void ( [noderef] __rcu * )( ... )
   kernel/trace/rethook.c:110:9: sparse:    void ( * )( ... )
   kernel/trace/rethook.c:140:19: sparse: sparse: incompatible types in comparison expression (different address spaces):
   kernel/trace/rethook.c:140:19: sparse:    void ( [noderef] __rcu * )( ... )
   kernel/trace/rethook.c:140:19: sparse:    void ( * )( ... )
   kernel/trace/rethook.c:161:19: sparse: sparse: incompatible types in comparison expression (different address spaces):
   kernel/trace/rethook.c:161:19: sparse:    void ( [noderef] __rcu * )( ... )
   kernel/trace/rethook.c:161:19: sparse:    void ( * )( ... )
   kernel/trace/rethook.c:305:27: sparse: sparse: incompatible types in comparison expression (different address spaces):
   kernel/trace/rethook.c:305:27: sparse:    void ( [noderef] __rcu * )( ... )
   kernel/trace/rethook.c:305:27: sparse:    void ( * )( ... )

vim +51 kernel/trace/rethook.c

    40	
    41	/**
    42	 * rethook_stop() - Stop using a rethook.
    43	 * @rh: the struct rethook to stop.
    44	 *
    45	 * Stop using a rethook to prepare for freeing it. If you want to wait for
    46	 * all running rethook handler before calling rethook_free(), you need to
    47	 * call this first and wait RCU, and call rethook_free().
    48	 */
    49	void rethook_stop(struct rethook *rh)
    50	{
  > 51		rcu_assign_pointer(rh->handler, NULL);
    52	}
    53	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] rethook: Use __rcu pointer for rethook::handler

[Masami Hiramatsu]

On Fri, 24 Nov 2023 23:40:57 +0800
kernel test robot <lkp@intel.com> wrote:

> Hi Masami,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on linus/master]
> [also build test WARNING on v6.7-rc2 next-20231124]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Masami-Hiramatsu-Google/rethook-Use-__rcu-pointer-for-rethook-handler/20231124-090634
> base:   linus/master
> patch link:    https://lore.kernel.org/r/170078778632.209874.7893551840863388753.stgit%40devnote2
> patch subject: [PATCH] rethook: Use __rcu pointer for rethook::handler
> config: x86_64-randconfig-r113-20231124 (https://download.01.org/0day-ci/archive/20231124/202311241808.rv9ceuAh-lkp@intel.com/config)
> compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231124/202311241808.rv9ceuAh-lkp@intel.com/reproduce)

Let me fix this issue. It seems that sparse with function pointer
needs a special care.

Thank you,

> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202311241808.rv9ceuAh-lkp@intel.com/
> 
> sparse warnings: (new ones prefixed by >>)
> >> kernel/trace/rethook.c:51:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
> >> kernel/trace/rethook.c:51:9: sparse:    void ( [noderef] __rcu * )( ... )
> >> kernel/trace/rethook.c:51:9: sparse:    void ( * )( ... )
>    kernel/trace/rethook.c:66:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
>    kernel/trace/rethook.c:66:9: sparse:    void ( [noderef] __rcu * )( ... )
>    kernel/trace/rethook.c:66:9: sparse:    void ( * )( ... )
>    kernel/trace/rethook.c:110:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
>    kernel/trace/rethook.c:110:9: sparse:    void ( [noderef] __rcu * )( ... )
>    kernel/trace/rethook.c:110:9: sparse:    void ( * )( ... )
>    kernel/trace/rethook.c:140:19: sparse: sparse: incompatible types in comparison expression (different address spaces):
>    kernel/trace/rethook.c:140:19: sparse:    void ( [noderef] __rcu * )( ... )
>    kernel/trace/rethook.c:140:19: sparse:    void ( * )( ... )
>    kernel/trace/rethook.c:161:19: sparse: sparse: incompatible types in comparison expression (different address spaces):
>    kernel/trace/rethook.c:161:19: sparse:    void ( [noderef] __rcu * )( ... )
>    kernel/trace/rethook.c:161:19: sparse:    void ( * )( ... )
>    kernel/trace/rethook.c:305:27: sparse: sparse: incompatible types in comparison expression (different address spaces):
>    kernel/trace/rethook.c:305:27: sparse:    void ( [noderef] __rcu * )( ... )
>    kernel/trace/rethook.c:305:27: sparse:    void ( * )( ... )
> 
> vim +51 kernel/trace/rethook.c
> 
>     40	
>     41	/**
>     42	 * rethook_stop() - Stop using a rethook.
>     43	 * @rh: the struct rethook to stop.
>     44	 *
>     45	 * Stop using a rethook to prepare for freeing it. If you want to wait for
>     46	 * all running rethook handler before calling rethook_free(), you need to
>     47	 * call this first and wait RCU, and call rethook_free().
>     48	 */
>     49	void rethook_stop(struct rethook *rh)
>     50	{
>   > 51		rcu_assign_pointer(rh->handler, NULL);
>     52	}
>     53	
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] rethook: Use __rcu pointer for rethook::handler

[Masami Hiramatsu]

On Tue, 28 Nov 2023 08:02:23 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Fri, 24 Nov 2023 23:40:57 +0800
> kernel test robot <lkp@intel.com> wrote:
> 
> > Hi Masami,
> > 
> > kernel test robot noticed the following build warnings:
> > 
> > [auto build test WARNING on linus/master]
> > [also build test WARNING on v6.7-rc2 next-20231124]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Masami-Hiramatsu-Google/rethook-Use-__rcu-pointer-for-rethook-handler/20231124-090634
> > base:   linus/master
> > patch link:    https://lore.kernel.org/r/170078778632.209874.7893551840863388753.stgit%40devnote2
> > patch subject: [PATCH] rethook: Use __rcu pointer for rethook::handler
> > config: x86_64-randconfig-r113-20231124 (https://download.01.org/0day-ci/archive/20231124/202311241808.rv9ceuAh-lkp@intel.com/config)
> > compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231124/202311241808.rv9ceuAh-lkp@intel.com/reproduce)
> 
> Let me fix this issue. It seems that sparse with function pointer
> needs a special care.

Hmm, I think we need to ignore some warnings from sparse for function pointer.

struct rethook {
	rethook_handler_t __rcu handler;
};

This doesn't work because the handler is not a pointer but a value type from
the sparse point of view(?)

So if we change the rethook_handler_t as below, most of the sparse warnings
are gone.

typedef void (__rcu *rethook_handler_t) (struct rethook_node *, void *, unsigned long, struct pt_regs *);
                  ^^^^^^

However, I found that this causes another warnings on the init function call
which requires rethook_handler_t as a parameter.

kernel/trace/fprobe.c:209:49: warning: incorrect type in argument 2 (different address spaces)
kernel/trace/fprobe.c:209:49:    expected void ( [noderef] __rcu *[usertype] handler )( ... )
kernel/trace/fprobe.c:209:49:    got void ( * )( ... )

        fp->rethook = rethook_alloc((void *)fp, fprobe_exit_handler,
							      ^^^^^^^^^^^^^^^^^^^^^^^^^
                                sizeof(struct fprobe_rethook_node), size);

Why? because fprobe_exit_handler() has no "__rcu"! Of course we can fix this
warning with force casting, e.g.

        fp->rethook = rethook_alloc((void *)fp, (rethook_handler_t)fprobe_exit_handler,

But this is totally wrong because it disables the compiler's type check!

One possible solution is to use a raw function pointer type for the
rethook_alloc() but that will be redundant or, just ignore
the sparse warnings as Documentation/RCU/rcu_dereference.rst said.

(Another tricky hack is to use a union just for rcu_assign_pointer/rcu_derefernce)

Thank you,


> 
> Thank you,
> 
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202311241808.rv9ceuAh-lkp@intel.com/
> > 
> > sparse warnings: (new ones prefixed by >>)
> > >> kernel/trace/rethook.c:51:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
> > >> kernel/trace/rethook.c:51:9: sparse:    void ( [noderef] __rcu * )( ... )
> > >> kernel/trace/rethook.c:51:9: sparse:    void ( * )( ... )
> >    kernel/trace/rethook.c:66:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
> >    kernel/trace/rethook.c:66:9: sparse:    void ( [noderef] __rcu * )( ... )
> >    kernel/trace/rethook.c:66:9: sparse:    void ( * )( ... )
> >    kernel/trace/rethook.c:110:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
> >    kernel/trace/rethook.c:110:9: sparse:    void ( [noderef] __rcu * )( ... )
> >    kernel/trace/rethook.c:110:9: sparse:    void ( * )( ... )
> >    kernel/trace/rethook.c:140:19: sparse: sparse: incompatible types in comparison expression (different address spaces):
> >    kernel/trace/rethook.c:140:19: sparse:    void ( [noderef] __rcu * )( ... )
> >    kernel/trace/rethook.c:140:19: sparse:    void ( * )( ... )
> >    kernel/trace/rethook.c:161:19: sparse: sparse: incompatible types in comparison expression (different address spaces):
> >    kernel/trace/rethook.c:161:19: sparse:    void ( [noderef] __rcu * )( ... )
> >    kernel/trace/rethook.c:161:19: sparse:    void ( * )( ... )
> >    kernel/trace/rethook.c:305:27: sparse: sparse: incompatible types in comparison expression (different address spaces):
> >    kernel/trace/rethook.c:305:27: sparse:    void ( [noderef] __rcu * )( ... )
> >    kernel/trace/rethook.c:305:27: sparse:    void ( * )( ... )
> > 
> > vim +51 kernel/trace/rethook.c
> > 
> >     40	
> >     41	/**
> >     42	 * rethook_stop() - Stop using a rethook.
> >     43	 * @rh: the struct rethook to stop.
> >     44	 *
> >     45	 * Stop using a rethook to prepare for freeing it. If you want to wait for
> >     46	 * all running rethook handler before calling rethook_free(), you need to
> >     47	 * call this first and wait RCU, and call rethook_free().
> >     48	 */
> >     49	void rethook_stop(struct rethook *rh)
> >     50	{
> >   > 51		rcu_assign_pointer(rh->handler, NULL);
> >     52	}
> >     53	
> > 
> > -- 
> > 0-DAY CI Kernel Test Service
> > https://github.com/intel/lkp-tests/wiki
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>