Re: CVS commit: src/sys/kern

2017-12-22 Thread Kamil Rytarowski 
On 22.12.2017 17:58, Robert Elz wrote:
> Date:Fri, 22 Dec 2017 23:35:12 +0700
> From:Robert Elz 
> Message-ID:  <19521.1513960...@andromeda.noi.kre.to>
> 
>   | The EFAULT that is returned there is probably the one Kamil mentioned.
> 
> And of course it is, as in the test ...
> 
>   if ((p != curproc && (p->p_sflag & PS_WEXIT) != 0) ||
> 
> (the || alternative is irrelevant, even though it is XXX'd) as
> we know p != curproc (this is ptrace I/O - the process whose addr
> space is being written is not the process doing the sys call
> "What never?   Well, hardly ever...")  and nor is that process usually
> exiting (too late to modify it then) - so for ptrace() that test
> is more or less guaranteed true - which leads to the EFAULT.
> 
> kre
> 

Thank you for the investigation!

I'm cleaning up now the status of the existing tests and be back to to
proper fix for PT_READ/WRITE_I/D.



signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/sys/kern

2017-12-22 Thread Robert Elz 
Date:Fri, 22 Dec 2017 23:35:12 +0700
From:Robert Elz 
Message-ID:  <19521.1513960...@andromeda.noi.kre.to>

  | The EFAULT that is returned there is probably the one Kamil mentioned.

And of course it is, as in the test ...

if ((p != curproc && (p->p_sflag & PS_WEXIT) != 0) ||

(the || alternative is irrelevant, even though it is XXX'd) as
we know p != curproc (this is ptrace I/O - the process whose addr
space is being written is not the process doing the sys call
"What never?   Well, hardly ever...")  and nor is that process usually
exiting (too late to modify it then) - so for ptrace() that test
is more or less guaranteed true - which leads to the EFAULT.

kre

Re: CVS commit: src/sys/kern

2017-12-22 Thread Robert Elz 
Date:Fri, 22 Dec 2017 23:22:37 +0700
From:Robert Elz 
Message-ID:  <1080.1513959...@andromeda.noi.kre.to>

  | where vm was declared as
  | 
  | struct vmspace *vm;
  | 
  | but is not otherwise initialised

Well, not quite, I missed this call in ptrce_doio()

error = proc_vmspace_getref(l->l_proc, vm);

where proc_vmspace_getref() is ...

if ((p != curproc && (p->p_sflag & PS_WEXIT) != 0) ||
(p->p_vmspace->vm_refcnt < 1)) { /* XXX */
return EFAULT;
}

uvmspace_addref(p->p_vmspace);
*vm = p->p_vmspace;

which is much more complex than the old (and now current way) which
more or less boils down to

*vm = proc0.p_vmspace;

(except there is no vm to * ... bt for easier comparison.)

The EFAULT that is returned there is probably the one Kamil mentioned.

kre

Re: CVS commit: src/sys/kern

2017-12-22 Thread Robert Elz 
Date:Fri, 22 Dec 2017 15:02:57 +
From:"Kamil Rytarowski" 
Message-ID:  <20171222150257.8e519f...@cvs.netbsd.org>

  | ptrace: Partially undo PT_{READ,WRITE}_{I,D} and unbreak these commands
  | 
  | The refactored code did not work and was generating EFAULT.

The only difference I see between the two (aside from an
#if defined(__HAVE_RAS)
which exists in the current version and is missing in the previous)
is that the earlier one is missing

UIO_SETUP_SYSSPACE();

and consequently is not initialising uio->uio_vmspace (or not
the same way, or properly) - the refactored code does

uio.uio_vmspace = *vm;

where vm is the final (6th) param to ptrace_doic() which it calls as

if ((error = ptrace_doio(l, t, lt, , addr, )) != 0)

where vm was declared as

struct vmspace *vm;

but is not otherwise initialised - fix this and the refactored code
would probably work.

kre