Broken ioctl error returns (was Re: [PATCH 2/3] block: fail SCSIpassthrough ioctls on partition devices)

[Linus Torvalds]

On Thu, Jan 5, 2012 at 8:16 AM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> Just fix the *obvious* breakage in BLKROSET. It's clearly what the
> code *intends* to do, it just didn't check for ENOIOCTLCMD.

So it seems from quick grepping that the block layer isn't actually
all that confused apart from that one BLK[SG]ETRO issue, although I'm
sure there are crazy drivers that think that EINVAL is the right error
return.

The *really* confused layer seems to be the damn media drivers. The
confusion there seems to go very deep indeed, where for some crazy
reason the media people seem to have made it part of the semantics
that "if a driver doesn't support a particular ioctl, it returns
EINVAL".

Added, linux-media and Mauro to the Cc, because I'm about to commit
something like the attached patch to see if anything breaks. We may
have to revert it if things get too nasty, but we should have done
this years and years ago, so let's hope not.

Basic rules: ENOTTY means "I don't recognize this ioctl". Yes, the
name is odd, and yes, it's for historical Unix reasons. ioctl's were
originally a way to control mainly terminal settings - think termios -
so when you did an ioctl on a file, you'd get "I'm not a tty, dummy".
File flags were controlled with fcntl().

In contrast, EINVAL means "there is something wrong with the
arguments", which very much implies "I do recognize the ioctl".

