From patchwork Thu Jan 5 17:02:00 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Linus Torvalds X-Patchwork-Id: 9382 Return-path: Envelope-to: mchehab@canuck.infradead.org Delivery-date: Thu, 05 Jan 2012 17:02:49 +0000 Received: from canuck.infradead.org [134.117.69.58] by gaivota with IMAP (fetchmail-6.3.20) for (single-drop); Thu, 05 Jan 2012 15:03:48 -0200 (BRST) Received: from casper.infradead.org ([2001:770:15f::2]) by canuck.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1RiqiH-0003sq-0F for mchehab@canuck.infradead.org; Thu, 05 Jan 2012 17:02:49 +0000 Received: from vger.kernel.org ([209.132.180.67]) by casper.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1RiqiF-0008UB-4p; Thu, 05 Jan 2012 17:02:47 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755225Ab2AERCX (ORCPT + 1 other); Thu, 5 Jan 2012 12:02:23 -0500 Received: from mail-yw0-f46.google.com ([209.85.213.46]:49174 "EHLO mail-yw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753094Ab2AERCW (ORCPT ); Thu, 5 Jan 2012 12:02:22 -0500 Received: by yhnn56 with SMTP id n56so220050yhn.19 for ; Thu, 05 Jan 2012 09:02:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:sender:from:date:x-google-sender-auth:message-id :subject:to:cc:content-type; bh=DzN6UYY4H4yysi3kFTB3FA316WSmCdngt3+oR1xZiMw=; b=qMpvG9hv3HNRTF9xKYO9wWLCi1jXGvMlIkcZQaC3vIGYDSzWqkW0eyfIOdPBoc81Kg 4FXjt8UCGZrFfuFTvvGiFjOvL4ljrPtz9VR9J5FlkD6q1ikhS8vAQNjM2KlKB/r/5rjr XsncVqsGwqJAQd9X/re71G9DUnaJYJVDYH5DM= Received: by 10.236.9.40 with SMTP id 28mr2675646yhs.29.1325782941207; Thu, 05 Jan 2012 09:02:21 -0800 (PST) MIME-Version: 1.0 Received: by 10.236.175.170 with HTTP; Thu, 5 Jan 2012 09:02:00 -0800 (PST) From: Linus Torvalds Date: Thu, 5 Jan 2012 09:02:00 -0800 X-Google-Sender-Auth: VOUHWb2SEwmgM8uVd0rZlwTw_vQ Message-ID: Subject: Broken ioctl error returns (was Re: [PATCH 2/3] block: fail SCSI passthrough ioctls on partition devices) To: Paolo Bonzini , Mauro Carvalho Chehab , linux-media@vger.kernel.org Cc: Willy Tarreau , linux-kernel@vger.kernel.org, security@kernel.org, pmatouse@redhat.com, agk@redhat.com, jbottomley@parallels.com, mchristi@redhat.com, msnitzer@redhat.com, Christoph Hellwig Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On Thu, Jan 5, 2012 at 8:16 AM, Linus Torvalds 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; }