Date: Wed, 20 Sep 2006 01:56:05 +0400 From: Ruslan Ermilov <ru@freebsd.org> To: Maksim Yevmenkin <maksim.yevmenkin@gmail.com> Cc: cvs-src@freebsd.org, Marius Strobl <marius@freebsd.org>, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/dev/kbdmux kbdmux.c Message-ID: <20060919215605.GB44298@rambler-co.ru> In-Reply-To: <bb4a86c70609191415m59a9ef89x91f25e6912339390@mail.gmail.com> References: <200609191303.k8JD3AHl050783@repoman.freebsd.org> <bb4a86c70609190944o2438a4a4vae7b3bb2332522ee@mail.gmail.com> <20060919190645.GA23068@rambler-co.ru> <bb4a86c70609191236j13fe1563w123cb046261ee129@mail.gmail.com> <20060919194819.GA23360@rambler-co.ru> <bb4a86c70609191300t3bea8380qa1898d1a89b6fc27@mail.gmail.com> <20060919203608.GE23360@rambler-co.ru> <bb4a86c70609191415m59a9ef89x91f25e6912339390@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--cmJC7u66zC7hs+87
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Tue, Sep 19, 2006 at 02:15:56PM -0700, Maksim Yevmenkin wrote:
> >> sparc64 crashes with (intptr_t *) casting (tested locally)
> >>
> >We need to find *where*, and find a proper fix.
>=20
> that will take some time, as ultra5 is very slow at compiling kernel :)
>=20
No need to, I know *where*. The problem is indeed with these
brain-damaged IOCTLs that are passed as "int" from userland
and as "int *" from kernelland.
> >not crash !=3D work properly. Can you print the value of argument
> >to KDSETLED and verify it's correct? Print it both as
> >(int)*(intptr_t *)arg and *(int *)arg, and compare.
>=20
> ok, it works from user's point of view, i.e. i press caps, num lock
> etc. and the light comes on/off on keyboard(s).
>=20
Yes, it works as long as you don't use these IOCTLs from
userland (which you typically don't).
> >> i tested (int *) casting on sparc64 with SETLED, SETREPEAT and it
> >> works fine. i made sure that parameters passed via ioctl() are all
> >> correct. i will do the same test on amd64 before committing.
> >>
> >KDSETREPEAT _does_ use *(int *) in the committed code, as it's
> >a normal "_IOW('K', 102, keyboard_repeat_t)" which takes a pointer
> >as an argument.
>=20
> yes, so?
>=20
I just confirmed that it's okay that it uses *(int *)arg for
KDSETREPEAT.
> >KDSETLED isn't used outside the kernel, so I assume you tested it
> >only when it's called from within the kernel? If so, try passing
> >it from useland to see the endianness problem; I'm pretty sure it
> >will fire. If it does, there are two options:
>=20
> ok, i see what you are saying now. i will try to use KDSETLED from=20
> userspace.
>=20
I already did, thanks to your vkbd(4):
# uname -p
sparc64
# ./a < /dev/vkbdctl0
current mode =3D 1
current mode =3D 0
> >- Fix in-kernel callers of KDSETLED to behave like userland.
> >- Fix the definition of KDSETLED to be:
> >
> >#define KDSETLED _IOW('K', 66, int)
> >
> >and then fix kbdmux.c to use *(int *) when it accesses KDSETLED's
> >argument.
>=20
> ok
>=20
Since it's obvious that it doesn't "work" on sparc64 in all
cases, we need to choose
> >Similarly for KDSKBSTATE. KDSETRAD should still be accessed
> >through (int)*(intptr_t *)arg to stay binary compatible.
>=20
> or perhaps as keyboard_repeat_t
>=20
KDSETRAD cannot use keyboard_repeat_t. It's an old interface
for binary compatibility (FWICT). If we change it we:
1) break the old API and binary compatibility
2) provide two identical IOCTLs
what's the point? It's also used (well, not probably in
real life) by kbdcontrol.c.
=2E/usr.sbin/kbdcontrol/kbdcontrol.c: if (ioctl(0, KDSETRAD, (d=
<< 5) | r))
> >> also, like i said before, all other keyboard drivers, including
> >> sunkbd(4) use (int *) cast and NOT (intptr_t *), and it seems to work
> >> just fine.
> >>
> >I think this problem is unnoticeable because you're talking about
> >the KDSETLED here, and it's only used inside the kernel, and
> >inside the kernel it's passed as a "pointer to int", while in
> >case of userland it would be passed as a 64-bit value on the
> >stack (on sparc64), and causing an endianness problem when
> >accessed through *(int *)arg.
>=20
> from quick search, i can see that SETLED, KBSKBMODE and KDBSKBSTATE
> are called from kernel with (caddr_t) &foo, i.e. pointer to int value.
>=20
Yes, in all cases. And this doesn't match the IOCTL's prototype
and hence userland.
> from another quick search in /usr/src/{usr.}{s}bin/ i do not see any
> userspace code that uses these ioctl() directly (and, yes, it does not
> mean that it does not exists).
>=20
> so, i propose
>=20
> 1) we change kbd/syscons/etc kernel code back to its default
> assumption that KBxxx ioctls defined as _IO(foo, bar) accept 3rd
> parameter as a pointer to int.
>=20
Yes.
> 2) we change
>=20
> KDSETLED _IO('K', 66, /* int */)
>=20
> to
>=20
> KDSETLED _IO('K', 66, /* (int *) */)
>=20
> so its "documented".
>=20
No, this isn't going to work and is plain incorrect.
We need to change it to "_IOW('K', 66, int)" to make it work.
That means that instead of
ioctl(0, KDSETLED, mode)
applications will have to do:
ioctl(0, KDSETLED, &mode)
Then they will match the kernel.
Let me explain (again) how _IO*'s are treated by ioctl().
_IOW('K', 66, int) means: the argument is a pointer to sizeof(int)
bytes of user memory. sysctl() will malloc sizeof(int) bytes of
memory in the kernel, and copyin() the data from userland.
_IO('K', 66 /*, whatever */) means: the argument is passed
by a value as an "int". The kernel then "emulates" the
pointer by assigning the pointer variable "data" an on-stack
address of this argument. The relevant code in sys_generic.c:
: caddr_t data, memp;
[...]
: if (size > 0) {
: memp =3D malloc((u_long)size, M_IOCTLOPS, M_WAITOK);
: data =3D memp;
: } else {
: memp =3D NULL;
: data =3D (void *)&uap->data;
: }
: if (com & IOC_IN) {
: error =3D copyin(uap->data, data, (u_int)size);
: if (error) {
: free(memp, M_IOCTLOPS);
: return (error);
: }
:=20
Here, "size" will be 4 for _IOW, and 0 for _IO. In case of
_IO, and the passed argument of say "8", and sparc64, the
memory "data" points to will be "0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 8".
If we access it through *(int *)data, it will be first 8
bytes which are 0. If we access it as (int)*(intptr_t *)data
it will be (int)8, i.e., the passed value.
> 3) take an "ostrich approach" (put our heads in the sand) to any 3rd
> party application that might use SETLED, KBSKBMODE etc. ioctls
> directly.
>=20
> the above requires minimal code change, which is, imo, is a good thing.
>=20
The proper fix would be to fix the kernel to pass an argument by
value, like the userland does. But from the practical point of
view, it may make sense to change the API and say that these
IOCTLs now take a pointer type argument. I'm not sure. If you
know of any applications (mine test util not counting :-) that
use KDSKBMODE/KDSETLED/KDSKBSTATE, then we should probably fix
the kernel callers.
Cheers,
--=20
Ruslan Ermilov
ru@FreeBSD.org
FreeBSD committer
--cmJC7u66zC7hs+87
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (FreeBSD)
iD8DBQFFEGd1qRfpzJluFF4RAuXbAJ4x0X6LBUbvj2YUOxOEJ6b+jSsJQgCgit9B
g8FPG+jVf2yj01/7tz1Z2H0=
=hFri
-----END PGP SIGNATURE-----
--cmJC7u66zC7hs+87--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060919215605.GB44298>