Re: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash

2020-08-28 Thread Masami Hiramatsu 
On Fri, 28 Aug 2020 13:11:15 +
"eddy...@trendmicro.com"  wrote:

> > -Original Message
> Hi, I found a NULL pointer dereference here, where 
> current->kretprobe_instances.first == NULL in these two scenario:
> 
> 1) In task "rs:main Q:Reg"
> # insmod samples/kprobes/kretprobe_example.ko func=schedule
> # pkill sddm-greeter
> 
> 2) In task "llvmpipe-10"
> # insmod samples/kprobes/kretprobe_example.ko func=schedule
> login plasmashell session from sddm graphical interface
> 
> based on Masami's v2 + Peter's lockless patch, I'll try the new branch once I 
> can compile kernel
> 
> Stacktrace may not be really useful here:
> [  402.008630] BUG: kernel NULL pointer dereference, address: 0018
> [  402.008633] #PF: supervisor read access in kernel mode
> [  402.008642] #PF: error_code(0x) - not-present page
> [  402.008644] PGD 0 P4D 0
> [  402.008646] Oops:  [#1] PREEMPT SMP PTI
> [  402.008649] CPU: 7 PID: 1505 Comm: llvmpipe-10 Kdump: loaded Not tainted 
> 5.9.0-rc2-00111-g72091ec08f03-dirty #45

Hmm, this case llvmpipe will be the user task (not kthread, I guess)

Here are some logs, both happened with following command and wait 5min or so.

cd /sys/kernel/debug/tracing/
echo r:event1 vfs_read >> kprobe_events
echo r:event2 vfs_read %ax >> kprobe_events
echo r:event3 rw_verify_area %ax >> kprobe_events
echo r:schedule schedule >> kprobe_events
echo 1 > events/kprobes/enable


[  332.986337] [ cut here ]
[  332.987312] kernel BUG at kernel/kprobes.c:1893!
[  332.988237] invalid opcode:  [#1] PREEMPT SMP PTI
[  332.989108] CPU: 7 PID: 55 Comm: kcompactd0 Not tainted 5.9.0-rc2+ #54
[  332.990480] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.13.0-1ubuntu1 04/01/2014
[  332.994600] RIP: 0010:__kretprobe_trampoline_handler+0xf2/0x100
[  332.995551] Code: 48 c7 05 e5 40 ec 7e c0 cc 28 82 4c 89 ff e8 c5 fe ff ff 
48 85 db 75 92 48 83 c4 08 4c 89 e0 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b 66 
66 2e 0f 1f 84 00 00 00 00 00 90 55 48 89 e5 41 56 41 55
[  332.998498] RSP: :c9217cf8 EFLAGS: 00010246
[  332.999405] RAX: 88807cfe9700 RBX:  RCX: 
[  333.000597] RDX: c9217de8 RSI: 810471e0 RDI: c9217d50
[  333.002058] RBP: c9217d28 R08: 0001 R09: 0001
[  333.003594] R10:  R11: 0001 R12: c9217d50
[  333.005219] R13: 88807d7dbac0 R14: c9217e00 R15: 88807d7dbac0
[  333.006826] FS:  () GS:88807d7c() 
knlGS:
[  333.008787] CS:  0010 DS:  ES:  CR0: 80050033
[  333.010249] CR2:  CR3: 0222 CR4: 06a0
[  333.011895] Call Trace:
[  333.012529]  trampoline_handler+0x43/0x60
[  333.013214]  kretprobe_trampoline+0x2a/0x50
[  333.014028] RIP: 0010:kretprobe_trampoline+0x0/0x50
[  333.014856] Code: c7 e9 2d 04 82 e8 a0 f2 0d 00 5d c3 31 f6 e9 79 ff ff ff 
be 01 00 00 00 e9 6f ff ff ff cc cc cc cc cc cc cc cc cc cc cc cc cc <54> 9c 48 
83 ec 18 57 56 52 51 50 41 50 41 51 41 52 41 53 53 55 41
[  333.017750] RSP: 81170fba:c9217df0 EFLAGS: 0246
[  333.018894] RAX: 40200040 RBX: 88807d7dbac0 RCX: 
[  333.020232] RDX: 0001 RSI: 818e51b4 RDI: 818e51b4
[  333.021476] RBP: c9217e88 R08: 0001 R09: 0001
[  333.022603] R10:  R11: 0001 R12: 00018044
[  333.024221] R13: 88807d7dbac0 R14: c9217e00 R15: 88807d7dbac0
[  333.025851]  ? schedule+0x54/0x100
[  333.026717]  ? schedule+0x54/0x100
[  333.027400]  ? trace_preempt_on+0x2a/0xd0
[  333.028161]  ? __next_timer_interrupt+0x110/0x110
[  333.029080]  kcompactd+0x20e/0x350
[  333.029882]  ? wait_woken+0x80/0x80
[  333.030593]  ? kcompactd_do_work+0x3a0/0x3a0
[  333.031347]  kthread+0x13c/0x180
[  333.031988]  ? kthread_park+0x90/0x90
[  333.032734]  ret_from_fork+0x22/0x30
[  333.033557] Modules linked in:
[  333.034451] ---[ end trace 901e8137e8d04982 ]---
[  333.035601] RIP: 0010:__kretprobe_trampoline_handler+0xf2/0x100
[  333.037073] Code: 48 c7 05 e5 40 ec 7e c0 cc 28 82 4c 89 ff e8 c5 fe ff ff 
48 85 db 75 92 48 83 c4 08 4c 89 e0 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b 66 
66 2e 0f 1f 84 00 00 00 00 00 90 55 48 89 e5 41 56 41 55
[  333.041089] RSP: :c9217cf8 EFLAGS: 00010246
[  333.042201] RAX: 88807cfe9700 RBX:  RCX: 
[  333.043747] RDX: c9217de8 RSI: 810471e0 RDI: c9217d50
[  333.045063] RBP: c9217d28 R08: 0001 R09: 0001
[  333.046547] R10:  R11: 0001 R12: c9217d50
[  333.048055] R13: 88807d7dbac0 R14: c9217e00 R15: 88807d7dbac0
[  333.049616] FS:  () GS:88807d7c() 
knlGS:
[  333.051487] CS:  0010 DS:  ES:

Re: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash

2020-08-28 Thread Masami Hiramatsu 
On Fri, 28 Aug 2020 16:19:17 +0200
pet...@infradead.org wrote:

> On Fri, Aug 28, 2020 at 02:11:18PM +, eddy...@trendmicro.com wrote:
> > > From: Masami Hiramatsu 
> > >
> > > OK, schedule function will be the key. I guess the senario is..
> > >
> > > 1) kretporbe replace the return address with kretprobe_trampoline on 
> > > task1's kernel stack
> > > 2) the task1 forks task2 before returning to the kretprobe_trampoline
> > > 3) while copying the process with the kernel stack, 
> > > task2->kretprobe_instances.first = NULL
> > 
> > I think new process created by fork/clone uses a brand new kernel
> > stack? I thought only user stack are copied.  Otherwise any process
> > launch should crash in the same way
> 
> I was under the same impression, we create a brand new stack-frame for
> the new task, this 'fake' frame we can schedule into.
> 
> It either points to ret_from_fork() for new user tasks, or
> kthread_frame_init() for kernel threads.

