lkml.org 
[lkml]   [2020]   [Aug]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 20/23] [RFC] kprobes: Remove task scan for updating kretprobe_instance
    On Fri, 28 Aug 2020 14:52:36 +0200
    peterz@infradead.org 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@kernel.org>

    \
     
     \ /
      Last update: 2020-08-28 17:10    [W:7.529 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site