[PATCH 0/5] drivers: fix up ENOIOCTLCMD error handling

[PATCH 0/5] drivers: fix up ENOIOCTLCMD error handling

[Wanlong Gao]

At this commit,

Cc: Linus Torvalds <torvalds@linux-foundation.org>
commit 07d106d0a33d6063d2061305903deb02489eba20
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu Jan 5 15:40:12 2012 -0800

    vfs: fix up ENOIOCTLCMD error handling
    
    We're doing some odd things there, which already messes up various users
    (see the net/socket.c code that this removes), and it was going to add
    yet more crud to the block layer because of the incorrect error code
    translation.
    
    ENOIOCTLCMD is not an error return that should be returned to user mode
    from the "ioctl()" system call, but it should *not* be translated as
    EINVAL ("Invalid argument").  It should be translated as ENOTTY
    ("Inappropriate ioctl for device").
    
    That EINVAL confusion has apparently so permeated some code that the
    block layer actually checks for it, which is sad.  We continue to do so
    for now, but add a big comment about how wrong that is, and we should
    remove it entirely eventually.  In the meantime, this tries to keep the
    changes localized to just the EINVAL -> ENOTTY fix, and removing code
    that makes it harder to do the right thing.
    
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>


Linus pointed out that ENOIOCTLCMD should be translated as ENOTTY to user
mode, and we should fix up the broken drivers then.

Wanlong Gao (5):
  drivers:tty:fix up ENOIOCTLCMD error handling
  net:atm:fix up ENOIOCTLCMD error handling
  media:dvb:fix up ENOIOCTLCMD error handling
  video:omap3isp:fix up ENOIOCTLCMD error handling
  s390:block:fix up ENOIOCTLCMD error handling

 drivers/media/dvb/dvb-core/dvbdev.c     |  2 +-
 drivers/media/video/omap3isp/ispvideo.c | 10 +++++-----
 drivers/s390/block/dasd_ioctl.c         |  2 +-
 drivers/tty/tty_io.c                    |  2 +-
 net/atm/resources.c                     |  2 +-
 5 files changed, 9 insertions(+), 9 deletions(-)

-- 
1.7.12

[PATCH 1/5] drivers:tty:fix up ENOIOCTLCMD error handling

[Wanlong Gao]

At commit 07d106d0, Linus pointed out that ENOIOCTLCMD should be
translated as ENOTTY to user mode.
For example:
	fd = open("/dev/tty", O_RDWR);
	ioctl(fd, -1, &argp);

then the errno should be ENOTTY but not EINVAL.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 drivers/tty/tty_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b425c79..720c408f 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2756,7 +2756,7 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	if (ld->ops->ioctl) {
 		retval = ld->ops->ioctl(tty, file, cmd, arg);
 		if (retval == -ENOIOCTLCMD)
-			retval = -EINVAL;
+			retval = -ENOTTY;
 	}
 	tty_ldisc_deref(ld);
 	return retval;
-- 
1.7.12

[PATCH 2/5] net:atm:fix up ENOIOCTLCMD error handling

[Wanlong Gao]

At commit 07d106d0, Linus pointed out that ENOIOCTLCMD should be
translated as ENOTTY to user mode.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 net/atm/resources.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/atm/resources.c b/net/atm/resources.c
index 23f45ce..0447d5d 100644
--- a/net/atm/resources.c
+++ b/net/atm/resources.c
@@ -432,7 +432,7 @@ int atm_dev_ioctl(unsigned int cmd, void __user *arg, int compat)
 			size = dev->ops->ioctl(dev, cmd, buf);
 		}
 		if (size < 0) {
-			error = (size == -ENOIOCTLCMD ? -EINVAL : size);
+			error = (size == -ENOIOCTLCMD ? -ENOTTY : size);
 			goto done;
 		}
 	}
-- 
1.7.12

[PATCH 3/5] media:dvb:fix up ENOIOCTLCMD error handling

[Wanlong Gao]

