tty: ioctl fixes and cleanups

PATCH New Johan Hovold 1 participant 9 messages Apr 7, 2021 TTY layer and serial drivers

lore

Share & tools

lore

Johan Hovold

author

2021-04-07 09:52 UTC #0 lore raw

cover tty: ioctl fixes and cleanups

This series fixes and cleans up a few (serial-) ioctl-related issues.

Johan

Johan Hovold (8):
tty: actually undefine superseded ASYNC flags
tty: fix return value for unsupported ioctls
tty: fix return value for unsupported termiox ioctls
tty: use pr_warn_ratelimited() for deprecated serial flags
tty: refactor TIOCSSERIAL handling
tty: add ASYNC_SPLIT_TERMIOS to deprecation mask
serial: core: drop redundant TIOCGSERIAL memset
serial: core: return early on unsupported ioctls

 drivers/tty/serial/serial_core.c |  8 ++---
 drivers/tty/tty_io.c             | 53 ++++++++++++++------------------
 drivers/tty/tty_ioctl.c          |  4 +--
 include/linux/tty_driver.h       |  2 +-
 include/uapi/linux/tty_flags.h   |  8 ++---
 5 files changed, 33 insertions(+), 42 deletions(-)

Signature · 1 line

2.26.3

Johan Hovold

author

2021-04-07 09:52 UTC #1 lore raw

PATCH 1/8 tty: actually undefine superseded ASYNC flags

Some kernel-internal ASYNC flags have been superseded by tty-port flags and should no longer be used by kernel drivers.

Fix the misspelled "__KERNEL__" compile guards which failed their sole purpose to break out-of-tree drivers that have not yet been updated.

Fixes 5c0517fefc92 ("tty: core: Undefine ASYNC_* flags superceded by TTY_PORT* flags")

Signed-off-by Johan Hovold <[email protected]>

---
 include/uapi/linux/tty_flags.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

include/uapi/linux/tty_flags.h +2 -2

index 900a32e63424..6a3ac496a56c 100644
@@ -39,7 +39,7 @@
  * WARNING: These flags are no longer used and have been superceded by the
  *	    TTY_PORT_ flags in the iflags field (and not userspace-visible)
  */
-#ifndef _KERNEL_
+#ifndef __KERNEL__
 #define ASYNCB_INITIALIZED	31 /* Serial port was initialized */
 #define ASYNCB_SUSPENDED	30 /* Serial port is suspended */
 #define ASYNCB_NORMAL_ACTIVE	29 /* Normal device is active */

@@ -81,7 +81,7 @@
 #define ASYNC_SPD_WARP		(ASYNC_SPD_HI|ASYNC_SPD_SHI)
 #define ASYNC_SPD_MASK		(ASYNC_SPD_HI|ASYNC_SPD_VHI|ASYNC_SPD_SHI)
 
-#ifndef _KERNEL_
+#ifndef __KERNEL__
 /* These flags are no longer used (and were always masked from userspace) */
 #define ASYNC_INITIALIZED	(1U << ASYNCB_INITIALIZED)
 #define ASYNC_NORMAL_ACTIVE	(1U << ASYNCB_NORMAL_ACTIVE)

Signature · 1 line

2.26.3

Johan Hovold

author

2021-04-07 09:52 UTC #2 lore raw

PATCH 2/8 tty: fix return value for unsupported ioctls

In reply to Johan Hovold

Drivers should return -ENOTTY ("Inappropriate I/O control operation") when an ioctl isn't supported, while -EINVAL is used for invalid arguments.

Fix up the TIOCMGET, TIOCMSET and TIOCGICOUNT helpers which returned -EINVAL when a tty driver did not implement the corresponding operations.

Note that the TIOCMGET and TIOCMSET helpers predate git and do not get a corresponding Fixes tag below.

Fixes d281da7ff6f7 ("tty: Make tiocgicount a handler")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/tty_io.c       | 10 +++++-----
 include/linux/tty_driver.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

drivers/tty/tty_io.c +5 -5

index ebcde1f96adb..7738514a15fe 100644
@@ -2529,14 +2529,14 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
  *	@p: pointer to result
  *
  *	Obtain the modem status bits from the tty driver if the feature
