[PATCH 0/5] TTY: fix tty_wait_until_sent on 64-bit machines

[PATCH 0/5] TTY: fix tty_wait_until_sent on 64-bit machines

[Johan Hovold]

This series fixes an overflow bug in tty_wait_until_sent, which is
currently broken on 64-bit machines for most TTY drivers (with
serial-core being the notable exception).

The final patch makes the tty_wait_until_sent timeout a maximum one so
that we actually honour the closing-wait timeout.

All but the second (clean-up) and final patch are marked for stable.

These patches are for v4.0 and I suggest Greg takes them all through the
TTY tree.

Thanks,
Johan


Johan Hovold (5):
  net: irda: fix wait_until_sent poll timeout
  TTY: bfin_jtag_comm: remove incorrect wait_until_sent operation
  USB: serial: fix infinite wait_until_sent timeout
  TTY: fix tty_wait_until_sent on 64-bit machines
  TTY: fix tty_wait_until_sent maximum timeout

 drivers/tty/bfin_jtag_comm.c | 13 -------------
 drivers/tty/tty_ioctl.c      | 16 +++++++++++-----
 drivers/usb/serial/generic.c |  5 +++--
 net/irda/ircomm/ircomm_tty.c |  4 +++-
 4 files changed, 17 insertions(+), 21 deletions(-)

-- 
2.0.5

[PATCH 1/5] net: irda: fix wait_until_sent poll timeout

[Johan Hovold]

In case an infinite timeout (0) is requested, the irda wait_until_sent
implementation would use a zero poll timeout rather than the default
200ms.

Note that wait_until_sent is currently never called with a 0-timeout
argument due to a bug in tty_wait_until_sent.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable <stable@vger.kernel.org>	# v2.6.12
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 net/irda/ircomm/ircomm_tty.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 40695b9751c1..4efe486baee6 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -798,7 +798,9 @@ static void ircomm_tty_wait_until_sent(struct tty_struct *tty, int timeout)
 	orig_jiffies = jiffies;
 
 	/* Set poll time to 200 ms */
-	poll_time = IRDA_MIN(timeout, msecs_to_jiffies(200));
+	poll_time = msecs_to_jiffies(200);
+	if (timeout)
+		poll_time = min_t(unsigned long, timeout, poll_time);
 
 	spin_lock_irqsave(&self->spinlock, flags);
 	while (self->tx_skb && self->tx_skb->len) {
-- 
2.0.5

[PATCH 2/5] TTY: bfin_jtag_comm: remove incorrect wait_until_sent operation

[Johan Hovold]

Remove incorrect and redundant wait_until_sent operation, which waits
for the driver buffer rather than any hardware buffers to drain,
something which is already taken care of by the tty layer (and
chars_in_buffer).

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/bfin_jtag_comm.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/tty/bfin_jtag_comm.c b/drivers/tty/bfin_jtag_comm.c
index d7b198c400c7..ce24182f8514 100644
--- a/drivers/tty/bfin_jtag_comm.c
+++ b/drivers/tty/bfin_jtag_comm.c
@@ -210,18 +210,6 @@ bfin_jc_chars_in_buffer(struct tty_struct *tty)
 	return circ_cnt(&bfin_jc_write_buf);
 }
 
