Subject: Re: COMPAT_NETBSD32's execve, copy/paste of code
To: Christos Zoulas <christos@astron.com>
From: Quentin Garnier <cube@cubidou.net>
List: tech-kern
Date: 07/11/2005 07:20:19
--GdbWtwDHkcXqP16f
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Jul 11, 2005 at 02:28:41AM +0300, Christos Zoulas wrote:
> In article <20050710221533.GE28480@gallia.cubidou.net>,
> Quentin Garnier  <cube@cubidou.net> wrote:
[...]
> >To properly fix netbsd32_wait4(), the only thing that would be needed is
> >a slighly smarter copyin()/copyout() that would check if the supposedly
> >user address is actually a kernel address or not, and in the former case
> >only do a memcpy().  I don't know if it easy to do that, even if it ends
> >as a set of MD implementations.  I do know that it wouldn't be only
> >useful for compat_netbsd32, though.
> >
> >However, a smarter copyin() is not enough when it comes to arrays of
> >structs of a size different from the native version.  This is the case
> >of execve(2) (and others, I know at least kevent(2)).
> >
> >netbsd32_execve() was missing the calls related to RAS support, which
> >showed that however close it was to the native sys_execve(), it had
> >drifted apart from it.
> >
> >The solution I propose for such syscalls that access an array in user
> >space is to use a helper function that takes an additional argument
> >which is a pointer to a function that will do the copyin() (or a
> >copyout(), in execve(2)'s case it's only a copyin() though), and the
> >translation in the compat_netbsd32 case.
> >
> >That way the actual syscall is only slowed down by a few function calls,
> >and remain readable.
> >
> >See for yourself with the attached patch for execve(2).
> >
> >There might be smarter ways of doing this, I'm open to suggestions.  But
> >keep in mind that we want to avoid as much as possible duplicating code,
> >but not at the expense of the native syscall's speed.
> >
> >The other solution is to teach people to think of compat_netbsd32
> >whenever they touch a syscall, but this has obviously failed, and is
> >doomed to fail anyway.
> >
> >Comments?
>=20
> I like it a lot because it simplifies the code and improves maintainabili=
ty
> because we don't need to maintain 2 copies.
>=20
> 1. I would have used size_t instead of u_int for the count.
> 2. Calling the function ptr should be (*foo)(args) not foo(args).
> 3. There is an extra space before paren in the line ... NULL )

Ok, I've changed that.

Now, what about the smart copyin/copyout?  Are they feasible?  If not,
will it be ok if I change quite a few syscalls to be passed a
copyout/copyin function as a parameter?  There's wait4, and I know I
could fix getitimer/setitimer in a similar way (setitimer is currently
broken in COMPAT_NETBSD32).

Actually, I think that almost all the rip-offs in compat/netbsd32 can
be fixed that way.  That means that for each of them I'll have to do
the following:
  o create a helper function for the syscall that does the work, while
    the sys_<syscall> function hardly does anything but getting the
    arguments for the syscall
  o make the helper function take a copyout/copyin function as a
    parameter so that the netbsd32 code can call it with a specialised
    function

Also, should the helper functions be kept in the namespace of the
syscall (like I'm declaring doexecve() in exec.h), or wouldn't it be
wiser to have them declared in some sort of compat_netbsd32.h which
makes their purpose explicit and avoid having all the dostuff()
functions everywhere in the system headers?

--=20
Quentin Garnier - cube@cubidou.net - cube@NetBSD.org
"When I find the controls, I'll go where I like, I'll know where I want
to be, but maybe for now I'll stay right here on a silent sea."
KT Tunstall, Silent Sea, Eye to the Telescope, 2004.

--GdbWtwDHkcXqP16f
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (NetBSD)

iQEVAwUBQtIBktgoQloHrPnoAQKQxAf/XnX5QNsQtPOcdLBIRGHc56dUqIMcL6fS
HAdhoXJ975x+W9pe8FKduZhg1HVjc6uVqv75rubQQMZLg4cld3MkVgrBgJX6V4qr
HWaBDAjAz7TAAt5rwICx0nrDegaHWCTH8Bn/ZogioPPuFb//bVE8PoAt1VDGUZ8z
DgZglQPh0pZ5M0rGjcGA+31dEpzhVgKtgdqj3r7OnKKP5QXp7GQ8UWh3rPtvdX5x
RCi4qc2yNUeY3tpoCAZZo0SF2ZTNWGDrxKsIaCB/a9e2lW8T5quggJk3vktMQUSM
0in9t2uYAR1AXmO/0RZCpEpeTu/VlyRMIgyrC9TLZx7u/zlL2unzrA==
=caJP
-----END PGP SIGNATURE-----

--GdbWtwDHkcXqP16f--