Ah sorry, then it's my misreading... anyway, I could reproduce the crash with
probing on schedule(). Hmm, it is better to dump the current comm with
BUG().

Thank you,



-- 
Masami Hiramatsu

Re: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash

2020-08-28 Thread Masami Hiramatsu 
On Fri, 28 Aug 2020 15:58:24 +0200
pet...@infradead.org wrote:

> On Fri, Aug 28, 2020 at 10:51:13PM +0900, Masami Hiramatsu wrote:
>  
> > OK, schedule function will be the key. I guess the senario is..
> > 
> > 1) kretporbe replace the return address with kretprobe_trampoline on 
> > task1's kernel stack
> > 2) the task1 forks task2 before returning to the kretprobe_trampoline
> > 3) while copying the process with the kernel stack, 
> > task2->kretprobe_instances.first = NULL
> > 4) task2 returns to the kretprobe_trampoline
> > 5) Bomb!
> > 
> > Hmm, we need to fixup the kernel stack when copying process. 
> 
> How would that scenario have been avoided in the old code? Because there
> task2 would have a different has and not have found a kretprobe_instance
> either.

Good question, I think this bug has not been solved in old code too.
Let me check.

Thanks,

-- 
Masami Hiramatsu

Re: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash

2020-08-28 Thread peterz 
On Fri, Aug 28, 2020 at 02:11:18PM +, eddy...@trendmicro.com wrote:
> > From: Masami Hiramatsu 
> >
> > OK, schedule function will be the key. I guess the senario is..
> >
> > 1) kretporbe replace the return address with kretprobe_trampoline on 
> > task1's kernel stack
> > 2) the task1 forks task2 before returning to the kretprobe_trampoline
> > 3) while copying the process with the kernel stack, 
> > task2->kretprobe_instances.first = NULL
> 
> I think new process created by fork/clone uses a brand new kernel
> stack? I thought only user stack are copied.  Otherwise any process
> launch should crash in the same way