At commit 07d106d0, Linus pointed out that ENOIOCTLCMD should be
translated as ENOTTY to user mode.

Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
Cc: linux-media@vger.kernel.org
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 drivers/media/dvb/dvb-core/dvbdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb/dvb-core/dvbdev.c b/drivers/media/dvb/dvb-core/dvbdev.c
index 39eab73..d33101a 100644
--- a/drivers/media/dvb/dvb-core/dvbdev.c
+++ b/drivers/media/dvb/dvb-core/dvbdev.c
@@ -420,7 +420,7 @@ int dvb_usercopy(struct file *file,
 	/* call driver */
 	mutex_lock(&dvbdev_mutex);
 	if ((err = func(file, cmd, parg)) == -ENOIOCTLCMD)
-		err = -EINVAL;
+		err = -ENOTTY;
 	mutex_unlock(&dvbdev_mutex);
 
 	if (err < 0)
-- 
1.7.12

[PATCH 4/5] video:omap3isp:fix up ENOIOCTLCMD error handling

[Wanlong Gao]

At commit 07d106d0, Linus pointed out that ENOIOCTLCMD should be
translated as ENOTTY to user mode.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
Cc: linux-media@vger.kernel.org
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 drivers/media/video/omap3isp/ispvideo.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/video/omap3isp/ispvideo.c b/drivers/media/video/omap3isp/ispvideo.c
index b37379d..2dd982e 100644
--- a/drivers/media/video/omap3isp/ispvideo.c
+++ b/drivers/media/video/omap3isp/ispvideo.c
@@ -337,7 +337,7 @@ __isp_video_get_format(struct isp_video *video, struct v4l2_format *format)
 	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
 	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
 	if (ret == -ENOIOCTLCMD)
-		ret = -EINVAL;
+		ret = -ENOTTY;
 
 	mutex_unlock(&video->mutex);
 
@@ -723,7 +723,7 @@ isp_video_try_format(struct file *file, void *fh, struct v4l2_format *format)
 	fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
 	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
 	if (ret)
-		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
+		return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
 
 	isp_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
 	return 0;
@@ -744,7 +744,7 @@ isp_video_cropcap(struct file *file, void *fh, struct v4l2_cropcap *cropcap)
 	ret = v4l2_subdev_call(subdev, video, cropcap, cropcap);
 	mutex_unlock(&video->mutex);
 
-	return ret == -ENOIOCTLCMD ? -EINVAL : ret;
+	return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
 }
 
 static int
@@ -771,7 +771,7 @@ isp_video_get_crop(struct file *file, void *fh, struct v4l2_crop *crop)
 	format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
 	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &format);
 	if (ret < 0)
-		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
+		return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
 
 	crop->c.left = 0;
 	crop->c.top = 0;
@@ -796,7 +796,7 @@ isp_video_set_crop(struct file *file, void *fh, struct v4l2_crop *crop)
 	ret = v4l2_subdev_call(subdev, video, s_crop, crop);
 	mutex_unlock(&video->mutex);
 
-	return ret == -ENOIOCTLCMD ? -EINVAL : ret;
+	return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
 }
 
 static int
-- 
1.7.12

[PATCH 5/5] s390:block:fix up ENOIOCTLCMD error handling

[Wanlong Gao]

At commit 07d106d0, Linus pointed out that ENOIOCTLCMD should be
translated as ENOTTY to user mode.

Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: linux390@de.ibm.com
Cc: linux-s390@vger.kernel.org (open list:S390)
Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 drivers/s390/block/dasd_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
index cceae70..809a89b 100644
--- a/drivers/s390/block/dasd_ioctl.c
+++ b/drivers/s390/block/dasd_ioctl.c
@@ -501,7 +501,7 @@ int dasd_ioctl(struct block_device *bdev, fmode_t mode,
 		if (base->discipline->ioctl) {
 			rc = base->discipline->ioctl(block, cmd, argp);
 			if (rc == -ENOIOCTLCMD)
-				rc = -EINVAL;
+				rc = -ENOTTY;
 		} else
 			rc = -EINVAL;
 	}
-- 
1.7.12

Re: [PATCH 5/5] s390:block:fix up ENOIOCTLCMD error handling

[Heiko Carstens]

On Mon, Aug 27, 2012 at 03:23:16PM +0800, Wanlong Gao wrote:
> At commit 07d106d0, Linus pointed out that ENOIOCTLCMD should be
> translated as ENOTTY to user mode.
> 
[...]

> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> ---
>  drivers/s390/block/dasd_ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
> index cceae70..809a89b 100644
> --- a/drivers/s390/block/dasd_ioctl.c
> +++ b/drivers/s390/block/dasd_ioctl.c
> @@ -501,7 +501,7 @@ int dasd_ioctl(struct block_device *bdev, fmode_t mode,
>  		if (base->discipline->ioctl) {
>  			rc = base->discipline->ioctl(block, cmd, argp);
>  			if (rc == -ENOIOCTLCMD)
> -				rc = -EINVAL;
> +				rc = -ENOTTY;
>  		} else
>  			rc = -EINVAL;
>  	}

Thanks, but you missed the else path. I'm going to commit the patch below
unless Stefan has any objections:

>From dac16bd8b314dc6f3f4e6815feab199fdfc8cddd Mon Sep 17 00:00:00 2001
From: Heiko Carstens <heiko.carstens@de.ibm.com>
Date: Mon, 27 Aug 2012 10:59:42 +0200
Subject: [PATCH] s390/dasd: fix ioctl return value

