Re: CVS commit: src/sys/kern

2016-11-12 Thread Robert Elz 
Date:Sun, 13 Nov 2016 12:13:48 +0700
From:Robert Elz 
Message-ID:  <18703.1479014...@andromeda.noi.kre.to>

  | In an earlier message I said ...

  | and the modification of p_ppid there [to init]
  | could be handled outside proc_reparent()

Too messy.  But, in proc_reparent()

if (parent == initproc)
child->p_ppid = parent->p_pid;

(or just " = 1;") would handle all the cases I think.   Could an extra test
and move the p_exitsit setting when being reparented to init so they're
both handled by the same test.

kre

Re: CVS commit: src/sys/kern

2016-11-12 Thread Robert Elz 
In an earlier message I said ...

  | Maybe the real bug here was that proc_reparent() is changing the
  | child's p_ppid ?
  | 
  | I can see no reason for that,

There is one, an obvious one, I had forgotten ... when a child is
inherited by init, we want its ppid to become 1.   That one is something
of a special case as it is the one permanent reparenting that exists,
and the modification of p_ppid there could be handled outside proc_reparent()

kre

Re: CVS commit: src/sys/kern

2016-11-12 Thread Robert Elz 
Date:Sun, 13 Nov 2016 02:44:03 +0100
From:Kamil Rytarowski 
Message-ID:  <332a57da-1ac6-38ed-4fc3-947e2e6ca...@gmx.com>

  | I can add a test for it, comparing old parent identifier with p_ppid
  | from kinfo_proc2.

That would be useful, I suspect they will be the same except when the
process is being traced.

  | Another place with ppid is in procfs: /proc//stat
  | The 4th field should be PPID. 

That one comes from p_ppid .. so will also probably be (currently) incorrect
for a traced process, so a test would be good to verify.   That could also be
fixed by using the new kern_getppid() or by just not changing p_ppid in
proc_reparent() if no-one can find a reason why the change is needed.

As best I can tell, p_ppid is used excludively for providing info to userland,
and the info wanted is always the original parent's pid, so changing it
doesn't make a lot of sense to me.

kre

Re: CVS commit: src/sys/kern

2016-11-12 Thread Kamil Rytarowski 


On 13.11.2016 02:44, Kamil Rytarowski wrote:
> 
> 
> On 13.11.2016 02:39, Robert Elz wrote:
>> Date:Sat, 12 Nov 2016 14:42:47 -0500
>> From:"Christos Zoulas" 
>> Message-ID:  <20161112194247.37910f...@cvs.netbsd.org>
>>
>>   | PR/51624: Return the original parent for a traced process.
>>
>> Maybe the real bug here was that proc_reparent() is changing the
>> child's p_ppid ?
>>
>> I can see no reason for that, and if it wasn't done, then p_ppid would
>> be what is wanted by getppid() without needing kern_getppid() to
>> do all that unwind logic (and assiated locting and unlocking to make
>> it safe.)
>>
>> Aside from proc_reparent() the only weirdness I can see with p_ppid are
>> in kern_proc.c in fill_eproc() and fill_kproc2().   They both use (and
>> continue to use, so the results will be different for a process being
>> traced, and the same process when not traced) p_pptr->p_pid rather than
>> the simpler p_ppid but I am not sure why (nor what the clients of those
>> functions are or what the info is used for, so I am not sure what is 
>> correct.)
>>
>> kre
>>
> 
> It's also common to use kinfo_proc2 and extract data from there. There
> is one field:
> 
> int32_t p_ppid; /* PID_T: Parent process id */
> 
> getppid(2) and p_ppid shall be the same.
> 
> I can add a test for it, comparing old parent identifier with p_ppid
> from kinfo_proc2.
> 

Another place with ppid is in procfs:
/proc//stat

The 4th field should be PPID.

Another idea for a test.



signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/sys/kern

2016-11-12 Thread Kamil Rytarowski 


On 13.11.2016 02:39, Robert Elz wrote:
> Date:Sat, 12 Nov 2016 14:42:47 -0500
> From:"Christos Zoulas" 
> Message-ID:  <20161112194247.37910f...@cvs.netbsd.org>
> 
>   | PR/51624: Return the original parent for a traced process.
> 
> Maybe the real bug here was that proc_reparent() is changing the
> child's p_ppid ?
> 
> I can see no reason for that, and if it wasn't done, then p_ppid would
> be what is wanted by getppid() without needing kern_getppid() to
> do all that unwind logic (and assiated locting and unlocking to make
> it safe.)
> 
> Aside from proc_reparent() the only weirdness I can see with p_ppid are
> in kern_proc.c in fill_eproc() and fill_kproc2().   They both use (and
> continue to use, so the results will be different for a process being
> traced, and the same process when not traced) p_pptr->p_pid rather than
> the simpler p_ppid but I am not sure why (nor what the clients of those
> functions are or what the info is used for, so I am not sure what is correct.)
> 
> kre
> 

It's also common to use kinfo_proc2 and extract data from there. There
is one field:

int32_t p_ppid; /* PID_T: Parent process id */