I was under the same impression, we create a brand new stack-frame for
the new task, this 'fake' frame we can schedule into.

It either points to ret_from_fork() for new user tasks, or
kthread_frame_init() for kernel threads.

RE: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash

2020-08-28 Thread eddy...@trendmicro.com 
> From: Masami Hiramatsu 
>
> OK, schedule function will be the key. I guess the senario is..
>
> 1) kretporbe replace the return address with kretprobe_trampoline on task1's 
> kernel stack
> 2) the task1 forks task2 before returning to the kretprobe_trampoline
> 3) while copying the process with the kernel stack, 
> task2->kretprobe_instances.first = NULL

I think new process created by fork/clone uses a brand new kernel stack? I 
thought only user stack are copied.
Otherwise any process launch should crash in the same way

By the way, I can reproduce this on the latest branch(v4)
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: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash

2020-08-28 Thread peterz 
On Fri, Aug 28, 2020 at 10:51:13PM +0900, Masami Hiramatsu wrote:
 
> OK, schedule function will be the key. I guess the senario is..
> 
> 1) kretporbe replace the return address with kretprobe_trampoline on task1's 
> kernel stack
> 2) the task1 forks task2 before returning to the kretprobe_trampoline
> 3) while copying the process with the kernel stack, 
> task2->kretprobe_instances.first = NULL
> 4) task2 returns to the kretprobe_trampoline
> 5) Bomb!
> 
> Hmm, we need to fixup the kernel stack when copying process. 

How would that scenario have been avoided in the old code? Because there
task2 would have a different has and not have found a kretprobe_instance
either.

Re: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash

2020-08-28 Thread Masami Hiramatsu 
On Fri, 28 Aug 2020 13:11:15 +
"eddy...@trendmicro.com"  wrote:

> > -Original Message-
> > From: Peter Zijlstra 
> > Sent: Friday, August 28, 2020 12:13 AM
> > To: linux-kernel@vger.kernel.org; mhira...@kernel.org
> > Cc: Eddy Wu (RD-TW) ; x...@kernel.org; 
> > da...@davemloft.net; rost...@goodmis.org;
> > naveen.n@linux.ibm.com; anil.s.keshavamur...@intel.com; 
> > linux-a...@vger.kernel.org; came...@moodycamel.com;
> > o...@redhat.com; w...@kernel.org; paul...@kernel.org; pet...@infradead.org
> > Subject: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash
> >
> > @@ -1935,71 +1932,45 @@ unsigned long __kretprobe_trampoline_han
> > unsigned long trampoline_address,
> > void *frame_pointer)
> >  {
> > // ... removed
> > // NULL here
> > +   first = node = current->kretprobe_instances.first;
> > +   while (node) {
> > +   ri = container_of(node, struct kretprobe_instance, llist);
> >
> > -   orig_ret_address = (unsigned long)ri->ret_addr;
> > -   if (skipped)
> > -   pr_warn("%ps must be blacklisted because of 
> > incorrect kretprobe order\n",
> > -   ri->rp->kp.addr);
> > +   BUG_ON(ri->fp != frame_pointer);
> >
> > -   if (orig_ret_address != trampoline_address)
> > +   orig_ret_address = (unsigned long)ri->ret_addr;
> > +   if (orig_ret_address != trampoline_address) {
> > /*
> >  * This is the real return address. Any other
> >  * instances associated with this task are for
> >  * other calls deeper on the call stack
> >  */
> > break;
> > +   }
> > +
> > +   node = node->next;
> > }
> >
> 
> Hi, I found a NULL pointer dereference here, where 
> current->kretprobe_instances.first == NULL in these two scenario:

Thanks! that may be what I'm chasing.

> 
> 1) In task "rs:main Q:Reg"
> # insmod samples/kprobes/kretprobe_example.ko func=schedule
> # pkill sddm-greeter
> 
> 2) In task "llvmpipe-10"
> # insmod samples/kprobes/kretprobe_example.ko func=schedule
> login plasmashell session from sddm graphical interface

