lkml.org 
[lkml]   [2020]   [Apr]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC] kretprobe: Prevent triggering kretprobe from within kprobe_flush_task
On Thu, 9 Apr 2020 14:52:13 +0200
Jiri Olsa <jolsa@redhat.com> wrote:

> On Thu, Apr 09, 2020 at 09:38:06PM +0900, Masami Hiramatsu wrote:
> > Hi Jiri,
> >
> > On Wed, 8 Apr 2020 18:46:41 +0200
> > Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > > hi,
> > > Ziqian reported lockup when adding retprobe on _raw_spin_lock_irqsave.
> >
> > Hmm, kprobe is lockless, but kretprobe involves spinlock.
> > Thus, eventually, I will blacklist the _raw_spin_lock_irqsave()
> > for kretprobe.
>
> I thought of blacklisting, but we were using that kretprobe
> in a bcc script.. it's not overloaded by using bpf trampolines,
> but still might be useful
>
> SNIP
>
> > > The code within the kretprobe handler checks for probe reentrancy,
> > > so we won't trigger any _raw_spin_lock_irqsave probe in there.
> > >
> > > The problem is in outside kprobe_flush_task, where we call:
> > >
> > > kprobe_flush_task
> > > kretprobe_table_lock
> > > raw_spin_lock_irqsave
> > > _raw_spin_lock_irqsave
> > >
> > > where _raw_spin_lock_irqsave triggers the kretprobe and installs
> > > kretprobe_trampoline handler on _raw_spin_lock_irqsave return.
> >
> > Hmm, OK. In this case, I think we should mark this process is
> > going to die and never try to kretprobe on it.
> >
> > >
> > > The kretprobe_trampoline handler is then executed with already
> > > locked kretprobe_table_locks, and first thing it does is to
> > > lock kretprobe_table_locks ;-) the whole lockup path like:
> > >
> > > kprobe_flush_task
> > > kretprobe_table_lock
> > > raw_spin_lock_irqsave
> > > _raw_spin_lock_irqsave ---> probe triggered, kretprobe_trampoline installed
> > >
> > > ---> kretprobe_table_locks locked
> > >
> > > kretprobe_trampoline
> > > trampoline_handler
> > > kretprobe_hash_lock(current, &head, &flags); <--- deadlock
> > >
> > > The change below sets current_kprobe in kprobe_flush_task, so the probe
> > > recursion protection check is hit and the probe is never set. It seems
> > > to fix the deadlock.
> > >
> > > I'm not sure this is the best fix, any ideas are welcome ;-)
> >
> > Hmm, this is a bit tricky to fix this issue. Of course, temporary disable
> > kprobes (and kretprobe) on an area by filling current_kprobe might
> > be a good idea, but it also involves other kprobes.
> >
> > How about let kretprobe skip the task which state == TASK_DEAD ?
>
> hum, isn't that considerable amount of paths (with state == TASK_DEAD)
> that we would kill kprobes for? someone might want to trace it

OK, and I found that even after calling kprobe_flush_task(), it may be
work because the task will not be switched. kretprobe instance will be
reclaimed.

Thank you,


--
Masami Hiramatsu <mhiramat@kernel.org>

\
 
 \ /
  Last update: 2020-04-09 16:17    [W:0.147 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site