linux-kernel - Re: [PATCH v2] fprobe: add unlock to match a succeeded ftrace_test_recursion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 7 Jul 2023 09:15:13 +0900
From:   Masami Hiramatsu (Google) <mhiramat@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Ze Gao <zegao2021@...il.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
        Ze Gao <zegao@...cent.com>, Yafang <laoar.shao@...il.com>
Subject: Re: [PATCH v2] fprobe: add unlock to match a succeeded
 ftrace_test_recursion_trylock

On Thu, 6 Jul 2023 12:09:16 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:

> On Mon,  3 Jul 2023 17:23:36 +0800
> Ze Gao <zegao2021@...il.com> wrote:
> 
> > Unlock ftrace recursion lock when fprobe_kprobe_handler() is failed
> > because of some running kprobe.
> > 
> > Fixes: 3cc4e2c5fbae ("fprobe: make fprobe_kprobe_handler recursion free")
> > Reported-by: Yafang <laoar.shao@...il.com>
> > Closes: https://lore.kernel.org/linux-trace-kernel/CALOAHbC6UpfFOOibdDiC7xFc5YFUgZnk3MZ=3Ny6we=AcrNbew@mail.gmail.com/
> > Signed-off-by: Ze Gao <zegao@...cent.com>
> 
> Reviewed-by: Steven Rostedt (Google) <rostedt@...dmis.org>
> 
> > ---
> >  kernel/trace/fprobe.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> > index 18d36842faf5..93b3e361bb97 100644
> > --- a/kernel/trace/fprobe.c
> > +++ b/kernel/trace/fprobe.c
> > @@ -102,12 +102,14 @@ static void fprobe_kprobe_handler(unsigned long ip, unsigned long parent_ip,
> >  
> >  	if (unlikely(kprobe_running())) {
> 
> Off topic for this patch, but Masami, what's the purpose of not calling the
> fprobe when a kprobe is running? Does that mean it has probed another kprobe?

This is for the user who is sharing their handler with kprobes (like eBPF),
which may expect that the handler is not called recursively. (e.g. an interrupt
happens while kprobe handler is running and that interrupt calls a function
which is fprobed)

> 
> Probably could add a comment here to explain the issue.

OK, it is also documented in Documentation/trace/fprobe.rst, but it is better
to comment in the code too.

Thanks,

> 
> -- Steve
> 
> 
> >  		fp->nmissed++;
> > -		return;
> > +		goto recursion_unlock;
> >  	}
> >  
> >  	kprobe_busy_begin();
> >  	__fprobe_handler(ip, parent_ip, ops, fregs);
> >  	kprobe_busy_end();
> > +
> > +recursion_unlock:
> >  	ftrace_test_recursion_unlock(bit);
> >  }
> >  
> 


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