OK, schedule function will be the key. I guess the senario is..

1) kretporbe replace the return address with kretprobe_trampoline on task1's 
kernel stack
2) the task1 forks task2 before returning to the kretprobe_trampoline
3) while copying the process with the kernel stack, 
task2->kretprobe_instances.first = NULL
4) task2 returns to the kretprobe_trampoline
5) Bomb!

Hmm, we need to fixup the kernel stack when copying process. 

Thank you,

> 
> based on Masami's v2 + Peter's lockless patch, I'll try the new branch once I 
> can compile kernel
> 
> Stacktrace may not be really useful here:
> [  402.008630] BUG: kernel NULL pointer dereference, address: 0018
> [  402.008633] #PF: supervisor read access in kernel mode
> [  402.008642] #PF: error_code(0x) - not-present page
> [  402.008644] PGD 0 P4D 0
> [  402.008646] Oops:  [#1] PREEMPT SMP PTI
> [  402.008649] CPU: 7 PID: 1505 Comm: llvmpipe-10 Kdump: loaded Not tainted 
> 5.9.0-rc2-00111-g72091ec08f03-dirty #45
> [  402.008650] Hardware name: VMware, Inc. VMware Virtual Platform/440BX 
> Desktop Reference Platform, BIOS 6.00 07/29/2019
> [  402.008653] RIP: 0010:__kretprobe_trampoline_handler+0xb8/0x17f
> [  402.008655] Code: 65 4c 8b 34 25 80 6d 01 00 4c 89 e2 48 c7 c7 91 6b 85 91 
> 49 8d b6 38 07 00 00 e8 d1 1a f9 ff 48 85 db 74 06 48 3b 5d d0 75 16 <49> 8b 
> 75 18 48 c7 c7 a0 6c 85 91 48
>  8b 56 28 e8 b2 1a f9 ff 0f 0b
> [  402.008655] RSP: 0018:ab408147bde0 EFLAGS: 00010246
> [  402.008656] RAX: 0021 RBX:  RCX: 
> 0002
> [  402.008657] RDX: 8002 RSI: 9189757d RDI: 
> 
> [  402.008658] RBP: ab408147be20 R08: 0001 R09: 
> 955c
> [  402.008658] R10: 0004 R11:  R12: 
> 
> [  402.008659] R13:  R14: 90736d305f40 R15: 
> 
> [  402.008661] FS:  7f20f6ffd700() GS:9073781c() 
> knlGS:
> [  402.008675] CS:  0010 DS:  ES:  CR0: 80050033
> [  402.008678] CR2: 0018 CR3: 0001ed256006 CR4: 
> 003706e0
> [  402.008684] Call Trace:
> [  402.008689]  ? elfcorehdr_read+0x40/0x40
> [  402.008690]  ? elfcorehdr_read+0x40/0x40
> [  402.008691]  trampoline_handler+0x42/0x60
> [  402.008692]  kretprobe_trampoline+0x2a/0x50
> [  402.008693] RIP: 0010:kretprobe_trampoline+0x0/0x50
> 
> 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

Re: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash

2020-08-28 Thread peterz 
On Fri, Aug 28, 2020 at 01:11:15PM +, eddy...@trendmicro.com wrote:
> > -Original Message-
> > From: Peter Zijlstra 
> > Sent: Friday, August 28, 2020 12:13 AM
> > To: linux-kernel@vger.kernel.org; mhira...@kernel.org
> > Cc: Eddy Wu (RD-TW) ; x...@kernel.org; 
> > da...@davemloft.net; rost...@goodmis.org;
> > naveen.n@linux.ibm.com; anil.s.keshavamur...@intel.com; 
> > linux-a...@vger.kernel.org; came...@moodycamel.com;
> > o...@redhat.com; w...@kernel.org; paul...@kernel.org; pet...@infradead.org
> > Subject: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash
> >
> > @@ -1935,71 +1932,45 @@ unsigned long __kretprobe_trampoline_han
> > unsigned long trampoline_address,
> > void *frame_pointer)
> >  {
> > // ... removed
> > // NULL here
> > +   first = node = current->kretprobe_instances.first;
> > +   while (node) {
> > +   ri = container_of(node, struct kretprobe_instance, llist);
> >
> > -   orig_ret_address = (unsigned long)ri->ret_addr;
> > -   if (skipped)
> > -   pr_warn("%ps must be blacklisted because of 
> > incorrect kretprobe order\n",
> > -   ri->rp->kp.addr);
> > +   BUG_ON(ri->fp != frame_pointer);
> >
> > -   if (orig_ret_address != trampoline_address)
> > +   orig_ret_address = (unsigned long)ri->ret_addr;
> > +   if (orig_ret_address != trampoline_address) {
> > /*
> >  * This is the real return address. Any other
> >  * instances associated with this task are for
> >  * other calls deeper on the call stack
> >  */
> > break;
> > +   }
> > +
> > +   node = node->next;
> > }
> >
> 
> Hi, I found a NULL pointer dereference here, where
> current->kretprobe_instances.first == NULL in these two scenario:

Hurmph, that would mean hitting the trampoline and not having a
kretprobe_instance, weird. Let me try and reproduce.

RE: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash

2020-08-28 Thread eddy...@trendmicro.com 
> -Original Message-
> From: Peter Zijlstra 
> Sent: Friday, August 28, 2020 12:13 AM
> To: linux-kernel@vger.kernel.org; mhira...@kernel.org
> Cc: Eddy Wu (RD-TW) ; x...@kernel.org; 
> da...@davemloft.net; rost...@goodmis.org;
> naveen.n@linux.ibm.com; anil.s.keshavamur...@intel.com; 
> linux-a...@vger.kernel.org; came...@moodycamel.com;
> o...@redhat.com; w...@kernel.org; paul...@kernel.org; pet...@infradead.org
> Subject: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash
>
> @@ -1935,71 +1932,45 @@ unsigned long __kretprobe_trampoline_han
> unsigned long trampoline_address,
> void *frame_pointer)
>  {
> // ... removed
> // NULL here
> +   first = node = current->kretprobe_instances.first;
> +   while (node) {
> +   ri = container_of(node, struct kretprobe_instance, llist);
>
> -   orig_ret_address = (unsigned long)ri->ret_addr;
> -   if (skipped)
> -   pr_warn("%ps must be blacklisted because of incorrect 
> kretprobe order\n",
> -   ri->rp->kp.addr);
> +   BUG_ON(ri->fp != frame_pointer);
>
> -   if (orig_ret_address != trampoline_address)
> +   orig_ret_address = (unsigned long)ri->ret_addr;
> +   if (orig_ret_address != trampoline_address) {
> /*
>  * This is the real return address. Any other
>  * instances associated with this task are for
>  * other calls deeper on the call stack
>  */
> break;
> +   }
> +
> +   node = node->next;
> }
>

Hi, I found a NULL pointer dereference here, where 
current->kretprobe_instances.first == NULL in these two scenario:

1) In task "rs:main Q:Reg"
# insmod samples/kprobes/kretprobe_example.ko func=schedule
# pkill sddm-greeter

