Re: [PATCH] ptrace: make former thread ID available via PTRACE_GETEVENTMSG after PTRACE_EVENT_EXEC stop (v.2)

[Oleg Nesterov <oleg@redhat.com>]

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 <tj@kernel.org> 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.