Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-10 Thread Masami Hiramatsu 
Hi Peter and Ingo,

On Wed, 2 Sep 2020 09:02:26 +0200
pet...@infradead.org wrote:

> On Wed, Sep 02, 2020 at 09:37:39AM +0900, Masami Hiramatsu wrote:
> > On Tue, 1 Sep 2020 21:08:08 +0200
> > Peter Zijlstra  wrote:
> > 
> > > On Sat, Aug 29, 2020 at 09:59:49PM +0900, Masami Hiramatsu wrote:
> > > > Masami Hiramatsu (16):
> > > >   kprobes: Add generic kretprobe trampoline handler
> > > >   x86/kprobes: Use generic kretprobe trampoline handler
> > > >   arm: kprobes: Use generic kretprobe trampoline handler
> > > >   arm64: kprobes: Use generic kretprobe trampoline handler
> > > >   arc: kprobes: Use generic kretprobe trampoline handler
> > > >   csky: kprobes: Use generic kretprobe trampoline handler
> > > >   ia64: kprobes: Use generic kretprobe trampoline handler
> > > >   mips: kprobes: Use generic kretprobe trampoline handler
> > > >   parisc: kprobes: Use generic kretprobe trampoline handler
> > > >   powerpc: kprobes: Use generic kretprobe trampoline handler
> > > >   s390: kprobes: Use generic kretprobe trampoline handler
> > > >   sh: kprobes: Use generic kretprobe trampoline handler
> > > >   sparc: kprobes: Use generic kretprobe trampoline handler
> > > >   kprobes: Remove NMI context check
> > > >   kprobes: Free kretprobe_instance with rcu callback
> > > >   kprobes: Make local used functions static
> > > > 
> > > > Peter Zijlstra (5):
> > > >   llist: Add nonatomic __llist_add() and __llist_dell_all()
> > > >   kprobes: Remove kretprobe hash
> > > >   asm-generic/atomic: Add try_cmpxchg() fallbacks
> > > >   freelist: Lock less freelist
> > > >   kprobes: Replace rp->free_instance with freelist
> > > 
> > > This looks good to me, do you want me to merge them through -tip? If so,
> > > do we want to try and get them in this release still?
> > 
> > Yes, thanks. For the kretprobe missing issue, we will need the first half
> > (up to "kprobes: Remove NMI context check"), so we can split the series
> > if someone think the lockless is still immature.
> 
> Ok, but then lockdep will yell at you if you have that enabled and run
> the unoptimized things.
> 
> > > Ingo, opinions? This basically fixes a regression cauesd by
> > > 
> > >   0d00449c7a28 ("x86: Replace ist_enter() with nmi_enter()")

So what would you think of this? I saw the unification part of this series
on the tip/master, but lockless part is not there. This might still keep
lockdep to warn on kretprobes if we disable CONFIG_FUNCTION_TRACER and
optprobe.

If we make the kretprobe lockless, we will remove all locks from in-kernel
kprobe handlers. So at least upstream user will be happy.

Or, do we fix lockdep warning on the spinlocks in kprobe handlers first?

Thank you,

-- 
Masami Hiramatsu

Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-08 Thread Masami Hiramatsu 
On Wed, 9 Sep 2020 00:09:23 +0900
Masami Hiramatsu  wrote:

> > > Of course make it lockless then warning is gone.
> > > But even without the lockless patch, this warning can be false-positive
> > > because we prohibit nested kprobe call, right?
> > 
> > Yes, because the actual nesting is avoided by kprobe_busy, but lockdep
> > can't tell. Lockdep sees a regular lock user and an in-nmi lock user and
> > figures that's a bad combination.

Hmm, what about introducing new LOCK_USED_KPROBE bit, which will be set
if the lock is accessed when the current_kprobe is set (including kprobe_busy)?
This means it is in the kprobe user-handler context. If we access the lock 
always
in the kprobes context, it is never nested.

Thank you,

-- 
Masami Hiramatsu

Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-08 Thread Masami Hiramatsu 
On Tue, 8 Sep 2020 12:37:36 +0200
pet...@infradead.org wrote:

> On Thu, Sep 03, 2020 at 10:39:54AM +0900, Masami Hiramatsu wrote:
> 
> > > There's a bug, that might make it miss it. I have a patch. I'll send it
> > > shortly.
> > 
> > OK, I've confirmed that the lockdep warns on kretprobe from INT3
> > with your fix.
> 
> I'm now trying and failing to reproduce I can't seem to make it use
> int3 today. It seems to want to use ftrace or refuses everything. I'm
> probably doing it wrong.

Ah, yes. For using the INT3, we need to disable CONFIG_FUNCTION_TRACER,
or enable CONFIG_KPROBE_EVENTS_ON_NOTRACE and put a kretprobe on a notrace
function :)

> 
> Could you verify the below actually works? It's on top of the first 16
> patches.

Sure. (BTW, you mean the first 15 patches, since kretprobe_hash_lock()
becomes static by 16th patch ?)

Here is the result. I got same warning with or without your patch.
I have built the kernel with CONFIG_FUNCTION_TRACER=n and 
CONFIG_KPROBES_SANITY_TEST=y. 

