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

[Google]

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

> On Fri, 24 Nov 2023 23:40:57 +0800
> kernel test robot <lkp@xxxxxxxxx> 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@xxxxxxxxx/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@xxxxxxxxx/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@xxxxxxxxx>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202311241808.rv9ceuAh-lkp@xxxxxxxxx/
> > 
> > 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@xxxxxxxxxx>


-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>