This patch allows tracer to figure out which of its potentially many
tracees performed the execve.
Run-tested.
Below is the output of a test program which creates two additional threads,
and one of them execs. PTRACE_O_TRACECLONE, PTRACE_O_TRACEEXIT and
PTRACE_O_TRACEEXEC are in effect:
4857: thread leader
4857: status:0003057f WIFSTOPPED sig:5 (TRAP) event:CLONE eventdata:0x12fa (4858)
4858: status:0000137f WIFSTOPPED sig:19 (STOP) event:none eventdata:0x0 (0)
4857: status:0003057f WIFSTOPPED sig:5 (TRAP) event:CLONE eventdata:0x12fb (4859)
4859: status:0000137f WIFSTOPPED sig:19 (STOP) event:none eventdata:0x12fa (4858)
4858: status:0006057f WIFSTOPPED sig:5 (TRAP) event:EXIT eventdata:0x0 (0)
4857: status:0006057f WIFSTOPPED sig:5 (TRAP) event:EXIT eventdata:0x0 (0)
4858: status:00000000 WIFEXITED exitcode:0
4857: status:0004057f WIFSTOPPED sig:5 (TRAP) event:EXEC eventdata:0x12fb (4859)
Signed-off-by: Denys Vlasenko <[email protected]>
diff --git a/fs/exec.c b/fs/exec.c
index 6075a1e..edf9ed2 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1366,13 +1366,22 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
for (try=0; try<2; try++) {
read_lock(&binfmt_lock);
list_for_each_entry(fmt, &formats, lh) {
- int (*fn)(struct linux_binprm *, struct pt_regs *) = fmt->load_binary;
- if (!fn)
+ int (*load_binary)(struct linux_binprm *, struct pt_regs *);
+ pid_t old_pid = old_pid; /* for compiler */
+
+ load_binary = fmt->load_binary;
+ if (!load_binary)
continue;
if (!try_module_get(fmt->module))
continue;
read_unlock(&binfmt_lock);
- retval = fn(bprm, regs);
+ if (task_ptrace(current) & PT_PTRACED) {
+ /* Need to fetch pid before load_binary changes it */
+ rcu_read_lock();
+ old_pid = task_pid_nr_ns(current, task_active_pid_ns(current->parent));
+ rcu_read_unlock();
+ }
+ retval = load_binary(bprm, regs);
/*
* Restore the depth counter to its starting value
* in this call, so we don't have to rely on every
@@ -1381,7 +1390,7 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
bprm->recursion_depth = depth;
if (retval >= 0) {
if (depth == 0)
- tracehook_report_exec(fmt, bprm, regs);
+ tracehook_report_exec(fmt, bprm, regs, old_pid);
put_binfmt(fmt);
allow_write_access(bprm->file);
if (bprm->file)
diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
index e95f523..c87866d 100644
--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -199,9 +199,10 @@ static inline struct task_struct *tracehook_tracer_task(struct task_struct *tsk)
*/
static inline void tracehook_report_exec(struct linux_binfmt *fmt,
struct linux_binprm *bprm,
- struct pt_regs *regs)
+ struct pt_regs *regs,
+ pid_t old_pid)
{
- if (!ptrace_event(PT_TRACE_EXEC, PTRACE_EVENT_EXEC, 0) &&
+ if (!ptrace_event(PT_TRACE_EXEC, PTRACE_EVENT_EXEC, old_pid) &&
unlikely(task_ptrace(current) & PT_PTRACED))
send_sig(SIGTRAP, current, 0);
}
On 06/26, Denys Vlasenko wrote:
>
> @@ -1366,13 +1366,22 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
> for (try=0; try<2; try++) {
> read_lock(&binfmt_lock);
> list_for_each_entry(fmt, &formats, lh) {
> - int (*fn)(struct linux_binprm *, struct pt_regs *) = fmt->load_binary;
> - if (!fn)
> + int (*load_binary)(struct linux_binprm *, struct pt_regs *);
> + pid_t old_pid = old_pid; /* for compiler */
we have uninitialized_var() for this,
pid_t uninitialized_var(old_pid);
> +
> + load_binary = fmt->load_binary;
> + if (!load_binary)
> continue;
> if (!try_module_get(fmt->module))
> continue;
> read_unlock(&binfmt_lock);
> - retval = fn(bprm, regs);
> + if (task_ptrace(current) & PT_PTRACED) {
May be PT_TRACE_EXEC makes more sense. Note that ptrace_event_enabled() was
recently added.
> + /* Need to fetch pid before load_binary changes it */
> + rcu_read_lock();
> + old_pid = task_pid_nr_ns(current, task_active_pid_ns(current->parent));
OK, this looks correct. But imho this code looks strange inside the
for (;;) loop. Perhaps it would be more clean to record the old pid
before.
Also, this line is too long. Personally I do not care, but I told you
we have the coding style police. Please use ./scripts/checkpatch.pl
> @@ -1381,7 +1390,7 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
> bprm->recursion_depth = depth;
> if (retval >= 0) {
> if (depth == 0)
> - tracehook_report_exec(fmt, bprm, regs);
> + tracehook_report_exec(fmt, bprm, regs, old_pid);
Heh, you are out of luck ;) This hook was already killed. Please redo
against git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc.git ptrace
Also, please update the changelog. It should clearly explain why do we
need this feature and what this patch does. The output from a test
program doesn't make too much sense unless you show the source code.
Oleg.
Hello, Oleg, Denys.
On Sun, Jun 26, 2011 at 10:04:42PM +0200, Oleg Nesterov wrote:
> May be PT_TRACE_EXEC makes more sense. Note that
> ptrace_event_enabled() was recently added.
Do we want to enable this silently? Wouldn't it be better to make it
dependent on PT_SEIZED? Otherwise, it's something which simply might
or might not be there.
--
tejun
On 06/27, Tejun Heo wrote:
>
> Hello, Oleg, Denys.
>
> On Sun, Jun 26, 2011 at 10:04:42PM +0200, Oleg Nesterov wrote:
> > May be PT_TRACE_EXEC makes more sense. Note that
> > ptrace_event_enabled() was recently added.
>
> Do we want to enable this silently? Wouldn't it be better to make it
> dependent on PT_SEIZED?
Hmm. Not sure I understand. Why can't PTRACE_SEIZE add PT_TRACE_EXEC
(and PT_TRACESYSGOOD) along with PT_SEIZED during attach?
I think this makes more sense, this way the tracer can disable this
later via PTRACE_SETOPTIONS if it wants. Not that I think this is really
useful but still. Otherwise we are going to silently disable
PTRACE_O_TRACEEXEC, this may be confusing.
But. If we want the PT_TRACE_EXEC behaviour for PT_SEIZED task (personally
I think we do), then we should probably record the old pid unconditionally,
the tracer can attach later.
Even in this case ptrace_event(PTRACE_EVENT_EXEC) can race with detach +
attach in theory, but I think in this case we do not care.
Oleg.
Hello, Oleg.
On Mon, Jun 27, 2011 at 03:47:13PM +0200, Oleg Nesterov wrote:
> > Do we want to enable this silently? Wouldn't it be better to make it
> > dependent on PT_SEIZED?
>
> Hmm. Not sure I understand. Why can't PTRACE_SEIZE add PT_TRACE_EXEC
> (and PT_TRACESYSGOOD) along with PT_SEIZED during attach?
I'm worrying about !PT_SEIZED case. If we make it solely depend on
PT_TRACE_EXEC, newer kernels report the old pid while olders ones
don't and the only way to discover would be either comparing kernel
version or actually trying it - both aren't too nice.
Thanks.
--
tejun
On 06/27, Tejun Heo wrote:
>
> Hello, Oleg.
>
> On Mon, Jun 27, 2011 at 03:47:13PM +0200, Oleg Nesterov wrote:
> > > Do we want to enable this silently? Wouldn't it be better to make it
> > > dependent on PT_SEIZED?
> >
> > Hmm. Not sure I understand. Why can't PTRACE_SEIZE add PT_TRACE_EXEC
> > (and PT_TRACESYSGOOD) along with PT_SEIZED during attach?
>
> I'm worrying about !PT_SEIZED case. If we make it solely depend on
> PT_TRACE_EXEC, newer kernels report the old pid while olders ones
> don't
Ah, understood. So you think that the old pid should be only reported
if PT_SEIZED.
May be... Denys, what do you think?
OTOH, it looks simpler if PT_TRACE_EXEC always reports the old pid,
this can't break the applications which do not know about this new
feature.
> and the only way to discover would be either comparing kernel
> version or actually trying it - both aren't too nice.
Fortunately, currently tracehook_report_exec() zeroes ->ptrace_message.
At least this means that anything != 0 means it works.
Oleg.
On Sun, Jun 26, 2011 at 10:04 PM, Oleg Nesterov <[email protected]> wrote:
>> + ? ? ? ? ? ? ? ? ? ? pid_t old_pid = old_pid; /* for compiler */
>
> we have uninitialized_var() for this,
>
> ? ? ? ?pid_t uninitialized_var(old_pid);
>
>> - ? ? ? ? ? ? ? ? ? ? retval = fn(bprm, regs);
>> + ? ? ? ? ? ? ? ? ? ? if (task_ptrace(current) & PT_PTRACED) {
>
> May be PT_TRACE_EXEC makes more sense. Note that ptrace_event_enabled() was
> recently added.
>
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? /* Need to fetch pid before load_binary changes it */
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? rcu_read_lock();
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? old_pid = task_pid_nr_ns(current, task_active_pid_ns(current->parent));
>
> OK, this looks correct. But imho this code looks strange inside the
> for (;;) loop. Perhaps it would be more clean to record the old pid
> before.
>
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (depth == 0)
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? tracehook_report_exec(fmt, bprm, regs);
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? tracehook_report_exec(fmt, bprm, regs, old_pid);
>
> Heh, you are out of luck ;) This hook was already killed. Please redo
> against git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc.git ptrace
I just sent rediffed version against this branch as a separate mail.
I think I addressed all your concerns in it.
--
vda
Hello,
On Mon, Jun 27, 2011 at 05:18:27PM +0200, Oleg Nesterov wrote:
> > and the only way to discover would be either comparing kernel
> > version or actually trying it - both aren't too nice.
>
> Fortunately, currently tracehook_report_exec() zeroes ->ptrace_message.
> At least this means that anything != 0 means it works.
Yeah, but that's a pretty silly way to do it. If we make it depend on
PT_SEIZED, we can simply say "if seized, EXEC reports..." but as it
currently stands, it would go like "If the message is non-zero on
EXEC, it indicates... This behavior is valid since kernel version
x.x.x". Maybe adding a guarantee that PTRACE_SEIZE capable kernel
always reports the old pid on EXEC but that would still seem
unnecessarily complicated. It isn't a bug fix. I don't see much
point in introducing new behavior separately.
Thanks.
--
tejun
On Tue, Jun 28, 2011 at 10:25 AM, Tejun Heo <[email protected]> wrote:
> On Mon, Jun 27, 2011 at 05:18:27PM +0200, Oleg Nesterov wrote:
>> > and the only way to discover would be either comparing kernel
>> > version or actually trying it - both aren't too nice.
>>
>> Fortunately, currently tracehook_report_exec() zeroes ->ptrace_message.
>> At least this means that anything != 0 means it works.
>
> Yeah, but that's a pretty silly way to do it. ?If we make it depend on
> PT_SEIZED, we can simply say "if seized, EXEC reports..." but as it
> currently stands, it would go like "If the message is non-zero on
> EXEC, it indicates... ?This behavior is valid since kernel version
> x.x.x".
This is true for any new addition to API.
It starts from some kernel version.
>?Maybe adding a guarantee that PTRACE_SEIZE capable kernel
> always reports the old pid on EXEC but that would still seem
> unnecessarily complicated. ?It isn't a bug fix. ?I don't see much
> point in introducing new behavior separately.
This new feature looks orthogonal to PTRACE_SEIZE to me.
--
vda
Hello,
On Tue, Jun 28, 2011 at 02:30:36PM +0200, Denys Vlasenko wrote:
> On Tue, Jun 28, 2011 at 10:25 AM, Tejun Heo <[email protected]> wrote:
> > On Mon, Jun 27, 2011 at 05:18:27PM +0200, Oleg Nesterov wrote:
> > Yeah, but that's a pretty silly way to do it. ?If we make it depend on
> > PT_SEIZED, we can simply say "if seized, EXEC reports..." but as it
> > currently stands, it would go like "If the message is non-zero on
> > EXEC, it indicates... ?This behavior is valid since kernel version
> > x.x.x".
>
> This is true for any new addition to API.
> It starts from some kernel version.
Hmmm... but as I wrote above, we have a choice to make here and the
two options are clearly different?
> >?Maybe adding a guarantee that PTRACE_SEIZE capable kernel
> > always reports the old pid on EXEC but that would still seem
> > unnecessarily complicated. ?It isn't a bug fix. ?I don't see much
> > point in introducing new behavior separately.
>
> This new feature looks orthogonal to PTRACE_SEIZE to me.
Yeah, sure, but we're bundling a number of behavior changes with
PT_SEIZED because it's impractical to introduce each with its own
compatibility / feature test bits, so it makes sense to do the same
with this one, I think. It's not like there's a lot to gain by
supporting this outside of SEIZE anyway.
Thanks.
--
tejun
On 06/28, Tejun Heo wrote:
>
> Hello,
>
> On Tue, Jun 28, 2011 at 02:30:36PM +0200, Denys Vlasenko wrote:
> > On Tue, Jun 28, 2011 at 10:25 AM, Tejun Heo <[email protected]> wrote:
> > > On Mon, Jun 27, 2011 at 05:18:27PM +0200, Oleg Nesterov wrote:
> > > Yeah, but that's a pretty silly way to do it. ?If we make it depend on
> > > PT_SEIZED, we can simply say "if seized, EXEC reports..." but as it
> > > currently stands, it would go like "If the message is non-zero on
> > > EXEC, it indicates... ?This behavior is valid since kernel version
> > > x.x.x".
> >
> > This is true for any new addition to API.
> > It starts from some kernel version.
>
> Hmmm... but as I wrote above, we have a choice to make here and the
> two options are clearly different?
I do not really understand your concerns. Yes, yes, yes, if we do not
bind this feauture with PT_SEIZED, then the application has to take
care _if_ it wants to use it without PT_SEIZED. So what? This is the
problem of that application. This change can't break the application
which do not want or do not know about this feature.
And how can we bind it to PT_SEIZED? We can't do something like
old_pid = 0;
if (PT_SEIZED) {
old_pid = task_pid_nr_ns(...);
}
...
ptrace_event(PTRACE_EVENT_EXEC, old_pid);
in search_binary_handler(), this is racy, the tracer can attach in the
window and we want to avoid SIGTRAP.
So, we should pass the valid old_pid to ptrace_event() unconditionally
(btw, Denys, I think we should do this anyway, but perhaps we can do
this later) and then uglify ptrace_event() somehow.
Say,
static inline void ptrace_event(int event, unsigned long message)
{
+ if (event == PTRACE_EVENT_EXEC && !PT_SEIZED)
+ message = 0;
if (unlikely(ptrace_event_enabled(current, event))) {
current->ptrace_message = message;
ptrace_notify((event << 8) | SIGTRAP);
} else if (event == PTRACE_EVENT_EXEC && unlikely(current->ptrace)) {
/* legacy EXEC report via SIGTRAP */
send_sig(SIGTRAP, current, 0);
}
}
looks a bit ugly. Or, perhaps,
static inline void ptrace_event(int event, unsigned long message)
{
bool enabled = ptrace_event_enabled(current, event);
if (event == PTRACE_EVENT_EXEC) {
if (PT_SEIZED) {
enabled = true;
} else {
if (!enabled) {
send_sig(SIGTRAP, current, 0);
return;
}
message = 0;
}
}
if (enabled) {
current->ptrace_message = message;
ptrace_notify((event << 8) | SIGTRAP);
}
}
a bit better, bit still not very nice. For what? I tend to agree with
Denys, and I think we should listen to the user-space developer who
actually uses ptrace.
That said, I leave it to you and Denys, personally I am fine either way.
FYI, I need to disappear till Thursday.
Oleg.
Hello,
On Tue, Jun 28, 2011 at 6:35 PM, Oleg Nesterov <[email protected]> wrote:
> I do not really understand your concerns. Yes, yes, yes, if we do not
> bind this feauture with PT_SEIZED, then the application has to take
> care _if_ it wants to use it without PT_SEIZED. So what? This is the
> problem of that application. This change can't break the application
> which do not want or do not know about this feature.
It's not really a technical concern, more about documentation
simplicity. It's just simpler to be able to say "blah blah changes
when seized".
> a bit better, bit still not very nice. For what? I tend to agree with
> Denys, and I think we should listen to the user-space developer who
> actually uses ptrace.
>
> That said, I leave it to you and Denys, personally I am fine either way.
It's minor either way. Let's go with the current patch then.
Thanks.
--
tejun