rethook: Use __rcu pointer for rethook::handler
Commit Message
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(-)
Comments
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
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
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>
@@ -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)
{
@@ -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;
};
@@ -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);