[0.269235] PCI: Using configuration type 1 for base access
[0.272171] Kprobe smoke test: started
[0.281125] 
[0.281126] 
[0.281126] WARNING: inconsistent lock state
[0.281126] 5.9.0-rc2+ #101 Not tainted
[0.281126] 
[0.281127] inconsistent {INITIAL USE} -> {IN-NMI} usage.
[0.281127] swapper/0/1 [HC1[1]:SC0[0]:HE0:SE1] takes:
[0.281127] 8228c778 (&rp->lock){}-{2:2}, at: 
pre_handler_kretprobe+0x2b/0x1b0
[0.281128] {INITIAL USE} state was registered at:
[0.281129]   lock_acquire+0x9e/0x3c0
[0.281129]   _raw_spin_lock+0x2f/0x40
[0.281129]   recycle_rp_inst+0x48/0x90
[0.281129]   __kretprobe_trampoline_handler+0x15d/0x1e0
[0.281130]   trampoline_handler+0x43/0x60
[0.281130]   kretprobe_trampoline+0x2a/0x50
[0.281130]   kretprobe_trampoline+0x0/0x50
[0.281130]   init_kprobes+0x1b6/0x1d5
[0.281130]   do_one_initcall+0x5c/0x315
[0.281131]   kernel_init_freeable+0x21a/0x25f
[0.281131]   kernel_init+0x9/0x104
[0.281131]   ret_from_fork+0x22/0x30
[0.281131] irq event stamp: 25978
[0.281132] hardirqs last  enabled at (25977): [] 
queue_delayed_work_on+0x57/0x90
[0.281132] hardirqs last disabled at (25978): [] 
exc_int3+0x48/0x140
[0.281132] softirqs last  enabled at (25964): [] 
__do_softirq+0x389/0x48b
[0.281133] softirqs last disabled at (25957): [] 
asm_call_on_stack+0x12/0x20
[0.281133] 
[0.281133] other info that might help us debug this:
[0.281133]  Possible unsafe locking scenario:
[0.281134] 
[0.281134]CPU0
[0.281134]
[0.281134]   lock(&rp->lock);
[0.281135]   
[0.281135] lock(&rp->lock);
[0.281136] 
[0.281136]  *** DEADLOCK ***
[0.281136] 
[0.281136] no locks held by swapper/0/1.
[0.281136] 
[0.281137] stack backtrace:
[0.281137] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc2+ #101
[0.281137] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.13.0-1ubuntu1 04/01/2014
[0.281137] Call Trace:
[0.281138]  dump_stack+0x81/0xba
[0.281138]  print_usage_bug.cold+0x195/0x19e
[0.281138]  lock_acquire+0x314/0x3c0
[0.281138]  ? __text_poke+0x2db/0x400
[0.281139]  ? pre_handler_kretprobe+0x2b/0x1b0
[0.281139]  _raw_spin_lock_irqsave+0x3a/0x50
[0.281139]  ? pre_handler_kretprobe+0x2b/0x1b0
[0.281139]  pre_handler_kretprobe+0x2b/0x1b0
[0.281139]  ? stop_machine_from_inactive_cpu+0x120/0x120
[0.281140]  aggr_pre_handler+0x5f/0xd0
[0.281140]  kprobe_int3_handler+0x10f/0x160
[0.281140]  exc_int3+0x52/0x140
[0.281140]  asm_exc_int3+0x31/0x40
[0.281141] RIP: 0010:kprobe_target+0x1/0x10
[0.281141] Code: 65 48 33 04 25 28 00 00 00 75 10 48 81 c4 90 00 00 00 44 
89 e0 41 5c 5d c3 0f 0b e8 a
[0.281141] RSP: :c9013e48 EFLAGS: 0246
[0.281142] RAX: 81142130 RBX: 0001 RCX: c9013cec
[0.281142] RDX: 0002 RSI:  RDI: f3eb0b20
[0.281142] RBP: c9013e68 R08:  R09: 0001
[0.281143] R10:  R11:  R12: 
[0.281143] R13: 8224c2b0 R14: 8255cf60 R15: 
[0.281143]  ? stop_machine_from_inactive_cpu+0x120/0x120
[0.281143]  ? kprobe_target+0x1/0x10
[0.281144]  ? stop_machine_from_inactive_cpu+0x120/0x120
[0.281144]  ? init_test_probes.cold+0x2e3/0x391
[0.281144]  init_kprobes+0x1b6/0x1d5
[0.281144]  ? debugfs_kprobe_init+0xa9/0xa9
[0.281145]  do_one_initcall+0x5c/0x315
[0.281145]  ? rcu_read_lock_sched_held+0x4a/0x80
[0.281145]  kernel_init_freeable+0x21a/0x25f
[0.281145]  ? rest_init+0x23c/0x23c
[0.281145]  kernel_init+0x9/0x104
[0.281146]  ret_from_fork+0x22/0x30
[0.281282] Kprobe smoke test: passed successfully


> 
> > Of course make it lockless

Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-08 Thread peterz 
On Tue, Sep 08, 2020 at 11:15:14AM +, eddy...@trendmicro.com wrote:
> > From: pet...@infradead.org 
> >
> > I'm now trying and failing to reproduce I can't seem to make it use
> > int3 today. It seems to want to use ftrace or refuses everything. I'm
> > probably doing it wrong.
> >
> 
> You can turn off CONFIG_KPROBES_ON_FTRACE (and also sysctl
> debug.kprobes-optimization) to make it use int3 handler

I'm fairly sure I used the sysctl like in your original reproducer.

I'll try again later.

RE: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-08 Thread eddy...@trendmicro.com 
> From: pet...@infradead.org 
>
> I'm now trying and failing to reproduce I can't seem to make it use
> int3 today. It seems to want to use ftrace or refuses everything. I'm
> probably doing it wrong.
>

You can turn off CONFIG_KPROBES_ON_FTRACE (and also sysctl 
debug.kprobes-optimization) to make it use int3 handler


TREND MICRO EMAIL NOTICE

The information contained in this email and any attachments is confidential and 
may be subject to copyright or other intellectual property protection. If you 
are not the intended recipient, you are not authorized to use or disclose this 
information, and we request that you notify us by reply mail or telephone and 
delete the original message from your mail system.

