On Fri, 28 Aug 2020 14:52:36 +0200
peterz@xxxxxxxxxxxxx wrote:
>
> If you do this, can you merge this into the previos patch and then
> delete the sched try_to_invoke..() patch?
Yes, this is just for making code review easy. :)
>
> Few comments below.
>
> On Fri, Aug 28, 2020 at 09:30:17PM +0900, Masami Hiramatsu wrote:
>
>
> > +static nokprobe_inline struct kretprobe *get_kretprobe(struct kretprobe_instance *ri)
> > +{
> > + /* rph->rp can be updated by unregister_kretprobe() on other cpu */
> > + smp_rmb();
> > + return ri->rph->rp;
> > +}
>
> That ordering doesn't really make sense, ordering requires at least two
> variables, here there is only 1. That said, get functions usually need
> an ACQUIRE order to make sure subsequent accesses are indeed done later.
So,
return smp_load_acquire(ri->rph->rp);
will be enough?
>
> > #else /* CONFIG_KRETPROBES */
> > static inline void arch_prepare_kretprobe(struct kretprobe *rp,
> > struct pt_regs *regs)
>
> > @@ -1922,6 +1869,7 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
> > kprobe_opcode_t *correct_ret_addr = NULL;
> > struct kretprobe_instance *ri = NULL;
> > struct llist_node *first, *node;
> > + struct kretprobe *rp;
> >
> > first = node = current->kretprobe_instances.first;
> > while (node) {
> > @@ -1951,12 +1899,13 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
> > /* Run them.. */
> > while (first) {
> > ri = container_of(first, struct kretprobe_instance, llist);
> > + rp = get_kretprobe(ri);
> > node = first->next;
>
> (A)
>
> > - if (ri->rp && ri->rp->handler) {
> > - __this_cpu_write(current_kprobe, &ri->rp->kp);
> > + if (rp && rp->handler) {
> > + __this_cpu_write(current_kprobe, &rp->kp);
> > ri->ret_addr = correct_ret_addr;
> > - ri->rp->handler(ri, regs);
> > + rp->handler(ri, regs);
> > __this_cpu_write(current_kprobe, &kprobe_busy);
> > }
>
> So here we're using get_kretprobe(), but what is to stop anybody from
> doing unregister_kretprobe() right at (A) such that we did observe our
> rp, but by the time we use it, it's a goner.
In kprobe_busy_begin() we disable preempt, so this block is not preemptive.
And as you may know, the unregister_kretprobe() is waiting rcu grace period
after it clear the rp->rph->rp. So, someone does unregister_kretprobe() at
(A), rph->rp = NULL but rp itself is not released until all running
trampoline_handlers exit.
>
>
> > + rp->rph = kzalloc(sizeof(struct kretprobe_holder), GFP_KERNEL);
> > + rp->rph->rp = rp;
>
> I think you'll need to check the allocation succeeded, no? :-)
Oops, I had found it once but forgot to fix :(
>
>
> > @@ -2114,16 +2065,20 @@ void unregister_kretprobes(struct kretprobe **rps, int num)
> > if (num <= 0)
> > return;
> > mutex_lock(&kprobe_mutex);
> > - for (i = 0; i < num; i++)
> > + for (i = 0; i < num; i++) {
> > if (__unregister_kprobe_top(&rps[i]->kp) < 0)
> > rps[i]->kp.addr = NULL;
> > + rps[i]->rph->rp = NULL;
> > + }
> > + /* Ensure the rph->rp updated after this */
> > + smp_wmb();
> > mutex_unlock(&kprobe_mutex);
>
> That ordering is dodgy again, those barriers don't help anything if
> someone else is at (A) above.
>
> >
> > synchronize_rcu();
>
> This one might help, this means we can do rcu_read_lock() around
> get_kretprobe() and it's usage. Can we call rp->handler() under RCU?
Yes, as I said above, the get_kretprobe() (and kretprobe handler) must be
called under preempt-disabled.
Thank you,
>
> > for (i = 0; i < num; i++) {
> > if (rps[i]->kp.addr) {
> > __unregister_kprobe_bottom(&rps[i]->kp);
> > - cleanup_rp_inst(rps[i]);
> > + free_rp_inst(rps[i]);
> > }
> > }
> > }
--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>
