cvs commit: src/sys/dev/atkbdc atkbd.c src/sys/dev/digi digi.c src/sys/dev/kbdmux kbdmux.c src/sys/dev/syscons scvidctl.c syscons.c src/sys/dev/uart uart_kbd_sun.c src/sys/dev/usb ukbd.c src/sys/dev/vkbd vkbd.c src/sys/fs/procfs procfs_ioctl.c ...

[Ruslan Ermilov]

Hi John,

On Wed, Sep 27, 2006 at 05:03:42PM -0400, John Baldwin wrote:
> On Wednesday 27 September 2006 15:57, Ruslan Ermilov wrote:
> > ru          2006-09-27 19:57:02 UTC
> > 
> >   FreeBSD src repository
> > 
> >   Modified files:
> >     sys/dev/atkbdc       atkbd.c 
> >     sys/dev/digi         digi.c 
> >     sys/dev/kbdmux       kbdmux.c 
> >     sys/dev/syscons      scvidctl.c syscons.c 
> >     sys/dev/uart         uart_kbd_sun.c 
> >     sys/dev/usb          ukbd.c 
> >     sys/dev/vkbd         vkbd.c 
> >     sys/fs/procfs        procfs_ioctl.c 
> >     sys/kern             sys_generic.c tty_pts.c tty_pty.c 
> >     sys/modules/digi/digi Makefile 
> >     sys/modules/if_tap   Makefile 
> >     sys/modules/kbdmux   Makefile 
> >     sys/modules/procfs   Makefile 
> >     sys/modules/ukbd     Makefile 
> >     sys/modules/vkbd     Makefile 
> >     sys/net              if_tap.c if_tap.h 
> >     sys/pc98/cbus        pckbd.c 
> >     sys/sys              consio.h digiio.h ioccom.h kbio.h 
> >                          pioctl.h ttycom.h 
> >   Log:
> >   Fix our ioctl(2) implementation when the argument is "int".  New
> >   ioctls passing integer arguments should use the _IOWINT() macro.
> >   This fixes a lot of ioctl's not working on sparc64, most notable
> >   being keyboard/syscons ioctls.
> >   
> >   Full ABI compatibility is provided, with the bonus of fixing the
> >   handling of old ioctls on sparc64.
> 
> Eh?  You just changed ioctl values breaking ABI all over the place, e.g. 
> sys/pioctl.h.
> 
No, see also the sys/fs/procfs/procfs_ioctl.c part of a change.

> The size field changed from 0 to sizeof(int) meaning different 
> ioctl values and thus ABI breakage.
> 
No.  Old ioctls are still supported, just not in headers.

> Plus, what if you have:
> 
> 	struct foo {
> 		int bar;
> 	};
> 
> #define FOOIO	_IOW('y', 0, struct foo)
> 
> that's going to have the same issue isn't it?
> 
No.  In this case, you're passing a pointer.

Here's what happened when you called ioctl(fd, IOCTL, (int)1)
on sparc64.  "1" is passed in a register (64-bit).  Due to the
ioctl() prototype in the kernel, it sees 1 as "caddr_t data".
This means that you have "0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1"
copied on the stack location pointed to by uap->data.  Later
on, a conversion from "int" to "int *" is made, and a pointer
to that location of memory is passed to a device's ioctl()
handler.  It then accesses it as *(int *)arg and gets 0.

> This really just looks like a 
> big hack and it doesn't help with ABI compat at all. :(
> 
It does.  It has been tested on i386/amd64/sparc64, with
both old and new userland.

> I think instead the various ioctl handlers have to realize that for IOC_VOID 
> ioctls declared using _IO() data is a (caddr_t *), not an (int *) (the uap 
> struct for ioctl clearly defines data as a caddr_t).  Fix whatever crap you 
> have to in the kernel to deal with it, but don't change the userland ABI. :(
> 
POSIX and our manpage both say that an argument should either
be an "int" or a pointer.  This means we need to support "int"
arguments but do it properly.  My first version of the patch
fixed the kernel only, replacing "int" with intptr_t where a
kernel initiated an ioctl().  This is very inconvenient for
a driver writer, when a userland passes an "int" argument,
and a kernel has to pass an "intptr_t" argument, due to the
bogus implementation.

I think you were too quick in replying, and didn't look at
the whole patch in detail.  :-)


Cheers,
-- 
Ruslan Ermilov
ru at FreeBSD.org
FreeBSD committer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/cvs-src/attachments/20060927/08ef9fd3/attachment.pgp