- *	is supported. Return -EINVAL if it is not available.
+ *	is supported. Return -ENOTTY if it is not available.
  *
  *	Locking: none (up to the driver)
  */
 
 static int tty_tiocmget(struct tty_struct *tty, int __user *p)
 {
-	int retval = -EINVAL;
+	int retval = -ENOTTY;
 
 	if (tty->ops->tiocmget) {
 		retval = tty->ops->tiocmget(tty);

@@ -2554,7 +2554,7 @@ static int tty_tiocmget(struct tty_struct *tty, int __user *p)
  *	@p: pointer to desired bits
  *
  *	Set the modem status bits from the tty driver if the feature
- *	is supported. Return -EINVAL if it is not available.
+ *	is supported. Return -ENOTTY if it is not available.
  *
  *	Locking: none (up to the driver)
  */

@@ -2566,7 +2566,7 @@ static int tty_tiocmset(struct tty_struct *tty, unsigned int cmd,
 	unsigned int set, clear, val;
 
 	if (tty->ops->tiocmset == NULL)
-		return -EINVAL;
+		return -ENOTTY;
 
 	retval = get_user(val, p);
 	if (retval)

@@ -2606,7 +2606,7 @@ int tty_get_icount(struct tty_struct *tty,
 	if (tty->ops->get_icount)
 		return tty->ops->get_icount(tty, icount);
 	else
-		return -EINVAL;
+		return -ENOTTY;
 }
 EXPORT_SYMBOL_GPL(tty_get_icount);

include/linux/tty_driver.h +1 -1

index 61c3372d3f32..2f719b471d52 100644
@@ -228,7 +228,7 @@
  *
  *	Called when the device receives a TIOCGICOUNT ioctl. Passed a kernel
  *	structure to complete. This method is optional and will only be called
- *	if provided (otherwise EINVAL will be returned).
+ *	if provided (otherwise ENOTTY will be returned).
  */
 
 #include <linux/export.h>

Signature · 1 line

2.26.3

Johan Hovold

author

2021-04-07 09:52 UTC #3 lore raw

PATCH 3/8 tty: fix return value for unsupported termiox ioctls

In reply to Johan Hovold

Drivers should return -ENOTTY ("Inappropriate I/O control operation") when an ioctl isn't supported, while -EINVAL is used for invalid arguments.

Support for termiox was added by commit 1d65b4a088de ("tty: Add termiox") in 2008 but no driver support ever followed and it was recently ripped out by commit e0efb3168d34 ("tty: Remove dead termiox code").

Fix the return value for the unsupported termiox ioctls, which have always returned -EINVAL, by explicitly returning -ENOTTY rather than removing them completely and falling back to the default unrecognised- ioctl handling.

Fixes 1d65b4a088de ("tty: Add termiox")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/tty_ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/tty/tty_ioctl.c +2 -2

index deffaefcf41d..420e7678330f 100644
@@ -773,8 +773,8 @@ int tty_mode_ioctl(struct tty_struct *tty, struct file *file,
 	case TCSETX:
 	case TCSETXW:
 	case TCSETXF:
-		return -EINVAL;
-#endif		
+		return -ENOTTY;
+#endif
 	case TIOCGSOFTCAR:
 		copy_termios(real_tty, &kterm);
 		ret = put_user((kterm.c_cflag & CLOCAL) ? 1 : 0,

Signature · 1 line

2.26.3

Johan Hovold

author

2021-04-07 09:52 UTC #4 lore raw

PATCH 4/8 tty: use pr_warn_ratelimited() for deprecated serial flags

In reply to Johan Hovold

Use pr_warn_ratelimited() when warning about deprecated serial flags instead of open coding.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/tty_io.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

drivers/tty/tty_io.c +6 ~~-12~~

index 7738514a15fe..e00efd299abd 100644
@@ -2626,9 +2626,6 @@ static int tty_tiocgicount(struct tty_struct *tty, void __user *arg)
 
 static int tty_tiocsserial(struct tty_struct *tty, struct serial_struct __user *ss)
 {
-	static DEFINE_RATELIMIT_STATE(depr_flags,
-			DEFAULT_RATELIMIT_INTERVAL,
-			DEFAULT_RATELIMIT_BURST);
 	char comm[TASK_COMM_LEN];
 	struct serial_struct v;
 	int flags;

@@ -2638,9 +2635,9 @@ static int tty_tiocsserial(struct tty_struct *tty, struct serial_struct __user *
 
 	flags = v.flags & ASYNC_DEPRECATED;
 
-	if (flags && __ratelimit(&depr_flags))
-		pr_warn("%s: '%s' is using deprecated serial flags (with no effect): %.8x\n",
-			__func__, get_task_comm(comm, current), flags);
+	if (flags)
+		pr_warn_ratelimited("%s: '%s' is using deprecated serial flags (with no effect): %.8x\n",
+				__func__, get_task_comm(comm, current), flags);
 	if (!tty->ops->set_serial)
 		return -ENOTTY;
 	return tty->ops->set_serial(tty, &v);

@@ -2841,9 +2838,6 @@ struct serial_struct32 {
 static int compat_tty_tiocsserial(struct tty_struct *tty,
 		struct serial_struct32 __user *ss)
 {
-	static DEFINE_RATELIMIT_STATE(depr_flags,
-			DEFAULT_RATELIMIT_INTERVAL,
-			DEFAULT_RATELIMIT_BURST);
 	char comm[TASK_COMM_LEN];
 	struct serial_struct32 v32;
 	struct serial_struct v;

@@ -2860,9 +2854,9 @@ static int compat_tty_tiocsserial(struct tty_struct *tty,
 
 	flags = v.flags & ASYNC_DEPRECATED;
 
-	if (flags && __ratelimit(&depr_flags))
-		pr_warn("%s: '%s' is using deprecated serial flags (with no effect): %.8x\n",
-			__func__, get_task_comm(comm, current), flags);
+	if (flags)
+		pr_warn_ratelimited("%s: '%s' is using deprecated serial flags (with no effect): %.8x\n",
+				__func__, get_task_comm(comm, current), flags);
 	if (!tty->ops->set_serial)
 		return -ENOTTY;
 	return tty->ops->set_serial(tty, &v);

Signature · 1 line

2.26.3

Johan Hovold

author

2021-04-07 09:52 UTC #5 lore raw

PATCH 5/8 tty: refactor TIOCSSERIAL handling

In reply to Johan Hovold

Factor out the deprecated serial flags handling and tty-operation check shared with the compat TIOCSSERIAL handler.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/tty_io.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

drivers/tty/tty_io.c +16 ~~-17~~

index e00efd299abd..36e6b28fb80d 100644
@@ -2624,23 +2624,31 @@ static int tty_tiocgicount(struct tty_struct *tty, void __user *arg)
 	return 0;
 }
 
-static int tty_tiocsserial(struct tty_struct *tty, struct serial_struct __user *ss)
+static int tty_set_serial(struct tty_struct *tty, struct serial_struct *ss)
 {
 	char comm[TASK_COMM_LEN];
-	struct serial_struct v;
 	int flags;
 
-	if (copy_from_user(&v, ss, sizeof(*ss)))
-		return -EFAULT;
-
-	flags = v.flags & ASYNC_DEPRECATED;
+	flags = ss->flags & ASYNC_DEPRECATED;
 
 	if (flags)
 		pr_warn_ratelimited("%s: '%s' is using deprecated serial flags (with no effect): %.8x\n",
 				__func__, get_task_comm(comm, current), flags);
+
 	if (!tty->ops->set_serial)
 		return -ENOTTY;
-	return tty->ops->set_serial(tty, &v);
+
+	return tty->ops->set_serial(tty, ss);
+}
+
+static int tty_tiocsserial(struct tty_struct *tty, struct serial_struct __user *ss)
+{
+	struct serial_struct v;
+
+	if (copy_from_user(&v, ss, sizeof(*ss)))
+		return -EFAULT;
+
+	return tty_set_serial(tty, &v);
 }
 
 static int tty_tiocgserial(struct tty_struct *tty, struct serial_struct __user *ss)

@@ -2838,10 +2846,8 @@ struct serial_struct32 {
 static int compat_tty_tiocsserial(struct tty_struct *tty,
 		struct serial_struct32 __user *ss)
 {
-	char comm[TASK_COMM_LEN];
 	struct serial_struct32 v32;
 	struct serial_struct v;
-	int flags;
 
 	if (copy_from_user(&v32, ss, sizeof(*ss)))
 		return -EFAULT;

@@ -2852,14 +2858,7 @@ static int compat_tty_tiocsserial(struct tty_struct *tty,
 	v.port_high = v32.port_high;
 	v.iomap_base = 0;
 
-	flags = v.flags & ASYNC_DEPRECATED;
-
-	if (flags)
-		pr_warn_ratelimited("%s: '%s' is using deprecated serial flags (with no effect): %.8x\n",
-				__func__, get_task_comm(comm, current), flags);
-	if (!tty->ops->set_serial)
-		return -ENOTTY;
-	return tty->ops->set_serial(tty, &v);
+	return tty_set_serial(tty, &v);
 }
 
 static int compat_tty_tiocgserial(struct tty_struct *tty,

Signature · 1 line

2.26.3

Johan Hovold

author

2021-04-07 09:52 UTC #6 lore raw

PATCH 6/8 tty: add ASYNC_SPLIT_TERMIOS to deprecation mask

In reply to Johan Hovold

Callout devices are long-gone, but the ASYNC_SPLIT_TERMIOS flag was never added to the deprecation mask.

Add it so that a warning is printed if it is ever used.

Fixes 8a8ae62f8296 ("tty: warn on deprecated serial flags")

Signed-off-by Johan Hovold <[email protected]>

---
 include/uapi/linux/tty_flags.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

include/uapi/linux/tty_flags.h +2 -2

index 6a3ac496a56c..cf25056d4b27 100644
@@ -73,8 +73,8 @@
 #define ASYNC_MAGIC_MULTIPLIER	(1U << ASYNCB_MAGIC_MULTIPLIER)
 
 #define ASYNC_FLAGS		((1U << (ASYNCB_LAST_USER + 1)) - 1)
-#define ASYNC_DEPRECATED	(ASYNC_SESSION_LOCKOUT | ASYNC_PGRP_LOCKOUT | \
-		ASYNC_CALLOUT_NOHUP | ASYNC_AUTOPROBE)
+#define ASYNC_DEPRECATED	(ASYNC_SPLIT_TERMIOS | ASYNC_SESSION_LOCKOUT | \
+		ASYNC_PGRP_LOCKOUT | ASYNC_CALLOUT_NOHUP | ASYNC_AUTOPROBE)
 #define ASYNC_USR_MASK		(ASYNC_SPD_MASK|ASYNC_CALLOUT_NOHUP| \
 		ASYNC_LOW_LATENCY)
 #define ASYNC_SPD_CUST		(ASYNC_SPD_HI|ASYNC_SPD_VHI)

Signature · 1 line

2.26.3

Johan Hovold

author

2021-04-07 09:52 UTC #7 lore raw

PATCH 7/8 serial: core: drop redundant TIOCGSERIAL memset

In reply to Johan Hovold

Since commit 5099d234a52a ("serial_core: switch to ->[sg]et_serial()") the serial structure passed to uart_get_info() has already have been cleared by the tty layer so drop the redundant memset.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serial/serial_core.c | 2 --
 1 file changed, 2 deletions(-)

drivers/tty/serial/serial_core.c +0 -2

index ba31e97d3d96..59336943a86f 100644
@@ -759,8 +759,6 @@ static int uart_get_info(struct tty_port *port, struct serial_struct *retinfo)
 	struct uart_port *uport;
 	int ret = -ENODEV;
 
-	memset(retinfo, 0, sizeof(*retinfo));
-
 	/*
 	 * Ensure the state we copy is consistent and no hardware changes
 	 * occur as we go

Signature · 1 line

2.26.3

Johan Hovold

author

2021-04-07 09:52 UTC #8 lore raw

PATCH 8/8 serial: core: return early on unsupported ioctls

In reply to Johan Hovold

Drivers can return -ENOIOCTLCMD when an ioctl is not recognised to tell the upper layers to continue looking for a handler.

This is not the case for the RS485 and ISO7816 ioctls whose handlers should return -ENOTTY directly in case a serial driver does not implement the corresponding methods.

Fixes a5f276f10ff7 ("serial_core: Handle TIOC[GS]RS485 ioctls.")

Fixes ad8c0eaa0a41 ("tty/serial_core: add ISO7816 infrastructure")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/serial/serial_core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/tty/serial/serial_core.c +3 -3

index 59336943a86f..87f7127b57e6 100644
@@ -1303,7 +1303,7 @@ static int uart_set_rs485_config(struct uart_port *port,
 	unsigned long flags;
 
 	if (!port->rs485_config)
-		return -ENOIOCTLCMD;
+		return -ENOTTY;
 
 	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
 		return -EFAULT;

@@ -1327,7 +1327,7 @@ static int uart_get_iso7816_config(struct uart_port *port,
 	struct serial_iso7816 aux;
 
 	if (!port->iso7816_config)
-		return -ENOIOCTLCMD;
+		return -ENOTTY;
 
 	spin_lock_irqsave(&port->lock, flags);
 	aux = port->iso7816;

@@ -1347,7 +1347,7 @@ static int uart_set_iso7816_config(struct uart_port *port,
 	unsigned long flags;
 
 	if (!port->iso7816_config)
-		return -ENOIOCTLCMD;
+		return -ENOTTY;
 
 	if (copy_from_user(&iso7816, iso7816_user, sizeof(*iso7816_user)))
 		return -EFAULT;

Signature · 1 line

2.26.3