For unimplemented ioctls the dasd driver should return -ENOTTY.

Reported-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 drivers/s390/block/dasd_eckd.c  | 2 +-
 drivers/s390/block/dasd_ioctl.c | 7 ++-----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 40a826a..2fb2b9e 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -3804,7 +3804,7 @@ dasd_eckd_ioctl(struct dasd_block *block, unsigned int cmd, void __user *argp)
 	case BIODASDSYMMIO:
 		return dasd_symm_io(device, argp);
 	default:
-		return -ENOIOCTLCMD;
+		return -ENOTTY;
 	}
 }
 
diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
index cceae70..654c692 100644
--- a/drivers/s390/block/dasd_ioctl.c
+++ b/drivers/s390/block/dasd_ioctl.c
@@ -498,12 +498,9 @@ int dasd_ioctl(struct block_device *bdev, fmode_t mode,
 		break;
 	default:
 		/* if the discipline has an ioctl method try it. */
-		if (base->discipline->ioctl) {
+		rc = -ENOTTY;
+		if (base->discipline->ioctl)
 			rc = base->discipline->ioctl(block, cmd, argp);
-			if (rc == -ENOIOCTLCMD)
-				rc = -EINVAL;
-		} else
-			rc = -EINVAL;
 	}
 	dasd_put_device(base);
 	return rc;
-- 
1.7.11.5

Re: [PATCH 5/5] s390:block:fix up ENOIOCTLCMD error handling

[Stefan Weinhuber]

heicars2@linux.vnet.ibm.com wrote on 2012-08-27 11:03:55:

[..]
> 
> Thanks, but you missed the else path. I'm going to commit the patch 
below
> unless Stefan has any objections:
> 
> From dac16bd8b314dc6f3f4e6815feab199fdfc8cddd Mon Sep 17 00:00:00 2001
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> Date: Mon, 27 Aug 2012 10:59:42 +0200
> Subject: [PATCH] s390/dasd: fix ioctl return value
> 
> For unimplemented ioctls the dasd driver should return -ENOTTY.
> 
> Reported-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
>  drivers/s390/block/dasd_eckd.c  | 2 +-
>  drivers/s390/block/dasd_ioctl.c | 7 ++-----
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/s390/block/dasd_eckd.c 
b/drivers/s390/block/dasd_eckd.c
> index 40a826a..2fb2b9e 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -3804,7 +3804,7 @@ dasd_eckd_ioctl(struct dasd_block *block, 
> unsigned int cmd, void __user *argp)
>     case BIODASDSYMMIO:
>        return dasd_symm_io(device, argp);
>     default:
> -      return -ENOIOCTLCMD;
> +      return -ENOTTY;
>     }
>  }
> 
> diff --git a/drivers/s390/block/dasd_ioctl.c 
b/drivers/s390/block/dasd_ioctl.c
> index cceae70..654c692 100644
> --- a/drivers/s390/block/dasd_ioctl.c
> +++ b/drivers/s390/block/dasd_ioctl.c
> @@ -498,12 +498,9 @@ int dasd_ioctl(struct block_device *bdev, fmode_t 
mode,
>        break;
>     default:
>        /* if the discipline has an ioctl method try it. */
> -      if (base->discipline->ioctl) {
> +      rc = -ENOTTY;
> +      if (base->discipline->ioctl)
>           rc = base->discipline->ioctl(block, cmd, argp);
> -         if (rc == -ENOIOCTLCMD)
> -            rc = -EINVAL;
> -      } else
> -         rc = -EINVAL;
>     }
>     dasd_put_device(base);
>     return rc;
> -- 
> 1.7.11.5
> 

The patch is looking fine to me, thanks.

Mit freundlichen Grüßen / Kind regards
 
Stefan Weinhuber

Re: [PATCH 1/5] drivers:tty:fix up ENOIOCTLCMD error handling

[Alan Cox]

On Mon, 27 Aug 2012 15:23:12 +0800
Wanlong Gao <gaowanlong@cn.fujitsu.com> wrote:

> At commit 07d106d0, Linus pointed out that ENOIOCTLCMD should be
> translated as ENOTTY to user mode.
> For example:
> 	fd = open("/dev/tty", O_RDWR);
> 	ioctl(fd, -1, &argp);
> 
> then the errno should be ENOTTY but not EINVAL.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>

Acked-by: Alan Cox <alan@linux.intel.com>

Re: [PATCH 2/5] net:atm:fix up ENOIOCTLCMD error handling

[David Miller]

From: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Date: Mon, 27 Aug 2012 15:23:13 +0800

