netdev - Re: [PATCH] connector: add an event for monitoring process tracers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMW3pwbr1N6Yc7K=up3mYNq2f6+_6xA4hSgh8K1hWwHPnj_d9g@mail.gmail.com>
Date:	Fri, 15 Jul 2011 20:41:59 +0300
From:	Vladimir Zapolskiy <vzapolskiy@...il.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Evgeniy Polyakov <zbr@...emap.net>,
	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] connector: add an event for monitoring process tracers

Oleg,

first of all thank you for a good review, please see my comments below.

On Wed, Jul 13, 2011 at 6:09 PM, Oleg Nesterov <oleg@...hat.com> wrote:
> On 07/12, Vladimir Zapolskiy wrote:
>>
>> Note, a detach signal is not emitted, if a tracer process terminates
>> without explicit PTRACE_DETACH request. Such cases can be covered
>> listening to PROC_EVENT_EXIT connector events.
>
> Hmm. More and more reasons to make the implicit detach sleepable...
>
> But. There is another case. The (dead) tracee can be detached via
> do_wait(). Perhaps this falls into "covered listening to EXIT" too,
> but imho makes sense to document in the changelog. Oh, and probably
> we will add the ability to detach a zombie...
>
> I don't really understand why do you need this, but I won't argue.
>
I found that implicit ptrace detach codepath is quite mutable and
vast, and I don't want to interfere in that changes without knowing
even basic pitfalls. Somehow the sending a connector signal on
explicit detach is quite sufficient at least for the most of the proc
connector usecases I can imagine, because hopefully almost(?) all
implicit detach cases are related to tracer or tracee thread
completion, and that is supposed to be reported to userspace via
do_exit()/proc_exit_connector() path.

> As for the patch,
>
>> +void proc_ptrace_connector(struct task_struct *task)
>> +{
>> +     struct cn_msg *msg;
>> +     struct proc_event *ev;
>> +     struct timespec ts;
>> +     __u8 buffer[CN_PROC_MSG_SIZE];
>> +     struct task_struct *tracer;
>> +
>> +     if (atomic_read(&proc_event_num_listeners) < 1)
>> +             return;
>> +
>> +     msg = (struct cn_msg *)buffer;
>> +     ev = (struct proc_event *)msg->data;
>> +     get_seq(&msg->seq, &ev->cpu);
>> +     ktime_get_ts(&ts); /* get high res monotonic timestamp */
>> +     put_unaligned(timespec_to_ns(&ts), (__u64 *)&ev->timestamp_ns);
>> +     ev->what = PROC_EVENT_PTRACE;
>> +     ev->event_data.ptrace.process_pid  = task->pid;
>> +     ev->event_data.ptrace.process_tgid = task->tgid;
>> +
>> +     rcu_read_lock();
>> +     tracer = tracehook_tracer_task(task);
>> +     if (tracer) {
>> +             ev->event_data.ptrace.tracer_pid  = tracer->pid;
>> +             ev->event_data.ptrace.tracer_tgid = tracer->tgid;
>> +     } else {
>> +             ev->event_data.ptrace.tracer_pid  = 0;
>> +             ev->event_data.ptrace.tracer_tgid = 0;
>> +     }
>
> This doesn't look right. The code uses tracehook_tracer_task() to
> figure out whether this task traced or not. But this is racy.
>
> ptrace_attach:
>
>        ...attach...
>
>        /* WINDOW */
>
>        proc_ptrace_connector(task);
>
> The task can exit in between, and the caller's subthread can do
> wait4() and release it. In this case proc_ptrace_connector() will
> see tracehook_tracer_task() == NULL and report "detach".
>
> The similar race in ptrace_detach() path. Another tracer can attach
> to this task before we proc_ptrace_connector().
>
> I think proc_ptrace_connector() needs the explicit "task_struct *tracer"
> argument, NULL if ptrace_detach(). Or a simple boolean, the tracer is
> current.
>
> If you think this is fine - I won't argue.
>
Fixed in the second version of the change, thanks.

>
>
> But in any case, please rediff against
>
>        git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc.git ptrace
>
> tracehook_tracer_task() was removed, and
>
>> @@ -260,6 +261,9 @@ out:
>>       if (wait_trap)
>>               wait_event(current->signal->wait_chldexit,
>>                          !(task->group_stop & GROUP_STOP_TRAPPING));
>> +     if (!retval)
>> +             proc_ptrace_connector(task);
>> +
>>       return retval;
>>  }
>
> this chunk probably should be updated.
Rebased, thanks a lot.

Vladimir
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html