Re: [PATCH v4 20/23] [RFC] kprobes: Remove task scan for updating kretprobe_instance

[Masami Hiramatsu]

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>