On Wed, Mar 30, 2022 at 2:15 AM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
>
> On Wed, 30 Mar 2022 13:52:17 +0900
> Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
>
> > On Tue, 29 Mar 2022 18:51:22 -0700
> > Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > > On Tue, Mar 29, 2022 at 6:41 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> > > >
> > > > On Tue, 29 Mar 2022 16:49:24 -0700 Alexei Starovoitov wrote:
> > > > > Hi David, hi Jakub,
> > > > >
> > > > > The following pull-request contains BPF updates for your *net* tree.
> > > > >
> > > > > We've added 16 non-merge commits during the last 1 day(s) which contain
> > > > > a total of 24 files changed, 354 insertions(+), 187 deletions(-).
> > > > >
> > > > > The main changes are:
> > > > >
> > > > > 1) x86 specific bits of fprobe/rethook, from Masami and Peter.
> > > > >
> > > > > 2) ice/xsk fixes, from Maciej and Magnus.
> > > > >
> > > > > 3) Various small fixes, from Andrii, Yonghong, Geliang and others.
> > > >
> > > > There are some new sparse warnings here that look semi-legit.
> > > > As in harmless but not erroneous.
> > >
> > > Both are new warnings and not due to these patches, right?
> > >
> > > > kernel/trace/rethook.c:68:9: error: incompatible types in comparison expression (different address spaces):
> > > > kernel/trace/rethook.c:68:9: void ( [noderef] __rcu * )( ... )
> > > > kernel/trace/rethook.c:68:9: void ( * )( ... )
> > > >
> > > > 66 void rethook_free(struct rethook *rh)
> > > > 67 {
> > > > 68 rcu_assign_pointer(rh->handler, NULL);
> > > > 69
> > > > 70 call_rcu(&rh->rcu, rethook_free_rcu);
> > > > 71 }
> > > >
> > > > Looks like this should be a WRITE_ONCE() ?
> > >
> > > Masami, please take a look.
> >
> > Yeah, I think we should make this rcu pointer (and read side must use rcu_dereference())
> > because this rh->handler becomes the key to disable this rethook.
> > Let me fix that.
>
> Sorry, please ignore this. Since the handler pointed by rh->handler never
> be removed (unless removed by modules, but this will not happen while
> the rethook is running), YES, WRITE_ONCE() is enough.
> Please add below.
>
> From 92c9c784458f03900823360981812220ce3c7bf3 Mon Sep 17 00:00:00 2001
> From: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> Date: Wed, 30 Mar 2022 18:13:42 +0900
> Subject: [PATCH] rethook: Fix to use WRITE_ONCE() for rethook::handler
>
> Since the function pointered by rethook::handler never be removed when
> the rethook is alive, it doesn't need to use rcu_assign_pointer() to
> update it. Just use WRITE_ONCE().
>
> Reported-by: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>
> Signed-off-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
Could you please send it as a proper patch so it registers in patchwork?
> ---
> kernel/trace/rethook.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
> index ab463a4d2b23..b56833700d23 100644
> --- a/kernel/trace/rethook.c
> +++ b/kernel/trace/rethook.c
> @@ -65,7 +65,7 @@ static void rethook_free_rcu(struct rcu_head *head)
> */
> void rethook_free(struct rethook *rh)
> {
> - rcu_assign_pointer(rh->handler, NULL);
> + WRITE_ONCE(rh->handler, NULL);
>
> call_rcu(&rh->rcu, rethook_free_rcu);
> }
> --
> 2.25.1
> --
> Masami Hiramatsu <mhiramat@xxxxxxxxxx>