getppid(2) and p_ppid shall be the same.

I can add a test for it, comparing old parent identifier with p_ppid
from kinfo_proc2.



signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/sys/kern

2016-11-12 Thread Robert Elz 
Date:Sat, 12 Nov 2016 14:42:47 -0500
From:"Christos Zoulas" 
Message-ID:  <20161112194247.37910f...@cvs.netbsd.org>

  | PR/51624: Return the original parent for a traced process.

Maybe the real bug here was that proc_reparent() is changing the
child's p_ppid ?

I can see no reason for that, and if it wasn't done, then p_ppid would
be what is wanted by getppid() without needing kern_getppid() to
do all that unwind logic (and assiated locting and unlocking to make
it safe.)

Aside from proc_reparent() the only weirdness I can see with p_ppid are
in kern_proc.c in fill_eproc() and fill_kproc2().   They both use (and
continue to use, so the results will be different for a process being
traced, and the same process when not traced) p_pptr->p_pid rather than
the simpler p_ppid but I am not sure why (nor what the clients of those
functions are or what the info is used for, so I am not sure what is correct.)

kre

Re: CVS commit: src/sys/kern

2016-11-12 Thread Christos Zoulas 
On Nov 12,  5:47pm, u...@stderr.spb.ru (Valery Ushakov) wrote:
-- Subject: Re: CVS commit: src/sys/kern

| So what's going on here?

I think that the tests were broken. I have reverted it...

christos

Re: CVS commit: src/sys/kern

2016-11-12 Thread Kamil Rytarowski 
On 12.11.2016 15:47, Valery Ushakov wrote:
> On Fri, Nov 11, 2016 at 12:10:04 -0500, Christos Zoulas wrote:
> 
>> Date:Fri Nov 11 17:10:04 UTC 2016
>>
>> Modified Files:
>>  src/sys/kern: sys_ptrace_common.c
>>
>> Log Message:
>> kern/51621: When attaching to a child send it a SIGTRAP not a SIGSTOP like
>> Linux and FreeBSD do.
>>
>>
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.2 -r1.3 src/sys/kern/sys_ptrace_common.c
> 
> Hmm, I'm confused.  The PR says:
> 
> - PT_ATTACH seems to work, but waiting for stopped status and signal from the
>   child results in getting SIGTRAP, not SIGSTOP like in Linux and FreeBSD.
> 
> This revision changes PT_ATTACH case to:
> 
> proc_changeparent(t, p);
> -   signo = SIGSTOP;
> +   signo = SIGTRAP;
> goto sendsig;
> 
> which looks like it's the opposite of what's said in the PR.
> 
> FreeBSD from source code inspection (abridged version quoted below)
> does indeed send SIGSTOP:
> 
>   case PT_ATTACH:
>   proc_set_traced(p, true);
>   if (p->p_pptr != td->td_proc) {
>   proc_reparent(p, td->td_proc);
>   }
>   data = SIGSTOP;
>   goto sendsig;
> 
> From a very quick look through the code it looks like the intention
> (before this commit) is to send SIGSTOP to a process that we attach
> to, which is semantically correct, as we want to just stop it as-is
> and let the tracer poke at it.
> 
> The SIGTRAP comes from the fork path as far as I can tell.
> 
> So what's going on here?
> 
> -uwe
> 

I'm going to set SIGSTOP as expected signal in attach3, is it is
preferred in general. Not just because FreeBSD and Linux works this way,
but to make it consistent and regardless of relation between tracee and
tracer always get the same signal.



signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/sys/kern

2016-11-12 Thread Valery Ushakov 
On Fri, Nov 11, 2016 at 12:10:04 -0500, Christos Zoulas wrote:

> Date: Fri Nov 11 17:10:04 UTC 2016
> 
> Modified Files:
>   src/sys/kern: sys_ptrace_common.c
> 
> Log Message:
> kern/51621: When attaching to a child send it a SIGTRAP not a SIGSTOP like
> Linux and FreeBSD do.
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.2 -r1.3 src/sys/kern/sys_ptrace_common.c

Hmm, I'm confused.  The PR says:

- PT_ATTACH seems to work, but waiting for stopped status and signal from the
  child results in getting SIGTRAP, not SIGSTOP like in Linux and FreeBSD.

This revision changes PT_ATTACH case to:

proc_changeparent(t, p);
-   signo = SIGSTOP;
+   signo = SIGTRAP;
goto sendsig;

which looks like it's the opposite of what's said in the PR.

FreeBSD from source code inspection (abridged version quoted below)
does indeed send SIGSTOP:

case PT_ATTACH:
proc_set_traced(p, true);
if (p->p_pptr != td->td_proc) {
proc_reparent(p, td->td_proc);
}
data = SIGSTOP;
goto sendsig;

>From a very quick look through the code it looks like the intention
(before this commit) is to send SIGSTOP to a process that we attach
to, which is semantically correct, as we want to just stop it as-is
and let the tracer poke at it.

The SIGTRAP comes from the fork path as far as I can tell.

So what's going on here?

-uwe