> At commit 07d106d0, Linus pointed out that ENOIOCTLCMD should be
> translated as ENOTTY to user mode.
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>

Applied to net-next, thanks.

Re: [PATCH 4/5] video:omap3isp:fix up ENOIOCTLCMD error handling

[Laurent Pinchart]

Hi Wanlong,

Thanks for the patch.

On Monday 27 August 2012 15:23:15 Wanlong Gao wrote:
> At commit 07d106d0, Linus pointed out that ENOIOCTLCMD should be
> translated as ENOTTY to user mode.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> ---
>  drivers/media/video/omap3isp/ispvideo.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/video/omap3isp/ispvideo.c
> b/drivers/media/video/omap3isp/ispvideo.c index b37379d..2dd982e 100644
> --- a/drivers/media/video/omap3isp/ispvideo.c
> +++ b/drivers/media/video/omap3isp/ispvideo.c
> @@ -337,7 +337,7 @@ __isp_video_get_format(struct isp_video *video, struct
> v4l2_format *format) fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>  	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
>  	if (ret == -ENOIOCTLCMD)
> -		ret = -EINVAL;
> +		ret = -ENOTTY;

I don't think this location should be changed. __isp_video_get_format() is 
called by isp_video_check_format() only, which in turn is called by 
isp_video_streamon() only. A failure to retrieve the format in 
__isp_video_get_format() does not really mean the VIDIOC_STREAMON is not 
supported.

I'll apply hunks 2 to 5 and drop hunk 1 if that's fine with you.

> 
>  	mutex_unlock(&video->mutex);
> 
> @@ -723,7 +723,7 @@ isp_video_try_format(struct file *file, void *fh, struct
> v4l2_format *format) fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>  	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
>  	if (ret)
> -		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
> +		return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
> 
>  	isp_video_mbus_to_pix(video, &fmt.format, &format->fmt.pix);
>  	return 0;
> @@ -744,7 +744,7 @@ isp_video_cropcap(struct file *file, void *fh, struct
> v4l2_cropcap *cropcap) ret = v4l2_subdev_call(subdev, video, cropcap,
> cropcap);
>  	mutex_unlock(&video->mutex);
> 
> -	return ret == -ENOIOCTLCMD ? -EINVAL : ret;
> +	return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
>  }
> 
>  static int
> @@ -771,7 +771,7 @@ isp_video_get_crop(struct file *file, void *fh, struct
> v4l2_crop *crop) format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>  	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &format);
>  	if (ret < 0)
> -		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
> +		return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
> 
>  	crop->c.left = 0;
>  	crop->c.top = 0;
> @@ -796,7 +796,7 @@ isp_video_set_crop(struct file *file, void *fh, struct
> v4l2_crop *crop) ret = v4l2_subdev_call(subdev, video, s_crop, crop);
>  	mutex_unlock(&video->mutex);
> 
> -	return ret == -ENOIOCTLCMD ? -EINVAL : ret;
> +	return ret == -ENOIOCTLCMD ? -ENOTTY : ret;
>  }
> 
>  static int

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/5] video:omap3isp:fix up ENOIOCTLCMD error handling

[Wanlong Gao]

On 09/13/2012 12:03 PM, Laurent Pinchart wrote:
> Hi Wanlong,
> 
> Thanks for the patch.
> 
> On Monday 27 August 2012 15:23:15 Wanlong Gao wrote:
>> At commit 07d106d0, Linus pointed out that ENOIOCTLCMD should be
>> translated as ENOTTY to user mode.
>>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
>> Cc: linux-media@vger.kernel.org
>> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
>> ---
>>  drivers/media/video/omap3isp/ispvideo.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/video/omap3isp/ispvideo.c
>> b/drivers/media/video/omap3isp/ispvideo.c index b37379d..2dd982e 100644
>> --- a/drivers/media/video/omap3isp/ispvideo.c
>> +++ b/drivers/media/video/omap3isp/ispvideo.c
>> @@ -337,7 +337,7 @@ __isp_video_get_format(struct isp_video *video, struct
>> v4l2_format *format) fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>>  	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
>>  	if (ret == -ENOIOCTLCMD)
>> -		ret = -EINVAL;
>> +		ret = -ENOTTY;
> 
> I don't think this location should be changed. __isp_video_get_format() is 
> called by isp_video_check_format() only, which in turn is called by 
> isp_video_streamon() only. A failure to retrieve the format in 
> __isp_video_get_format() does not really mean the VIDIOC_STREAMON is not 
> supported.
> 
> I'll apply hunks 2 to 5 and drop hunk 1 if that's fine with you.

Fine, I defer to your great knowledge in this.

Thanks,
Wanlong Gao