-static void
-bfin_jc_wait_until_sent(struct tty_struct *tty, int timeout)
-{
-	unsigned long expire = jiffies + timeout;
-	while (!circ_empty(&bfin_jc_write_buf)) {
-		if (signal_pending(current))
-			break;
-		if (time_after(jiffies, expire))
-			break;
-	}
-}
-
 static const struct tty_operations bfin_jc_ops = {
 	.open            = bfin_jc_open,
 	.close           = bfin_jc_close,
@@ -230,7 +218,6 @@ static const struct tty_operations bfin_jc_ops = {
 	.flush_chars     = bfin_jc_flush_chars,
 	.write_room      = bfin_jc_write_room,
 	.chars_in_buffer = bfin_jc_chars_in_buffer,
-	.wait_until_sent = bfin_jc_wait_until_sent,
 };
 
 static int __init bfin_jc_init(void)
-- 
2.0.5

Re: [PATCH 2/5] TTY: bfin_jtag_comm: remove incorrect wait_until_sent operation

[Peter Hurley]

On 03/04/2015 04:39 AM, Johan Hovold wrote:
> Remove incorrect and redundant wait_until_sent operation, which waits
> for the driver buffer rather than any hardware buffers to drain,
> something which is already taken care of by the tty layer (and
> chars_in_buffer).

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

[PATCH 3/5] USB: serial: fix infinite wait_until_sent timeout

[Johan Hovold]

Make sure to handle an infinite timeout (0).

Note that wait_until_sent is currently never called with a 0-timeout
argument due to a bug in tty_wait_until_sent.

Fixes: dcf010503966 ("USB: serial: add generic wait_until_sent
implementation")
Cc: stable <stable@vger.kernel.org>	# v3.10

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/generic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index ccf1df7c4b80..54e170dd3dad 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -258,7 +258,8 @@ void usb_serial_generic_wait_until_sent(struct tty_struct *tty, long timeout)
 	 * character or at least one jiffy.
 	 */
 	period = max_t(unsigned long, (10 * HZ / bps), 1);
-	period = min_t(unsigned long, period, timeout);
+	if (timeout)
+		period = min_t(unsigned long, period, timeout);
 
 	dev_dbg(&port->dev, "%s - timeout = %u ms, period = %u ms\n",
 					__func__, jiffies_to_msecs(timeout),
@@ -268,7 +269,7 @@ void usb_serial_generic_wait_until_sent(struct tty_struct *tty, long timeout)
 		schedule_timeout_interruptible(period);
 		if (signal_pending(current))
 			break;
-		if (time_after(jiffies, expire))
+		if (timeout && time_after(jiffies, expire))
 			break;
 	}
 }
-- 
2.0.5

[PATCH 4/5] TTY: fix tty_wait_until_sent on 64-bit machines

[Johan Hovold]

Fix overflow bug in tty_wait_until_sent on 64-bit machines, where an
infinite timeout (0) would be passed to the underlying tty-driver's
wait_until_sent-operation as a negative timeout (-1), causing it to
return immediately.

This manifests itself for example as tcdrain() returning immediately,
drivers not honouring the drain flags when setting terminal attributes,
or even dropped data on close as a requested infinite closing-wait
timeout would be ignored.

The first symptom  was reported by Asier LLANO who noted that tcdrain()
returned prematurely when using the ftdi_sio usb-serial driver.

Fix this by passing 0 rather than MAX_SCHEDULE_TIMEOUT (LONG_MAX) to the
underlying tty driver.

Note that the serial-core wait_until_sent-implementation is not affected
by this bug due to a lucky chance (comparison to an unsigned maximum
timeout), and neither is the cyclades one that had an explicit check for
negative timeouts, but all other tty drivers appear to be affected.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable <stable@vger.kernel.org>	# v2.6.12
Reported-by: ZIV-Asier Llano Palacios <asier.llano@cgglobal.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/tty_ioctl.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index a5cf253b2544..89ae23ac9ae6 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -217,11 +217,17 @@ void tty_wait_until_sent(struct tty_struct *tty, long timeout)
 #endif
 	if (!timeout)
 		timeout = MAX_SCHEDULE_TIMEOUT;
+
 	if (wait_event_interruptible_timeout(tty->write_wait,
-			!tty_chars_in_buffer(tty), timeout) >= 0) {
-		if (tty->ops->wait_until_sent)
-			tty->ops->wait_until_sent(tty, timeout);
+			!tty_chars_in_buffer(tty), timeout) < 0) {
+		return;
 	}
+
+	if (timeout == MAX_SCHEDULE_TIMEOUT)
+		timeout = 0;
+
+	if (tty->ops->wait_until_sent)
+		tty->ops->wait_until_sent(tty, timeout);
 }
 EXPORT_SYMBOL(tty_wait_until_sent);
 