For details about what personal information we collect and why, please see our 
Privacy Notice on our website at: Read privacy 
policy

Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-08 Thread peterz 
On Thu, Sep 03, 2020 at 10:39:54AM +0900, Masami Hiramatsu wrote:

> > There's a bug, that might make it miss it. I have a patch. I'll send it
> > shortly.
> 
> OK, I've confirmed that the lockdep warns on kretprobe from INT3
> with your fix.

I'm now trying and failing to reproduce I can't seem to make it use
int3 today. It seems to want to use ftrace or refuses everything. I'm
probably doing it wrong.

Could you verify the below actually works? It's on top of the first 16
patches.

> Of course make it lockless then warning is gone.
> But even without the lockless patch, this warning can be false-positive
> because we prohibit nested kprobe call, right?

Yes, because the actual nesting is avoided by kprobe_busy, but lockdep
can't tell. Lockdep sees a regular lock user and an in-nmi lock user and
figures that's a bad combination.


---
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1241,48 +1241,47 @@ void recycle_rp_inst(struct kretprobe_in
 }
 NOKPROBE_SYMBOL(recycle_rp_inst);
 
-void kretprobe_hash_lock(struct task_struct *tsk,
-struct hlist_head **head, unsigned long *flags)
-__acquires(hlist_lock)
-{
-   unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
-   raw_spinlock_t *hlist_lock;
-
-   *head = &kretprobe_inst_table[hash];
-   hlist_lock = kretprobe_table_lock_ptr(hash);
-   raw_spin_lock_irqsave(hlist_lock, *flags);
-}
-NOKPROBE_SYMBOL(kretprobe_hash_lock);
-
 static void kretprobe_table_lock(unsigned long hash,
 unsigned long *flags)
 __acquires(hlist_lock)
 {
raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
-   raw_spin_lock_irqsave(hlist_lock, *flags);
+   /*
+* HACK, due to kprobe_busy we'll never actually recurse, make NMI
+* context use a different lock class to avoid it reporting.
+*/
+   raw_spin_lock_irqsave_nested(hlist_lock, *flags, !!in_nmi());
 }
 NOKPROBE_SYMBOL(kretprobe_table_lock);
 
-void kretprobe_hash_unlock(struct task_struct *tsk,
-  unsigned long *flags)
+static void kretprobe_table_unlock(unsigned long hash,
+  unsigned long *flags)
 __releases(hlist_lock)
 {
+   raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
+   raw_spin_unlock_irqrestore(hlist_lock, *flags);
+}
+NOKPROBE_SYMBOL(kretprobe_table_unlock);
+
+void kretprobe_hash_lock(struct task_struct *tsk,
+struct hlist_head **head, unsigned long *flags)
+__acquires(hlist_lock)
+{
unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
-   raw_spinlock_t *hlist_lock;
 
-   hlist_lock = kretprobe_table_lock_ptr(hash);
-   raw_spin_unlock_irqrestore(hlist_lock, *flags);
+   *head = &kretprobe_inst_table[hash];
+   kretprobe_table_lock(hash, flags);
 }
-NOKPROBE_SYMBOL(kretprobe_hash_unlock);
+NOKPROBE_SYMBOL(kretprobe_hash_lock);
 
-static void kretprobe_table_unlock(unsigned long hash,
-  unsigned long *flags)
+void kretprobe_hash_unlock(struct task_struct *tsk,
+  unsigned long *flags)
 __releases(hlist_lock)
 {
-   raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
-   raw_spin_unlock_irqrestore(hlist_lock, *flags);
+   unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
+   kretprobe_table_unlock(hash, flags);
 }
