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>

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 Torvalds
1 parent 805a6af8db
Showing 4 changed files with 26 additions and 56 deletions Side-by-side Diff
block/ioctl.c
fs/compat_ioctl.c
fs/ioctl.c
net/socket.c
@@ -180,6 +180,26 @@
 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 @@
 			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 @@
  
 	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;
@@ -1506,35 +1506,6 @@
 	return -ENOIOCTLCMD;
 }
  
-static void compat_ioctl_error(struct file *filp, unsigned int fd,
-		unsigned int cmd, unsigned long arg)
-{
-	char buf[10];
-	char *fn = "?";
-	char *path;
-
-	/* find the name of the device. */
-	path = (char *)__get_free_page(GFP_KERNEL);
-	if (path) {
-		fn = d_path(&filp->f_path, path, PAGE_SIZE);
-		if (IS_ERR(fn))
-			fn = "?";
-	}
-
-	 sprintf(buf,"'%c'", (cmd>>_IOC_TYPESHIFT) & _IOC_TYPEMASK);
-	if (!isprint(buf[1]))
-		sprintf(buf, "%02x", buf[1]);
-	compat_printk("ioctl32(%s:%d): Unknown cmd fd(%d) "
-			"cmd(%08x){t:%s;sz:%u} arg(%08x) on %s\n",
-			current->comm, current->pid,
-			(int)fd, (unsigned int)cmd, buf,
-			(cmd >> _IOC_SIZESHIFT) & _IOC_SIZEMASK,
-			(unsigned int)arg, fn);
-
-	if (path)
-		free_page((unsigned long)path);
-}
-
 static int compat_ioctl_check_table(unsigned int xcmd)
 {
 	int i;
@@ -1621,13 +1592,8 @@
 		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;
  
@@ -42,7 +42,7 @@
  
 	error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
 	if (error == -ENOIOCTLCMD)
-		error = -EINVAL;
+		error = -ENOTTY;
  out:
 	return error;
 }
@@ -2883,7 +2883,7 @@
  
 		return dev_ioctl(net, cmd, uifr);
 	default:
-		return -EINVAL;
+		return -ENOIOCTLCMD;
 	}
 }
  
@@ -3208,20 +3208,6 @@
 	case SIOCDARP:
 	case SIOCATMARK:
 		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;
...	...	@@ -180,6 +180,26 @@
180	180	EXPORT_SYMBOL_GPL(__blkdev_driver_ioctl);
181	181
182	182	/*
	183	+ * Is it an unrecognized ioctl? The correct returns are either
	184	+ * ENOTTY (final) or ENOIOCTLCMD ("I don't know this one, try a
	185	+ * fallback"). ENOIOCTLCMD gets turned into ENOTTY by the ioctl
	186	+ * code before returning.
	187	+ *
	188	+ * Confused drivers sometimes return EINVAL, which is wrong. It
	189	+ * means "I understood the ioctl command, but the parameters to
	190	+ * it were wrong".
	191	+ *
	192	+ * We should aim to just fix the broken drivers, the EINVAL case
	193	+ * should go away.
	194	+ */
	195	+static inline int is_unrecognized_ioctl(int ret)
	196	+{
	197	+ return ret == -EINVAL \|\|
	198	+ ret == -ENOTTY \|\|
	199	+ ret == -ENOIOCTLCMD;
	200	+}
	201	+
	202	+/*
183	203	* always keep this in sync with compat_blkdev_ioctl()
184	204	*/
185	205	int blkdev_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
...	...	@@ -196,8 +216,7 @@
196	216	return -EACCES;
197	217
198	218	ret = __blkdev_driver_ioctl(bdev, mode, cmd, arg);
199		- /* -EINVAL to handle old uncorrected drivers */
200		- if (ret != -EINVAL && ret != -ENOTTY)
	219	+ if (!is_unrecognized_ioctl(ret))
