[01/11] compat_ioctl: remove keyboard ioctl translation

Message ID 20180908142837.2819693-1-arnd@arndb.de
State Superseded
Headers show
Series
  • [01/11] compat_ioctl: remove keyboard ioctl translation
Related show

Commit Message

Arnd Bergmann Sept. 8, 2018, 2:28 p.m.
The KD* family of ioctls is implemented in two drivers:
drivers/tty/vt and drivers/s390/char/tty3270.c. Both of them
have compat handlers for all their ioctl commands, so translation
in fs/compat_ioctl.c is never used.

Commit fb07a5f857ac ("compat_ioctl: remove all VT ioctl handling")
removed the compat handling for all the other VT ioctls back in
2009, but it seems I missed the keyboard ones back then.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 fs/compat_ioctl.c | 26 --------------------------
 1 file changed, 26 deletions(-)

-- 
2.18.0

Comments

Al Viro Sept. 9, 2018, 4:16 a.m. | #1
On Sat, Sep 08, 2018 at 04:28:09PM +0200, Arnd Bergmann wrote:
> The SNDCTL_* and SOUND_* commands are the old OSS user interface.

> 

> I checked all the sound ioctl commands listed in fs/compat_ioctl.c

> to see if we still need the translation handlers. Here is what I

> found:

> 

> - sound/oss/ is (almost) gone from the kernel, this is what actually

>   needed all the translations

> - The ALSA emulation for OSS correctly handles all compat_ioctl

>   commands already.

> - sound/oss/dmasound/ is the last holdout of the original OSS code,

>   this is only used on arch/m68k, which has no 64-bit mode and

>   hence needs no compat handlers

> - arch/um/drivers/hostaudio_kern.c may run in 64-bit mode with

>   32-bit x86 user space underneath it. This rare corner case is

>   the only one that still needs the compat handlers.

> 

> By adding a simple redirect of .compat_ioctl to .unlocked_ioctl in the

> UML driver, we can remove all the COMPATIBLE_IOCTL() annotations without

> a change in functionality. For completeness, I'm adding the same thing

> to the dmasound file, knowing that it makes no difference.


> diff --git a/arch/um/drivers/hostaudio_kern.c b/arch/um/drivers/hostaudio_kern.c

> index 7f9dbdbc4eb7..0278a642a622 100644

> --- a/arch/um/drivers/hostaudio_kern.c

> +++ b/arch/um/drivers/hostaudio_kern.c