And finally, ENOIOCTLCMD is a way to say ENOTTY in a sane manner, and
will now be turned into ENOTTY for the user space return (not EINVAL -
I have no idea where that idiocy came from, but it's clearly confused,
although it's also clearly very old).

This fixes the core files I noticed. It removes the *insane*
complaints from the compat_ioctl() (which would complain if you
returned ENOIOCTLCMD after an ioctl translation - the reason that is
totally insane is that somebody might use an ioctl on the wrong kind
of file descriptor, so even if it was translated perfectly fine,
ENOIOCTLCMD is a perfectly fine error return and shouldn't cause
warnings - and that allows us to remove stupid crap from the socket
ioctl code).

Does this break things and need to be reverted? Maybe. There could be
user code that tests *explicitly* for EINVAL and considers that the
"wrong ioctl number", even though it's the wrong error return.

And we may have those kinds of mistakes inside the kernel too. We did
in the block layer BLKSETRO code, for example, as pointed out by
Paulo. That one is fixed here, but there may be others.

I didn't change any media layers, since there it's clearly an endemic
problem, and there it seems to be used as a "we pass media ioctls down
and drivers should by definition recognize them, so if they don't, we
assume the driver is limited and doesn't support those particular
settings and return EINVAL".

But in general, any code like this is WRONG:

   switch (cmd) {
   case MYIOCTL:
      .. do something ..
   default:
      return -EINVAL;
   }

while something like this is CORRECT:

   switch (cmd) {
   case MYIOCT:
      if (arg)
         return -EINVAL;
      ...

   case OTHERIOCT:
      /* I recognize this one, but I don't support it */
      return -EINVAL;

   default:
      return -ENOIOCTLCMD; // Or -ENOTTY - see below about the difference
   }

where right now we do have some magic differences between ENOIOCTLCMD
and ENOTTY (the compat layer will try to do a translated ioctl *only*
if it gets ENOIOCTLCMD, iirc, so ENOTTY basically means "this is my
final answer").

I'll try it out on my own setup here to see what problems I can
trigger, but I thought I'd send it out first just as (a) a heads-up
and (b) to let others try it out and see.. See the block/ioctl.c code
for an example of the kinds of things we may need even just inside the
kernel (and the kinds of things that could cause problems for
user-space that makes a difference between EINVAL and ENOTTY).

                             Linus
 block/ioctl.c     |   26 ++++++++++++++++++++++----
 fs/compat_ioctl.c |    9 ++-------
 fs/ioctl.c        |    2 +-
 net/socket.c      |   16 +---------------
 4 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index ca939fc1030f..af8363e775a8 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -180,6 +180,26 @@ int __blkdev_driver_ioctl(struct block_device *bdev, fmode_t mode,
 EXPORT_SYMBOL_GPL(__blkdev_driver_ioctl);
 
 /*
+ * Is it an unrecognized ioctl? The correct returns are either
+ * ENOTTY (final) or ENOIOCTLCMD ("I don't know this one, try a
+ * fallback"). ENOIOCTLCMD gets turned into ENOTTY by the ioctl
+ * code before returning.
+ *
+ * Confused drivers sometimes return EINVAL, which is wrong. It
+ * means "I understood the ioctl command, but the parameters to
+ * it were wrong".
+ *
+ * We should aim to just fix the broken drivers, the EINVAL case
+ * should go away.
+ */
+static inline int is_unrecognized_ioctl(int ret)
+{
+	return	ret == -EINVAL ||
+		ret == -ENOTTY ||
+		ret == ENOIOCTLCMD;
+}
+
+/*
  * always keep this in sync with compat_blkdev_ioctl()
  */
 int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
@@ -196,8 +216,7 @@ int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 			return -EACCES;
 
 		ret = __blkdev_driver_ioctl(bdev, mode, cmd, arg);
-		/* -EINVAL to handle old uncorrected drivers */
-		if (ret != -EINVAL && ret != -ENOTTY)
+		if (!is_unrecognized_ioctl(ret))
 			return ret;
 
 		fsync_bdev(bdev);
@@ -206,8 +225,7 @@ int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 
 	case BLKROSET:
 		ret = __blkdev_driver_ioctl(bdev, mode, cmd, arg);
-		/* -EINVAL to handle old uncorrected drivers */
-		if (ret != -EINVAL && ret != -ENOTTY)
+		if (!is_unrecognized_ioctl(ret))
 			return ret;
 		if (!capable(CAP_SYS_ADMIN))
 			return -EACCES;
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 51352de88ef1..5c14e8d428a5 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -1621,13 +1621,8 @@ asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd,
 		goto found_handler;
 
 	error = do_ioctl_trans(fd, cmd, arg, filp);
-	if (error == -ENOIOCTLCMD) {
-		static int count;
-
-		if (++count <= 50)
-			compat_ioctl_error(filp, fd, cmd, arg);
-		error = -EINVAL;
-	}
+	if (error == -ENOIOCTLCMD)
+		error = -ENOTTY;
 
 	goto out_fput;
 
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 1d9b9fcb2db4..066836e81848 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -42,7 +42,7 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd,
 
 	error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
 	if (error == -ENOIOCTLCMD)
-		error = -EINVAL;
+		error = -ENOTTY;
  out:
 	return error;
 }
diff --git a/net/socket.c b/net/socket.c
index 2877647f347b..a0053750e37a 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2883,7 +2883,7 @@ static int bond_ioctl(struct net *net, unsigned int cmd,
 
 		return dev_ioctl(net, cmd, uifr);
 	default:
-		return -EINVAL;
+		return -ENOIOCTLCMD;
 	}
 }
 
@@ -3210,20 +3210,6 @@ static int compat_sock_ioctl_trans(struct file *file, struct socket *sock,
 		return sock_do_ioctl(net, sock, cmd, arg);
 	}
 
-	/* Prevent warning from compat_sys_ioctl, these always
-	 * result in -EINVAL in the native case anyway. */
-	switch (cmd) {
-	case SIOCRTMSG:
-	case SIOCGIFCOUNT:
-	case SIOCSRARP:
-	case SIOCGRARP:
-	case SIOCDRARP:
-	case SIOCSIFLINK:
-	case SIOCGIFSLAVE:
-	case SIOCSIFSLAVE:
-		return -EINVAL;
-	}
-
 	return -ENOIOCTLCMD;
 }