-NOKPROBE_SYMBOL(kretprobe_table_unlock);
+NOKPROBE_SYMBOL(kretprobe_hash_unlock);
 
 struct kprobe kprobe_busy = {
.addr = (void *) get_kprobe,

Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-07 Thread Masami Hiramatsu 
On Mon, 07 Sep 2020 13:44:19 -0400
f...@redhat.com (Frank Ch. Eigler) wrote:

> Masami Hiramatsu  writes:
> 
> > Sorry, for noticing this point, I Cc'd to systemtap. Is systemtap taking
> > care of spinlock too?
> 
> On PRREMPT_RT configurations, systemtap uses the raw_spinlock_t
> types/functions, to keep its probe handlers as atomic as we can make them.

OK, if the lock is only used in the probe handlers, there should be
no problem. Even if a probe hits in the NMI which happens in another
kprobe handler, the probe does not call its handler (because we don't
support nested kprobes* yet).
But maybe you'll get warnings if you enable the lockdep.

* 
https://lkml.kernel.org/r/158894789510.14896.13461271606820304664.stgit@devnote2
It seems that we need more work for the nested kprobes.

Thank you,

-- 
Masami Hiramatsu

Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-07 Thread Frank Ch. Eigler 
Masami Hiramatsu  writes:

> Sorry, for noticing this point, I Cc'd to systemtap. Is systemtap taking
> care of spinlock too?

On PRREMPT_RT configurations, systemtap uses the raw_spinlock_t
types/functions, to keep its probe handlers as atomic as we can make them.

- FChE

Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-02 Thread Masami Hiramatsu 
On Thu, 3 Sep 2020 10:39:54 +0900
Masami Hiramatsu  wrote:

> OK, I've confirmed that the lockdep warns on kretprobe from INT3
> with your fix. Of course make it lockless then warning is gone.
> But even without the lockless patch, this warning can be false-positive
> because we prohibit nested kprobe call, right?
> 
> If the kprobe user handler uses a spinlock, the spinlock is used
> only in that handler (and in the context between kprobe_busy_begin/end),
> it will be safe since the spinlock is not nested.
> But if the spinlock is shared with other context, it will be dangerous
> because it can be interrupted by NMI (including INT3). This also applied
> to the function which is called from kprobe user handlers, thus user
> has to take care of it.

Sorry, for noticing this point, I Cc'd to systemtap. Is systemtap taking
care of spinlock too?

Thank you,

-- 
Masami Hiramatsu

Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-02 Thread Masami Hiramatsu 
On Wed, 2 Sep 2020 15:42:52 +0200
pet...@infradead.org wrote:

> On Wed, Sep 02, 2020 at 10:19:26PM +0900, Masami Hiramatsu wrote:
> > On Wed, 2 Sep 2020 11:36:13 +0200
> > pet...@infradead.org wrote:
> > 
> > > On Wed, Sep 02, 2020 at 05:17:55PM +0900, Masami Hiramatsu wrote:
> > > 
> > > > > Ok, but then lockdep will yell at you if you have that enabled and run
> > > > > the unoptimized things.
> > > > 
> > > > Oh, does it warn for all spinlock things in kprobes if it is 
> > > > unoptimized?
> > > > Hmm, it has to be noted in the documentation.
> > > 
> > > Lockdep will warn about spinlocks used in NMI context that are also used
> > > outside NMI context.
> > 
> > OK, but raw_spin_lock_irqsave() will not involve lockdep, correct?
> 
> It will. The distinction between spin_lock and raw_spin_lock is only
> that raw_spin_lock stays a spinlock on PREEMPT_RT, while spin_lock will
> turn into a (PI) mutex in that case.
> 
> But both will call into lockdep. Unlike local_irq_disable() and
> raw_local_irq_disable(), where the latter will not. Yes your prefixes
> are a mess :/

Yeah, that's really confusing...

> > > Now, for the kretprobe that kprobe_busy flag prevents the actual
> > > recursion self-deadlock, but lockdep isn't smart enough to see that.
> > > 
> > > One way around this might be to use SINGLE_DEPTH_NESTING for locks when
> > > we use them from INT3 context. That way they'll have a different class
> > > and lockdep will not see the recursion.
> > 
> > Hmm, so lockdep warns only when it detects the spinlock in NMI context,
> > and int3 is now always NMI, thus all spinlock (except raw_spinlock?)
> > in kprobe handlers should get warned, right?
> > I have tested this series up to [16/21] with optprobe disabled, but
> > I haven't see the lockdep warnings.
> 
> There's a bug, that might make it miss it. I have a patch. I'll send it
> shortly.

OK, I've confirmed that the lockdep warns on kretprobe from INT3
with your fix. Of course make it lockless then warning is gone.
But even without the lockless patch, this warning can be false-positive
because we prohibit nested kprobe call, right?

If the kprobe user handler uses a spinlock, the spinlock is used
only in that handler (and in the context between kprobe_busy_begin/end),
it will be safe since the spinlock is not nested.
But if the spinlock is shared with other context, it will be dangerous
because it can be interrupted by NMI (including INT3). This also applied
to the function which is called from kprobe user handlers, thus user
has to take care of it.
BTW, what would you think about setting NMI count between kprobe_busy_begin/end?

> 
> > > pre_handler_kretprobe() is always called from INT3, right?
> > 
> > No, not always, it can be called from optprobe (same as original code
> > context) or ftrace handler.
> > But if you set 0 to /proc/sys/debug/kprobe_optimization, and compile
> > the kernel without function tracer, it should always be called from
> > INT3.
> 
> D'oh, ofcourse! Arguably I should make the optprobe context NMI like
> too.. but that's for another day.

Hmm, we still have kprobe-on-ftrace. Would you consider we will
make it NMI like too? (and what the ftrace does for this)

Thank you,

-- 
Masami Hiramatsu

Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-02 Thread Masami Hiramatsu 
On Wed, 2 Sep 2020 11:36:13 +0200
pet...@infradead.org wrote:

> On Wed, Sep 02, 2020 at 05:17:55PM +0900, Masami Hiramatsu wrote:
> 
> > > Ok, but then lockdep will yell at you if you have that enabled and run
> > > the unoptimized things.
> > 
> > Oh, does it warn for all spinlock things in kprobes if it is unoptimized?
> > Hmm, it has to be noted in the documentation.
> 
> Lockdep will warn about spinlocks used in NMI context that are also used
> outside NMI context.

OK, but raw_spin_lock_irqsave() will not involve lockdep, correct?

> Now, for the kretprobe that kprobe_busy flag prevents the actual
> recursion self-deadlock, but lockdep isn't smart enough to see that.
> 
> One way around this might be to use SINGLE_DEPTH_NESTING for locks when
> we use them from INT3 context. That way they'll have a different class
> and lockdep will not see the recursion.

Hmm, so lockdep warns only when it detects the spinlock in NMI context,
and int3 is now always NMI, thus all spinlock (except raw_spinlock?)
in kprobe handlers should get warned, right?
I have tested this series up to [16/21] with optprobe disabled, but
I haven't see the lockdep warnings.

> 
> pre_handler_kretprobe() is always called from INT3, right?

No, not always, it can be called from optprobe (same as original code
context) or ftrace handler.
But if you set 0 to /proc/sys/debug/kprobe_optimization, and compile
the kernel without function tracer, it should always be called from
INT3.

> 
> Something like the below might then work...

OK, but I would like to reproduce the lockdep warning on this for
confirmation.

Thank you,

> 
> ---
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 287b263c9cb9..b78f4fe08e86 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1255,11 +1255,11 @@ __acquires(hlist_lock)
>  NOKPROBE_SYMBOL(kretprobe_hash_lock);
>  
>  static void kretprobe_table_lock(unsigned long hash,
> -  unsigned long *flags)
> +  unsigned long *flags, int nesting)
>  __acquires(hlist_lock)
>  {
>   raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
> - raw_spin_lock_irqsave(hlist_lock, *flags);
> + raw_spin_lock_irqsave_nested(hlist_lock, *flags, nesting);
>  }
>  NOKPROBE_SYMBOL(kretprobe_table_lock);
>  
> @@ -1326,7 +1326,7 @@ void kprobe_flush_task(struct task_struct *tk)
>   INIT_HLIST_HEAD(&empty_rp);
>   hash = hash_ptr(tk, KPROBE_HASH_BITS);
>   head = &kretprobe_inst_table[hash];
> - kretprobe_table_lock(hash, &flags);
> + kretprobe_table_lock(hash, &flags, 0);
>   hlist_for_each_entry_safe(ri, tmp, head, hlist) {
>   if (ri->task == tk)
>   recycle_rp_inst(ri, &empty_rp);
> @@ -1361,7 +1361,7 @@ static void cleanup_rp_inst(struct kretprobe *rp)
>  
>   /* No race here */
>   for (hash = 0; hash < KPROBE_TABLE_SIZE; hash++) {
> - kretprobe_table_lock(hash, &flags);
> + kretprobe_table_lock(hash, &flags, 0);
>   head = &kretprobe_inst_table[hash];
>   hlist_for_each_entry_safe(ri, next, head, hlist) {
>   if (ri->rp == rp)
> @@ -1950,7 +1950,7 @@ static int pre_handler_kretprobe(struct kprobe *p, 
> struct pt_regs *regs)
>  
>   /* TODO: consider to only swap the RA after the last pre_handler fired 
> */
>   hash = hash_ptr(current, KPROBE_HASH_BITS);
> - raw_spin_lock_irqsave(&rp->lock, flags);
> + raw_spin_lock_irqsave_nested(&rp->lock, flags, SINGLE_DEPTH_NESTING);
>   if (!hlist_empty(&rp->free_instances)) {
>   ri = hlist_entry(rp->free_instances.first,
>   struct kretprobe_instance, hlist);
> @@ -1961,7 +1961,7 @@ static int pre_handler_kretprobe(struct kprobe *p, 
> struct pt_regs *regs)
>   ri->task = current;
>  
>   if (rp->entry_handler && rp->entry_handler(ri, regs)) {
> - raw_spin_lock_irqsave(&rp->lock, flags);
> + raw_spin_lock_irqsave_nested(&rp->lock, flags, 
> SINGLE_DEPTH_NESTING);
>   hlist_add_head(&ri->hlist, &rp->free_instances);
>   raw_spin_unlock_irqrestore(&rp->lock, flags);
>   return 0;
> @@ -1971,7 +1971,7 @@ static int pre_handler_kretprobe(struct kprobe *p, 
> struct pt_regs *regs)
>  
>   /* XXX(hch): why is there no hlist_move_head? */
>   INIT_HLIST_NODE(&ri->hlist);
> - kretprobe_table_lock(hash, &flags);
> + kretprobe_table_lock(hash, &flags, SINGLE_DEPTH_NESTING);
>   hlist_add_head(&ri->hlist, &kretprobe_inst_table[hash]);
>   kretprobe_table_unlock(hash, &flags);
>   } else {


-- 
Masami Hiramatsu

Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-02 Thread peterz 
On Wed, Sep 02, 2020 at 10:19:26PM +0900, Masami Hiramatsu wrote:
> On Wed, 2 Sep 2020 11:36:13 +0200
> pet...@infradead.org wrote:
> 
> > On Wed, Sep 02, 2020 at 05:17:55PM +0900, Masami Hiramatsu wrote:
> > 
> > > > Ok, but then lockdep will yell at you if you have that enabled and run
> > > > the unoptimized things.
> > > 
> > > Oh, does it warn for all spinlock things in kprobes if it is unoptimized?
> > > Hmm, it has to be noted in the documentation.
> > 
> > Lockdep will warn about spinlocks used in NMI context that are also used
> > outside NMI context.
> 
> OK, but raw_spin_lock_irqsave() will not involve lockdep, correct?

It will. The distinction between spin_lock and raw_spin_lock is only
that raw_spin_lock stays a spinlock on PREEMPT_RT, while spin_lock will
turn into a (PI) mutex in that case.

But both will call into lockdep. Unlike local_irq_disable() and
raw_local_irq_disable(), where the latter will not. Yes your prefixes
are a mess :/

> > Now, for the kretprobe that kprobe_busy flag prevents the actual
> > recursion self-deadlock, but lockdep isn't smart enough to see that.
> > 
> > One way around this might be to use SINGLE_DEPTH_NESTING for locks when
> > we use them from INT3 context. That way they'll have a different class
> > and lockdep will not see the recursion.
> 
> Hmm, so lockdep warns only when it detects the spinlock in NMI context,
> and int3 is now always NMI, thus all spinlock (except raw_spinlock?)
> in kprobe handlers should get warned, right?
> I have tested this series up to [16/21] with optprobe disabled, but
> I haven't see the lockdep warnings.

There's a bug, that might make it miss it. I have a patch. I'll send it
shortly.

> > pre_handler_kretprobe() is always called from INT3, right?
> 
> No, not always, it can be called from optprobe (same as original code
> context) or ftrace handler.
> But if you set 0 to /proc/sys/debug/kprobe_optimization, and compile
> the kernel without function tracer, it should always be called from
> INT3.

D'oh, ofcourse! Arguably I should make the optprobe context NMI like
too.. but that's for another day.

Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-02 Thread peterz 
On Wed, Sep 02, 2020 at 05:17:55PM +0900, Masami Hiramatsu wrote:

> > Ok, but then lockdep will yell at you if you have that enabled and run
> > the unoptimized things.
> 
> Oh, does it warn for all spinlock things in kprobes if it is unoptimized?
> Hmm, it has to be noted in the documentation.

Lockdep will warn about spinlocks used in NMI context that are also used
outside NMI context.

Now, for the kretprobe that kprobe_busy flag prevents the actual
recursion self-deadlock, but lockdep isn't smart enough to see that.

One way around this might be to use SINGLE_DEPTH_NESTING for locks when
we use them from INT3 context. That way they'll have a different class
and lockdep will not see the recursion.

pre_handler_kretprobe() is always called from INT3, right?

Something like the below might then work...

---
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 287b263c9cb9..b78f4fe08e86 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1255,11 +1255,11 @@ __acquires(hlist_lock)
 NOKPROBE_SYMBOL(kretprobe_hash_lock);
 
 static void kretprobe_table_lock(unsigned long hash,
-unsigned long *flags)
+unsigned long *flags, int nesting)
 __acquires(hlist_lock)
 {
raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
-   raw_spin_lock_irqsave(hlist_lock, *flags);
+   raw_spin_lock_irqsave_nested(hlist_lock, *flags, nesting);
 }
 NOKPROBE_SYMBOL(kretprobe_table_lock);
 
@@ -1326,7 +1326,7 @@ void kprobe_flush_task(struct task_struct *tk)
INIT_HLIST_HEAD(&empty_rp);
hash = hash_ptr(tk, KPROBE_HASH_BITS);
head = &kretprobe_inst_table[hash];
-   kretprobe_table_lock(hash, &flags);
+   kretprobe_table_lock(hash, &flags, 0);
hlist_for_each_entry_safe(ri, tmp, head, hlist) {
if (ri->task == tk)
recycle_rp_inst(ri, &empty_rp);
@@ -1361,7 +1361,7 @@ static void cleanup_rp_inst(struct kretprobe *rp)
 
/* No race here */
for (hash = 0; hash < KPROBE_TABLE_SIZE; hash++) {
-   kretprobe_table_lock(hash, &flags);
+   kretprobe_table_lock(hash, &flags, 0);
head = &kretprobe_inst_table[hash];
hlist_for_each_entry_safe(ri, next, head, hlist) {
if (ri->rp == rp)
@@ -1950,7 +1950,7 @@ static int pre_handler_kretprobe(struct kprobe *p, struct 
pt_regs *regs)
 
/* TODO: consider to only swap the RA after the last pre_handler fired 
*/
hash = hash_ptr(current, KPROBE_HASH_BITS);
-   raw_spin_lock_irqsave(&rp->lock, flags);
+   raw_spin_lock_irqsave_nested(&rp->lock, flags, SINGLE_DEPTH_NESTING);
if (!hlist_empty(&rp->free_instances)) {
ri = hlist_entry(rp->free_instances.first,
struct kretprobe_instance, hlist);
@@ -1961,7 +1961,7 @@ static int pre_handler_kretprobe(struct kprobe *p, struct 
pt_regs *regs)
ri->task = current;
 
if (rp->entry_handler && rp->entry_handler(ri, regs)) {
-   raw_spin_lock_irqsave(&rp->lock, flags);
+   raw_spin_lock_irqsave_nested(&rp->lock, flags, 
SINGLE_DEPTH_NESTING);
hlist_add_head(&ri->hlist, &rp->free_instances);
raw_spin_unlock_irqrestore(&rp->lock, flags);
return 0;
@@ -1971,7 +1971,7 @@ static int pre_handler_kretprobe(struct kprobe *p, struct 
pt_regs *regs)
 
/* XXX(hch): why is there no hlist_move_head? */
INIT_HLIST_NODE(&ri->hlist);
-   kretprobe_table_lock(hash, &flags);
+   kretprobe_table_lock(hash, &flags, SINGLE_DEPTH_NESTING);
hlist_add_head(&ri->hlist, &kretprobe_inst_table[hash]);
kretprobe_table_unlock(hash, &flags);
} else {

Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-02 Thread Masami Hiramatsu 
On Wed, 2 Sep 2020 09:02:26 +0200
pet...@infradead.org wrote:

> On Wed, Sep 02, 2020 at 09:37:39AM +0900, Masami Hiramatsu wrote:
> > On Tue, 1 Sep 2020 21:08:08 +0200
> > Peter Zijlstra  wrote:
> > 
> > > On Sat, Aug 29, 2020 at 09:59:49PM +0900, Masami Hiramatsu wrote:
> > > > Masami Hiramatsu (16):
> > > >   kprobes: Add generic kretprobe trampoline handler
> > > >   x86/kprobes: Use generic kretprobe trampoline handler
> > > >   arm: kprobes: Use generic kretprobe trampoline handler
> > > >   arm64: kprobes: Use generic kretprobe trampoline handler
> > > >   arc: kprobes: Use generic kretprobe trampoline handler
> > > >   csky: kprobes: Use generic kretprobe trampoline handler
> > > >   ia64: kprobes: Use generic kretprobe trampoline handler
> > > >   mips: kprobes: Use generic kretprobe trampoline handler
> > > >   parisc: kprobes: Use generic kretprobe trampoline handler
> > > >   powerpc: kprobes: Use generic kretprobe trampoline handler
> > > >   s390: kprobes: Use generic kretprobe trampoline handler
> > > >   sh: kprobes: Use generic kretprobe trampoline handler
> > > >   sparc: kprobes: Use generic kretprobe trampoline handler
> > > >   kprobes: Remove NMI context check
> > > >   kprobes: Free kretprobe_instance with rcu callback
> > > >   kprobes: Make local used functions static
> > > > 
> > > > Peter Zijlstra (5):
> > > >   llist: Add nonatomic __llist_add() and __llist_dell_all()
> > > >   kprobes: Remove kretprobe hash
> > > >   asm-generic/atomic: Add try_cmpxchg() fallbacks
> > > >   freelist: Lock less freelist
> > > >   kprobes: Replace rp->free_instance with freelist
> > > 
> > > This looks good to me, do you want me to merge them through -tip? If so,
> > > do we want to try and get them in this release still?
> > 
> > Yes, thanks. For the kretprobe missing issue, we will need the first half
> > (up to "kprobes: Remove NMI context check"), so we can split the series
> > if someone think the lockless is still immature.
> 
> Ok, but then lockdep will yell at you if you have that enabled and run
> the unoptimized things.

Oh, does it warn for all spinlock things in kprobes if it is unoptimized?
Hmm, it has to be noted in the documentation.

Thank you,


-- 
Masami Hiramatsu

Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-02 Thread peterz 
On Wed, Sep 02, 2020 at 09:37:39AM +0900, Masami Hiramatsu wrote:
> On Tue, 1 Sep 2020 21:08:08 +0200
> Peter Zijlstra  wrote:
> 
> > On Sat, Aug 29, 2020 at 09:59:49PM +0900, Masami Hiramatsu wrote:
> > > Masami Hiramatsu (16):
> > >   kprobes: Add generic kretprobe trampoline handler
> > >   x86/kprobes: Use generic kretprobe trampoline handler
> > >   arm: kprobes: Use generic kretprobe trampoline handler
> > >   arm64: kprobes: Use generic kretprobe trampoline handler
> > >   arc: kprobes: Use generic kretprobe trampoline handler
> > >   csky: kprobes: Use generic kretprobe trampoline handler
> > >   ia64: kprobes: Use generic kretprobe trampoline handler
> > >   mips: kprobes: Use generic kretprobe trampoline handler
> > >   parisc: kprobes: Use generic kretprobe trampoline handler
> > >   powerpc: kprobes: Use generic kretprobe trampoline handler
> > >   s390: kprobes: Use generic kretprobe trampoline handler
> > >   sh: kprobes: Use generic kretprobe trampoline handler
> > >   sparc: kprobes: Use generic kretprobe trampoline handler
> > >   kprobes: Remove NMI context check
> > >   kprobes: Free kretprobe_instance with rcu callback
> > >   kprobes: Make local used functions static
> > > 
> > > Peter Zijlstra (5):
> > >   llist: Add nonatomic __llist_add() and __llist_dell_all()
> > >   kprobes: Remove kretprobe hash
> > >   asm-generic/atomic: Add try_cmpxchg() fallbacks
> > >   freelist: Lock less freelist
> > >   kprobes: Replace rp->free_instance with freelist
> > 
> > This looks good to me, do you want me to merge them through -tip? If so,
> > do we want to try and get them in this release still?
> 
> Yes, thanks. For the kretprobe missing issue, we will need the first half
> (up to "kprobes: Remove NMI context check"), so we can split the series
> if someone think the lockless is still immature.

Ok, but then lockdep will yell at you if you have that enabled and run
the unoptimized things.

> > Ingo, opinions? This basically fixes a regression cauesd by
> > 
> >   0d00449c7a28 ("x86: Replace ist_enter() with nmi_enter()")
> > 
> 
> Oops, I missed Ingo in CC. 

You had x86@, he'll have a copy :-)

Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-01 Thread Masami Hiramatsu 
On Tue, 1 Sep 2020 21:08:08 +0200
Peter Zijlstra  wrote:

> On Sat, Aug 29, 2020 at 09:59:49PM +0900, Masami Hiramatsu wrote:
> > Masami Hiramatsu (16):
> >   kprobes: Add generic kretprobe trampoline handler
> >   x86/kprobes: Use generic kretprobe trampoline handler
> >   arm: kprobes: Use generic kretprobe trampoline handler
> >   arm64: kprobes: Use generic kretprobe trampoline handler
> >   arc: kprobes: Use generic kretprobe trampoline handler
> >   csky: kprobes: Use generic kretprobe trampoline handler
> >   ia64: kprobes: Use generic kretprobe trampoline handler
> >   mips: kprobes: Use generic kretprobe trampoline handler
> >   parisc: kprobes: Use generic kretprobe trampoline handler
> >   powerpc: kprobes: Use generic kretprobe trampoline handler
> >   s390: kprobes: Use generic kretprobe trampoline handler
> >   sh: kprobes: Use generic kretprobe trampoline handler
> >   sparc: kprobes: Use generic kretprobe trampoline handler
> >   kprobes: Remove NMI context check
> >   kprobes: Free kretprobe_instance with rcu callback
> >   kprobes: Make local used functions static
> > 
> > Peter Zijlstra (5):
> >   llist: Add nonatomic __llist_add() and __llist_dell_all()
> >   kprobes: Remove kretprobe hash
> >   asm-generic/atomic: Add try_cmpxchg() fallbacks
> >   freelist: Lock less freelist
> >   kprobes: Replace rp->free_instance with freelist
> 
> This looks good to me, do you want me to merge them through -tip? If so,
> do we want to try and get them in this release still?

Yes, thanks. For the kretprobe missing issue, we will need the first half
(up to "kprobes: Remove NMI context check"), so we can split the series
if someone think the lockless is still immature.

> 
> Ingo, opinions? This basically fixes a regression cauesd by
> 
>   0d00449c7a28 ("x86: Replace ist_enter() with nmi_enter()")
> 

Oops, I missed Ingo in CC. 

Thank you,

-- 
Masami Hiramatsu

Re: [PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-09-01 Thread Peter Zijlstra 
On Sat, Aug 29, 2020 at 09:59:49PM +0900, Masami Hiramatsu wrote:
> Masami Hiramatsu (16):
>   kprobes: Add generic kretprobe trampoline handler
>   x86/kprobes: Use generic kretprobe trampoline handler
>   arm: kprobes: Use generic kretprobe trampoline handler
>   arm64: kprobes: Use generic kretprobe trampoline handler
>   arc: kprobes: Use generic kretprobe trampoline handler
>   csky: kprobes: Use generic kretprobe trampoline handler
>   ia64: kprobes: Use generic kretprobe trampoline handler
>   mips: kprobes: Use generic kretprobe trampoline handler
>   parisc: kprobes: Use generic kretprobe trampoline handler
>   powerpc: kprobes: Use generic kretprobe trampoline handler
>   s390: kprobes: Use generic kretprobe trampoline handler
>   sh: kprobes: Use generic kretprobe trampoline handler
>   sparc: kprobes: Use generic kretprobe trampoline handler
>   kprobes: Remove NMI context check
>   kprobes: Free kretprobe_instance with rcu callback
>   kprobes: Make local used functions static
> 
> Peter Zijlstra (5):
>   llist: Add nonatomic __llist_add() and __llist_dell_all()
>   kprobes: Remove kretprobe hash
>   asm-generic/atomic: Add try_cmpxchg() fallbacks
>   freelist: Lock less freelist
>   kprobes: Replace rp->free_instance with freelist

This looks good to me, do you want me to merge them through -tip? If so,
do we want to try and get them in this release still?

Ingo, opinions? This basically fixes a regression cauesd by

  0d00449c7a28 ("x86: Replace ist_enter() with nmi_enter()")

[PATCH v5 00/21] kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

2020-08-29 Thread Masami Hiramatsu 
Hi,

Here is the 5th version of the series to unify the kretprobe trampoline handler
and to make the kretprobe lockless. Thanks Peter for this work !!

Previous version is here;

 
https://lkml.kernel.org/r/159861759775.992023.12553306821235086809.stgit@devnote2

This version merges the remove-task-scan patch into remove kretprobe hash
patch, fixes code according to the comments, and fixes a minor issues.

This version is also available on
git://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git 
lockless-kretprobe-v5

I ran smoke test and ftracetest on x86-64 and did build tests for x86-64, i386, 
arm,
arm64, and mips.


Thank you,

---

Masami Hiramatsu (16):
  kprobes: Add generic kretprobe trampoline handler
  x86/kprobes: Use generic kretprobe trampoline handler
  arm: kprobes: Use generic kretprobe trampoline handler
  arm64: kprobes: Use generic kretprobe trampoline handler
  arc: kprobes: Use generic kretprobe trampoline handler
  csky: kprobes: Use generic kretprobe trampoline handler
  ia64: kprobes: Use generic kretprobe trampoline handler
  mips: kprobes: Use generic kretprobe trampoline handler
  parisc: kprobes: Use generic kretprobe trampoline handler
  powerpc: kprobes: Use generic kretprobe trampoline handler
  s390: kprobes: Use generic kretprobe trampoline handler
  sh: kprobes: Use generic kretprobe trampoline handler
  sparc: kprobes: Use generic kretprobe trampoline handler
  kprobes: Remove NMI context check
  kprobes: Free kretprobe_instance with rcu callback
  kprobes: Make local used functions static

Peter Zijlstra (5):
  llist: Add nonatomic __llist_add() and __llist_dell_all()
  kprobes: Remove kretprobe hash
  asm-generic/atomic: Add try_cmpxchg() fallbacks
  freelist: Lock less freelist
  kprobes: Replace rp->free_instance with freelist


 arch/arc/kernel/kprobes.c |   54 --
 arch/arm/probes/kprobes/core.c|   78 -
 arch/arm64/kernel/probes/kprobes.c|   78 -
 arch/csky/kernel/probes/kprobes.c |   77 
 arch/ia64/kernel/kprobes.c|   77 
 arch/mips/kernel/kprobes.c|   54 --
 arch/parisc/kernel/kprobes.c  |   76 
 arch/powerpc/kernel/kprobes.c |   53 --
 arch/s390/kernel/kprobes.c|   79 -
 arch/sh/kernel/kprobes.c  |   58 --
 arch/sparc/kernel/kprobes.c   |   51 --
 arch/x86/include/asm/atomic.h |2 
 arch/x86/include/asm/atomic64_64.h|2 
 arch/x86/include/asm/cmpxchg.h|2 
 arch/x86/kernel/kprobes/core.c|  108 
 drivers/gpu/drm/i915/i915_request.c   |6 -
 include/asm-generic/atomic-instrumented.h |  216 ++--
 include/linux/atomic-arch-fallback.h  |   90 +-
 include/linux/atomic-fallback.h   |   90 +-
 include/linux/freelist.h  |  129 ++
 include/linux/kprobes.h   |   74 +---
 include/linux/llist.h |   23 +++
 include/linux/sched.h |4 
 kernel/fork.c |4 
 kernel/kprobes.c  |  264 +
 kernel/trace/trace_kprobe.c   |3 
 scripts/atomic/gen-atomic-fallback.sh |   63 ++-
 scripts/atomic/gen-atomic-instrumented.sh |   29 +++
 28 files changed, 735 insertions(+), 1109 deletions(-)
 create mode 100644 include/linux/freelist.h

--
Masami Hiramatsu (Linaro)