2) In task "llvmpipe-10"
# insmod samples/kprobes/kretprobe_example.ko func=schedule
login plasmashell session from sddm graphical interface

based on Masami's v2 + Peter's lockless patch, I'll try the new branch once I 
can compile kernel

Stacktrace may not be really useful here:
[  402.008630] BUG: kernel NULL pointer dereference, address: 0018
[  402.008633] #PF: supervisor read access in kernel mode
[  402.008642] #PF: error_code(0x) - not-present page
[  402.008644] PGD 0 P4D 0
[  402.008646] Oops:  [#1] PREEMPT SMP PTI
[  402.008649] CPU: 7 PID: 1505 Comm: llvmpipe-10 Kdump: loaded Not tainted 
5.9.0-rc2-00111-g72091ec08f03-dirty #45
[  402.008650] Hardware name: VMware, Inc. VMware Virtual Platform/440BX 
Desktop Reference Platform, BIOS 6.00 07/29/2019
[  402.008653] RIP: 0010:__kretprobe_trampoline_handler+0xb8/0x17f
[  402.008655] Code: 65 4c 8b 34 25 80 6d 01 00 4c 89 e2 48 c7 c7 91 6b 85 91 
49 8d b6 38 07 00 00 e8 d1 1a f9 ff 48 85 db 74 06 48 3b 5d d0 75 16 <49> 8b 75 
18 48 c7 c7 a0 6c 85 91 48
 8b 56 28 e8 b2 1a f9 ff 0f 0b
[  402.008655] RSP: 0018:ab408147bde0 EFLAGS: 00010246
[  402.008656] RAX: 0021 RBX:  RCX: 0002
[  402.008657] RDX: 8002 RSI: 9189757d RDI: 
[  402.008658] RBP: ab408147be20 R08: 0001 R09: 955c
[  402.008658] R10: 0004 R11:  R12: 
[  402.008659] R13:  R14: 90736d305f40 R15: 
[  402.008661] FS:  7f20f6ffd700() GS:9073781c() 
knlGS:
[  402.008675] CS:  0010 DS:  ES:  CR0: 80050033
[  402.008678] CR2: 0018 CR3: 0001ed256006 CR4: 003706e0
[  402.008684] Call Trace:
[  402.008689]  ? elfcorehdr_read+0x40/0x40
[  402.008690]  ? elfcorehdr_read+0x40/0x40
[  402.008691]  trampoline_handler+0x42/0x60
[  402.008692]  kretprobe_trampoline+0x2a/0x50
[  402.008693] RIP: 0010:kretprobe_trampoline+0x0/0x50

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: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash

2020-08-28 Thread Masami Hiramatsu 
On Fri, 28 Aug 2020 03:00:59 +0900
Masami Hiramatsu  wrote:

> On Thu, 27 Aug 2020 18:12:40 +0200
> Peter Zijlstra  wrote:
> 
> > +static void invalidate_rp_inst(struct task_struct *t, struct kretprobe *rp)
> > +{
> > +   struct invl_rp_ipi iri = {
> > +   .task = t,
> > +   .rp = rp,
> > +   .done = false
> > +   };
> > +
> > +   for (;;) {
> > +   if (try_invoke_on_locked_down_task(t, __invalidate_rp_inst, rp))
> > +   return;
> > +
> > +   smp_call_function_single(task_cpu(t), __invalidate_rp_ipi, 
> > , 1);
> > +   if (iri.done)
> > +   return;
> > +   }
> 
> Hmm, what about making a status place holder and point it from
> each instance to tell it is valid or not?
> 
> struct kretprobe_holder {
>   atomic_t refcnt;
>   struct kretprobe *rp;
> };
> 
> struct kretprobe {
> ...
>   struct kretprobe_holder *rph;   // allocate at register
> ...
> };
> 
> struct kretprobe_instance {
> ...
>   struct kretprobe_holder *rph; // free if refcnt == 0
> ...
> };
> 
> cleanup_rp_inst(struct kretprobe *rp)
> {
>   rp->rph->rp = NULL;
> }
> 
> kretprobe_trampoline_handler()
> {
> ...
>   rp = READ_ONCE(ri->rph-rp);
>   if (likely(rp)) {
>   // call rp->handler
>   } else
>   rcu_call(ri, free_rp_inst_rcu);
> ...
> }
> 
> free_rp_inst_rcu()
> {
>   if (!atomic_dec_return(ri->rph->refcnt))
>   kfree(ri->rph);
>   kfree(ri);
> }
> 
> This increase kretprobe_instance a bit, but make things simpler.
> (and still keep lockless, atomic op is in the rcu callback).

OK, I've written the code and run a smoke test on it.
I'll send it with my 4th version of series.

Thank you,

-- 
Masami Hiramatsu

Re: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash

2020-08-28 Thread peterz 
On Fri, Aug 28, 2020 at 03:00:59AM +0900, Masami Hiramatsu wrote:
> On Thu, 27 Aug 2020 18:12:40 +0200
> Peter Zijlstra  wrote:
> 
> > +static void invalidate_rp_inst(struct task_struct *t, struct kretprobe *rp)
> > +{
> > +   struct invl_rp_ipi iri = {
> > +   .task = t,
> > +   .rp = rp,
> > +   .done = false
> > +   };
> > +
> > +   for (;;) {
> > +   if (try_invoke_on_locked_down_task(t, __invalidate_rp_inst, rp))
> > +   return;
> > +
> > +   smp_call_function_single(task_cpu(t), __invalidate_rp_ipi, 
> > , 1);
> > +   if (iri.done)
> > +   return;
> > +   }
> 
> Hmm, what about making a status place holder and point it from
> each instance to tell it is valid or not?
> 
> struct kretprobe_holder {
>   atomic_t refcnt;
>   struct kretprobe *rp;
> };
> 
> struct kretprobe {
> ...
>   struct kretprobe_holder *rph;   // allocate at register
> ...
> };
> 
> struct kretprobe_instance {
> ...
>   struct kretprobe_holder *rph; // free if refcnt == 0
> ...
> };
> 
> cleanup_rp_inst(struct kretprobe *rp)
> {
>   rp->rph->rp = NULL;
> }
> 
> kretprobe_trampoline_handler()
> {
> ...
>   rp = READ_ONCE(ri->rph-rp);
>   if (likely(rp)) {
>   // call rp->handler
>   } else
>   rcu_call(ri, free_rp_inst_rcu);
> ...
> }
> 
> free_rp_inst_rcu()
> {
>   if (!atomic_dec_return(ri->rph->refcnt))
>   kfree(ri->rph);
>   kfree(ri);
> }
> 
> This increase kretprobe_instance a bit, but make things simpler.
> (and still keep lockless, atomic op is in the rcu callback).

Yes, much better.

Although I'd _love_ to get rid of rp->data_size, then we can simplify
all of this even more. I was thinking we could then have a single global
freelist thing and add some per-cpu cache to it (say 4-8 entries) to
avoid the worst contention.

And then make function-graph use this, instead of the other way around
:-)

Re: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash

2020-08-27 Thread Masami Hiramatsu 
On Thu, 27 Aug 2020 18:12:40 +0200
Peter Zijlstra  wrote:

> @@ -1313,25 +1261,28 @@ void kprobe_busy_end(void)
>  void kprobe_flush_task(struct task_struct *tk)
>  {
>   struct kretprobe_instance *ri;
> - struct hlist_head *head, empty_rp;
> + struct hlist_head empty_rp;
> + struct llist_node *node;
>   struct hlist_node *tmp;

We don't need this tmp anymore.

> @@ -1935,71 +1932,45 @@ unsigned long __kretprobe_trampoline_han
>   unsigned long trampoline_address,
>   void *frame_pointer)
>  {
> + kprobe_opcode_t *correct_ret_addr = NULL;
>   struct kretprobe_instance *ri = NULL;
> - struct hlist_head *head, empty_rp;
> + unsigned long orig_ret_address = 0;
> + struct llist_node *first, *node;
> + struct hlist_head empty_rp;
>   struct hlist_node *tmp;

Here too.

I'm trying to port this patch on my v4 series. I'll add my RFC patch of
kretprobe_holder too.

Thank you,

-- 
Masami Hiramatsu

Re: [RFC][PATCH 3/7] kprobes: Remove kretprobe hash

2020-08-27 Thread Masami Hiramatsu 
On Thu, 27 Aug 2020 18:12:40 +0200
Peter Zijlstra  wrote:

> +static void invalidate_rp_inst(struct task_struct *t, struct kretprobe *rp)
> +{
> + struct invl_rp_ipi iri = {
> + .task = t,
> + .rp = rp,
> + .done = false
> + };
> +
> + for (;;) {
> + if (try_invoke_on_locked_down_task(t, __invalidate_rp_inst, rp))
> + return;
> +
> + smp_call_function_single(task_cpu(t), __invalidate_rp_ipi, 
> , 1);
> + if (iri.done)
> + return;
> + }

Hmm, what about making a status place holder and point it from
each instance to tell it is valid or not?

struct kretprobe_holder {
atomic_t refcnt;
struct kretprobe *rp;
};

struct kretprobe {
...
struct kretprobe_holder *rph;   // allocate at register
...
};

struct kretprobe_instance {
...
struct kretprobe_holder *rph; // free if refcnt == 0
...
};

cleanup_rp_inst(struct kretprobe *rp)
{
rp->rph->rp = NULL;
}

kretprobe_trampoline_handler()
{
...
rp = READ_ONCE(ri->rph-rp);
if (likely(rp)) {
// call rp->handler
} else
rcu_call(ri, free_rp_inst_rcu);
...
}

free_rp_inst_rcu()
{
if (!atomic_dec_return(ri->rph->refcnt))
kfree(ri->rph);
kfree(ri);
}

This increase kretprobe_instance a bit, but make things simpler.
(and still keep lockless, atomic op is in the rcu callback).

Thank you,

-- 
Masami Hiramatsu