Re: CVS commit: src/sys/kern

[Kamil Rytarowski]

On 13.11.2016 03:39, Robert Elz wrote:
> 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.
> 

Done!

>   | 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
> 

I have added two tests: attach6 and attach7, in t_ptrace_wait*. The
former tests sysctl(7) and struct kinfo_proc2 and the latter
/proc/curproc/status.

As of now, both tests fail for me. I will wait for releng test bots to
confirm it.

There is a side topic. how useful is /proc, as it was implemented in
order to address old (4.4BSD as far as I know) defects in ptrace(2). The
deficiencies have been addressed long time ago and the duplicated
interface is still there. For now I will just ignore procfs, I'm just
wondering whether there are still users of it.



signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/sys/kern

[Christos Zoulas]

On Nov 13,  8:39am, k...@munnari.oz.au (Robert Elz) wrote:
-- Subject: Re: CVS commit: src/sys/kern

| 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.)

p_ppid is supposed to be the cached value of p->p_pptr->p_pid (or if you
want to make the situation more complex) or p->p_opptr->p_pid. We can undo
the complex logic if we make sure that p->p_ppid is always updated. Perhaps
we can put the refresh bits in proc_reparent()...

As far as leaving p->p_pptr alone, I am not sure. We'd have to check all
the places it's beeing used.

christos