> @@ -298,6 +298,7 @@ static const struct file_operations hostaudio_fops = {

>  	.write          = hostaudio_write,

>  	.poll           = hostaudio_poll,

>  	.unlocked_ioctl	= hostaudio_ioctl,

> +	.compat_ioctl	= hostaudio_ioctl,


Umm...  OK, seeing that it's not going to be used on s390...  It's still
not quite right, though, and I'm afraid that places where we have the
same ->unlocked_ioctl and ->compat_ioctl need an audit - there probably
had been other folks who'd stepped into the same.
Al Viro Sept. 9, 2018, 4:37 a.m. | #2
On Sat, Sep 08, 2018 at 04:28:17PM +0200, Arnd Bergmann wrote:
> MTIOCPOS and MTIOCGET are incompatible between 32-bit and 64-bit user

> space, and traditionally have been translated in fs/compat_ioctl.c.

> 

> To get rid of that translation handler, move a corresponding

> implementation into each of the four drivers implementing those commands.

> 

> The interesting part of that is now in a new linux/mtio.h header that

> wraps the existing uapi/linux/mtio.h header and provides an abstraction

> to let drivers handle both cases easily.


Ugh...  Frankly, this bool compat passed all way down looks wrong.
I can live with that; the question is whether block folks will be
OK with that thing...
Arnd Bergmann Sept. 11, 2018, 3:36 p.m. | #3
On Sun, Sep 9, 2018 at 6:38 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>

> On Sat, Sep 08, 2018 at 04:28:17PM +0200, Arnd Bergmann wrote:

> > MTIOCPOS and MTIOCGET are incompatible between 32-bit and 64-bit user

> > space, and traditionally have been translated in fs/compat_ioctl.c.

> >

> > To get rid of that translation handler, move a corresponding

> > implementation into each of the four drivers implementing those commands.

> >

> > The interesting part of that is now in a new linux/mtio.h header that

> > wraps the existing uapi/linux/mtio.h header and provides an abstraction

> > to let drivers handle both cases easily.

>

> Ugh...  Frankly, this bool compat passed all way down looks wrong.

> I can live with that; the question is whether block folks will be

> OK with that thing...


I have tried to come up with an alternative, but couldn't really find anything
that is less ugly. Since nobody else complained, I'll resend this version
along with the other patches.

       Arnd
Arnd Bergmann Sept. 11, 2018, 7:35 p.m. | #4
On Sun, Sep 9, 2018 at 6:17 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>

> On Sat, Sep 08, 2018 at 04:28:09PM +0200, Arnd Bergmann wrote:

> > The SNDCTL_* and SOUND_* commands are the old OSS user interface.

> >

> > I checked all the sound ioctl commands listed in fs/compat_ioctl.c

> > to see if we still need the translation handlers. Here is what I

> > found:

> >

> > - sound/oss/ is (almost) gone from the kernel, this is what actually

> >   needed all the translations

> > - The ALSA emulation for OSS correctly handles all compat_ioctl

> >   commands already.

> > - sound/oss/dmasound/ is the last holdout of the original OSS code,

> >   this is only used on arch/m68k, which has no 64-bit mode and

> >   hence needs no compat handlers

> > - arch/um/drivers/hostaudio_kern.c may run in 64-bit mode with

> >   32-bit x86 user space underneath it. This rare corner case is

> >   the only one that still needs the compat handlers.

> >

> > By adding a simple redirect of .compat_ioctl to .unlocked_ioctl in the

> > UML driver, we can remove all the COMPATIBLE_IOCTL() annotations without

> > a change in functionality. For completeness, I'm adding the same thing

> > to the dmasound file, knowing that it makes no difference.

>

> > diff --git a/arch/um/drivers/hostaudio_kern.c b/arch/um/drivers/hostaudio_kern.c

> > index 7f9dbdbc4eb7..0278a642a622 100644

> > --- a/arch/um/drivers/hostaudio_kern.c

> > +++ b/arch/um/drivers/hostaudio_kern.c

> > @@ -298,6 +298,7 @@ static const struct file_operations hostaudio_fops = {

> >       .write          = hostaudio_write,

> >       .poll           = hostaudio_poll,

> >       .unlocked_ioctl = hostaudio_ioctl,

> > +     .compat_ioctl   = hostaudio_ioctl,

>

> Umm...  OK, seeing that it's not going to be used on s390...  It's still

> not quite right, though, and I'm afraid that places where we have the

> same ->unlocked_ioctl and ->compat_ioctl need an audit - there probably

> had been other folks who'd stepped into the same.


It turns out that it's actually much more common to have the same
pointer for ioctl handlers that thake pointer arguments than to have
the wrapper. I found 16 instances of trivial wrappers to do compat_ptr()
for file_operations, 4 instances that have a wrapper but skip the
compat_ptr() and around 40 (depending on how you count) that use
the same pointer for both when they only use pointers and should
go through compat_ptr(). This includes a couple that don't use
the argument at all, so they are fine either way.

I've created patches to change all of the above to a new
generic_compat_ioctl_ptrarg() helper I added.

loop_control_ioctl(), kcov_ioctl(), proc_bus_pci_ioctl(), and nbd_ioctl() seem
to be ones that are correct because,  the argument is always an integer.
fs3270_ioctl() has a is_compat_task() check to deal with the problem.

inotify_ioctl(),  vsoc_ioctl(), and usblp_ioctl() are somethat uses both integer
and pointer arguments and may need special handling for s390.

I did not touch those so far.

       Arnd
Arnd Bergmann Sept. 11, 2018, 8:13 p.m. | #5
On Tue, Sep 11, 2018 at 5:36 PM Arnd Bergmann <arnd@arndb.de> wrote:
>

> On Sun, Sep 9, 2018 at 6:38 AM Al Viro <viro@zeniv.linux.org.uk> wrote:

> >

> > On Sat, Sep 08, 2018 at 04:28:17PM +0200, Arnd Bergmann wrote:

> > > MTIOCPOS and MTIOCGET are incompatible between 32-bit and 64-bit user

> > > space, and traditionally have been translated in fs/compat_ioctl.c.

> > >

> > > To get rid of that translation handler, move a corresponding

> > > implementation into each of the four drivers implementing those commands.

> > >

> > > The interesting part of that is now in a new linux/mtio.h header that

> > > wraps the existing uapi/linux/mtio.h header and provides an abstraction

> > > to let drivers handle both cases easily.

> >

> > Ugh...  Frankly, this bool compat passed all way down looks wrong.

> > I can live with that; the question is whether block folks will be

> > OK with that thing...

>

> I have tried to come up with an alternative, but couldn't really find anything

> that is less ugly. Since nobody else complained, I'll resend this version

> along with the other patches.


Actually, there was one idea that Deepa mentioned for another subsystem
with a similar issue: instead of passing down the fact that we come from
a compat syscall through multiple function calls, the lowest ones
(put_user_mtpos, put_user_mtget) could call in_compat_syscall().

Would you prefer that?

      Arnd

Patch

diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 9237076bdcf5..4c2f83a386a2 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -546,23 +546,6 @@  COMPATIBLE_IOCTL(FIGETBSZ)
 COMPATIBLE_IOCTL(FIFREEZE)
 COMPATIBLE_IOCTL(FITHAW)
 COMPATIBLE_IOCTL(FITRIM)
-COMPATIBLE_IOCTL(KDGETKEYCODE)
-COMPATIBLE_IOCTL(KDSETKEYCODE)
-COMPATIBLE_IOCTL(KDGKBTYPE)
-COMPATIBLE_IOCTL(KDGETMODE)
-COMPATIBLE_IOCTL(KDGKBMODE)
-COMPATIBLE_IOCTL(KDGKBMETA)
-COMPATIBLE_IOCTL(KDGKBENT)
-COMPATIBLE_IOCTL(KDSKBENT)
-COMPATIBLE_IOCTL(KDGKBSENT)
-COMPATIBLE_IOCTL(KDSKBSENT)
-COMPATIBLE_IOCTL(KDGKBDIACR)
-COMPATIBLE_IOCTL(KDSKBDIACR)
-COMPATIBLE_IOCTL(KDGKBDIACRUC)
-COMPATIBLE_IOCTL(KDSKBDIACRUC)
-COMPATIBLE_IOCTL(KDKBDREP)
-COMPATIBLE_IOCTL(KDGKBLED)
-COMPATIBLE_IOCTL(KDGETLED)
 #ifdef CONFIG_BLOCK
 /* Big S */
 COMPATIBLE_IOCTL(SCSI_IOCTL_GET_IDLUN)
@@ -974,15 +957,6 @@  static long do_ioctl_trans(unsigned int cmd,
 	case HOT_ADD_DISK:
 	case SET_DISK_FAULTY:
 	case SET_BITMAP_FILE:
-	/* Big K */
-	case KDSIGACCEPT:
-	case KIOCSOUND:
-	case KDMKTONE:
-	case KDSETMODE:
-	case KDSKBMODE:
-	case KDSKBMETA:
-	case KDSKBLED:
-	case KDSETLED:
 		return vfs_ioctl(file, cmd, arg);
 	}