-- 
2.0.5

Re: [PATCH 4/5] TTY: fix tty_wait_until_sent on 64-bit machines

[Peter Hurley]

On 03/04/2015 04:39 AM, Johan Hovold wrote:
> Fix overflow bug in tty_wait_until_sent on 64-bit machines, where an
> infinite timeout (0) would be passed to the underlying tty-driver's
> wait_until_sent-operation as a negative timeout (-1), causing it to
> return immediately.

Wow, that is a nasty bug.

> This manifests itself for example as tcdrain() returning immediately,
> drivers not honouring the drain flags when setting terminal attributes,
> or even dropped data on close as a requested infinite closing-wait
> timeout would be ignored.
> 
> The first symptom  was reported by Asier LLANO who noted that tcdrain()
> returned prematurely when using the ftdi_sio usb-serial driver.
> 
> Fix this by passing 0 rather than MAX_SCHEDULE_TIMEOUT (LONG_MAX) to the
> underlying tty driver.
> 
> Note that the serial-core wait_until_sent-implementation is not affected
> by this bug due to a lucky chance (comparison to an unsigned maximum
> timeout), and neither is the cyclades one that had an explicit check for
> negative timeouts, but all other tty drivers appear to be affected.

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

[PATCH 5/5] TTY: fix tty_wait_until_sent maximum timeout

[Johan Hovold]

Currently tty_wait_until_sent may take up to twice as long as the
requested timeout while waiting for driver and hardware buffers to
drain.

Fix this by taking the remaining number of jiffies after waiting for
driver buffers to drain into account so that the timeout actually
becomes a maximum timeout as it is documented to be.

Note that this specifically implies tighter timings when closing a port
as a consequence of actually honouring the port closing-wait setting
for drivers relying on tty_wait_until_sent_from_close (e.g. via
tty_port_close_start).

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/tty_ioctl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index 89ae23ac9ae6..632fc8152061 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -218,10 +218,10 @@ void tty_wait_until_sent(struct tty_struct *tty, long timeout)
 	if (!timeout)
 		timeout = MAX_SCHEDULE_TIMEOUT;
 
-	if (wait_event_interruptible_timeout(tty->write_wait,
-			!tty_chars_in_buffer(tty), timeout) < 0) {
+	timeout = wait_event_interruptible_timeout(tty->write_wait,
+			!tty_chars_in_buffer(tty), timeout);
+	if (timeout <= 0)
 		return;
-	}
 
 	if (timeout == MAX_SCHEDULE_TIMEOUT)
 		timeout = 0;
-- 
2.0.5

Re: [PATCH 5/5] TTY: fix tty_wait_until_sent maximum timeout

[Peter Hurley]

On 03/04/2015 04:39 AM, Johan Hovold wrote:
> Currently tty_wait_until_sent may take up to twice as long as the
> requested timeout while waiting for driver and hardware buffers to
> drain.
> 
> Fix this by taking the remaining number of jiffies after waiting for
> driver buffers to drain into account so that the timeout actually
> becomes a maximum timeout as it is documented to be.
> 
> Note that this specifically implies tighter timings when closing a port
> as a consequence of actually honouring the port closing-wait setting
> for drivers relying on tty_wait_until_sent_from_close (e.g. via
> tty_port_close_start).

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

Re: [PATCH 0/5] TTY: fix tty_wait_until_sent on 64-bit machines

[Greg Kroah-Hartman]

On Wed, Mar 04, 2015 at 10:39:02AM +0100, Johan Hovold wrote:
> This series fixes an overflow bug in tty_wait_until_sent, which is
> currently broken on 64-bit machines for most TTY drivers (with
> serial-core being the notable exception).
> 
> The final patch makes the tty_wait_until_sent timeout a maximum one so
> that we actually honour the closing-wait timeout.
> 
> All but the second (clean-up) and final patch are marked for stable.
> 
> These patches are for v4.0 and I suggest Greg takes them all through the
> TTY tree.

Nice fixes, all now queued up.