201	220	return ret;
202	221
203	222	fsync_bdev(bdev);
...	...	@@ -206,8 +225,7 @@
206	225
207	226	case BLKROSET:
208	227	ret = __blkdev_driver_ioctl(bdev, mode, cmd, arg);
209		- /* -EINVAL to handle old uncorrected drivers */
210		- if (ret != -EINVAL && ret != -ENOTTY)
	228	+ if (!is_unrecognized_ioctl(ret))
211	229	return ret;
212	230	if (!capable(CAP_SYS_ADMIN))
213	231	return -EACCES;
...	...	@@ -1506,35 +1506,6 @@
1506	1506	return -ENOIOCTLCMD;
1507	1507	}
1508	1508
1509		-static void compat_ioctl_error(struct file *filp, unsigned int fd,
1510		- unsigned int cmd, unsigned long arg)
1511		-{
1512		- char buf[10];
1513		- char *fn = "?";
1514		- char *path;
1515		-
1516		- /* find the name of the device. */
1517		- path = (char *)__get_free_page(GFP_KERNEL);
1518		- if (path) {
1519		- fn = d_path(&filp->f_path, path, PAGE_SIZE);
1520		- if (IS_ERR(fn))
1521		- fn = "?";
1522		- }
1523		-
1524		- sprintf(buf,"'%c'", (cmd>>_IOC_TYPESHIFT) & _IOC_TYPEMASK);
1525		- if (!isprint(buf[1]))
1526		- sprintf(buf, "%02x", buf[1]);
1527		- compat_printk("ioctl32(%s:%d): Unknown cmd fd(%d) "
1528		- "cmd(%08x){t:%s;sz:%u} arg(%08x) on %s\n",
1529		- current->comm, current->pid,
1530		- (int)fd, (unsigned int)cmd, buf,
1531		- (cmd >> _IOC_SIZESHIFT) & _IOC_SIZEMASK,
1532		- (unsigned int)arg, fn);
1533		-
1534		- if (path)
1535		- free_page((unsigned long)path);
1536		-}
1537		-
1538	1509	static int compat_ioctl_check_table(unsigned int xcmd)
1539	1510	{
1540	1511	int i;
...	...	@@ -1621,13 +1592,8 @@
1621	1592	goto found_handler;
1622	1593
1623	1594	error = do_ioctl_trans(fd, cmd, arg, filp);
1624		- if (error == -ENOIOCTLCMD) {
1625		- static int count;
1626		-
1627		- if (++count <= 50)
1628		- compat_ioctl_error(filp, fd, cmd, arg);
1629		- error = -EINVAL;
1630		- }
	1595	+ if (error == -ENOIOCTLCMD)
	1596	+ error = -ENOTTY;
1631	1597
1632	1598	goto out_fput;
1633	1599
...	...	@@ -42,7 +42,7 @@
42	42
43	43	error = filp->f_op->unlocked_ioctl(filp, cmd, arg);
44	44	if (error == -ENOIOCTLCMD)
45		- error = -EINVAL;
	45	+ error = -ENOTTY;
46	46	out:
47	47	return error;
48	48	}
...	...	@@ -2883,7 +2883,7 @@
2883	2883
2884	2884	return dev_ioctl(net, cmd, uifr);
2885	2885	default:
2886		- return -EINVAL;
	2886	+ return -ENOIOCTLCMD;
2887	2887	}
2888	2888	}
2889	2889
...	...	@@ -3208,20 +3208,6 @@
3208	3208	case SIOCDARP:
3209	3209	case SIOCATMARK:
3210	3210	return sock_do_ioctl(net, sock, cmd, arg);
3211		- }
3212		-
3213		- /* Prevent warning from compat_sys_ioctl, these always
3214		- * result in -EINVAL in the native case anyway. */
3215		- switch (cmd) {
3216		- case SIOCRTMSG:
3217		- case SIOCGIFCOUNT:
3218		- case SIOCSRARP:
3219		- case SIOCGRARP:
3220		- case SIOCDRARP:
3221		- case SIOCSIFLINK:
3222		- case SIOCGIFSLAVE:
3223		- case SIOCSIFSLAVE:
3224		- return -EINVAL;
3225	3211	}
3226	3212
3227	3213	return -ENOIOCTLCMD;