LinuxLists.cc - [PATCH 0/7] n

2013-03-06 13:38:54

Subject: [PATCH 0/7] n_tty fixes

This patch series combines a number of fixes for the N_TTY
line discipline.

These address unsafe use of the foreground process group id:
n_tty: Fix unsafe driver-side signals
n_tty: Lock access to tty->pgrp for POSIX job control

These continue Jiri's work to separate ldisc from tty:
tty: Fix checkpatch errors in tty_ldisc.h
n_tty: Encapsulate minimum_to_wake within N_TTY

The rest are self-explanatory:
n_tty: Untangle read completion variables
n_tty: Fix unsafe update of available buffer space
n_tty: Buffer work should not reschedule itself

Peter Hurley (7):
n_tty: Fix unsafe driver-side signals
n_tty: Lock access to tty->pgrp for POSIX job control
tty: Fix checkpatch errors in tty_ldisc.h
n_tty: Encapsulate minimum_to_wake within N_TTY
n_tty: Untangle read completion variables
n_tty: Fix unsafe update of available buffer space
n_tty: Buffer work should not reschedule itself

drivers/tty/n_tty.c | 144 +++++++++++++++++++++++++---------------------
drivers/tty/tty_io.c | 17 +++---
include/linux/tty.h | 1 -
include/linux/tty_ldisc.h | 138 +++++++++++++++++++++++---------------------
4 files changed, 161 insertions(+), 139 deletions(-)

--
1.8.1.2

2013-03-06 13:38:59

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 2/7] n_tty: Lock access to tty->pgrp for POSIX job control

Concurrent access to tty->pgrp must be protected with tty->ctrl_lock.
Also, as noted in the comments, reading current->signal->tty is
safe because either,
1) current->signal->tty is assigned by current, or
2) current->signal->tty is set to NULL.

NB: for reference, tty_check_change() implements a similar POSIX
check for the ioctls corresponding to tcflush(), tcdrain(),
tcsetattr(), tcsetpgrp(), tcflow() and tcsendbreak().

Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/n_tty.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index cd9ba3d..61797c4 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1744,10 +1744,9 @@ extern ssize_t redirected_tty_write(struct file *, const char __user *,
* and if appropriate send any needed signals and return a negative
* error code if action should be taken.
*
- * FIXME:
- * Locking: None - redirected write test is safe, testing
- * current->signal should possibly lock current->sighand
- * pgrp locking ?
+ * Locking: redirected write test is safe
+ * current->signal->tty check is safe
+ * ctrl_lock to safely reference tty->pgrp
*/

static int job_control(struct tty_struct *tty, struct file *file)
@@ -1757,19 +1756,22 @@ static int job_control(struct tty_struct *tty, struct file *file)
/* NOTE: not yet done after every sleep pending a thorough
check of the logic of this change. -- jlc */
/* don't stop on /dev/console */
- if (file->f_op->write != redirected_tty_write &&
- current->signal->tty == tty) {
- if (!tty->pgrp)
- printk(KERN_ERR "n_tty_read: no tty->pgrp!\n");
- else if (task_pgrp(current) != tty->pgrp) {
- if (is_ignored(SIGTTIN) ||
- is_current_pgrp_orphaned())
- return -EIO;
- kill_pgrp(task_pgrp(current), SIGTTIN, 1);
- set_thread_flag(TIF_SIGPENDING);
- return -ERESTARTSYS;
- }
+ if (file->f_op->write == redirected_tty_write ||
+ current->signal->tty != tty)
+ return 0;
+
+ spin_lock_irq(&tty->ctrl_lock);
+ if (!tty->pgrp)
+ printk(KERN_ERR "n_tty_read: no tty->pgrp!\n");
+ else if (task_pgrp(current) != tty->pgrp) {
+ spin_unlock_irq(&tty->ctrl_lock);
+ if (is_ignored(SIGTTIN) || is_current_pgrp_orphaned())
+ return -EIO;
+ kill_pgrp(task_pgrp(current), SIGTTIN, 1);
+ set_thread_flag(TIF_SIGPENDING);
+ return -ERESTARTSYS;
}
+ spin_unlock_irq(&tty->ctrl_lock);
return 0;
}

--
1.8.1.2

2013-03-06 13:39:16

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 7/7] n_tty: Buffer work should not reschedule itself

Although the driver-side input path must update the available
buffer space, it should not reschedule itself. If space is still
available and the flip buffers are not empty, flush_to_ldisc()
will loop again.

Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/n_tty.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 61a55f4..6f9694d 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -118,14 +118,14 @@ static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
* n_tty_set_room - receive space
* @tty: terminal
*
- * Sets tty->receive_room to reflect the currently available space
+ * Updates tty->receive_room to reflect the currently available space
* in the input buffer, and re-schedules the flip buffer work if space
* just became available.
*
* Locks: Concurrent update is protected with read_lock
*/

-static void n_tty_set_room(struct tty_struct *tty)
+static int set_room(struct tty_struct *tty)
{
struct n_tty_data *ldata = tty->disc_data;
int left;
@@ -155,8 +155,13 @@ static void n_tty_set_room(struct tty_struct *tty)

raw_spin_unlock_irqrestore(&ldata->read_lock, flags);

+ return left && !old_left;
+}
+
+static void n_tty_set_room(struct tty_struct *tty)
+{
/* Did this open up the receive buffer? We may need to flip */
- if (left && !old_left) {
+ if (set_room(tty)) {
WARN_RATELIMIT(tty->port->itty == NULL,
"scheduling with invalid itty\n");
/* see if ldisc has been killed - if so, this means that
@@ -1475,7 +1480,7 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
tty->ops->flush_chars(tty);
}

- n_tty_set_room(tty);
+ set_room(tty);

if ((!ldata->icanon && (ldata->read_cnt >= ldata->minimum_to_wake)) ||
L_EXTPROC(tty)) {
--
1.8.1.2

2013-03-06 13:39:13

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 6/7] n_tty: Fix unsafe update of available buffer space

receive_room is used to control the amount of data the flip
buffer work can push to the read buffer. This update is unsafe:

CPU 0 | CPU 1
|
| n_tty_read()
| n_tty_set_room()
| left = <calc of space>
n_tty_receive_buf() |
<push data to buffer> |
n_tty_set_room() |
left = <calc of space> |
tty->receive_room = left |
| tty->receive_room = left

receive_room is now updated with a stale calculation of the
available buffer space, and the subsequent work loop will likely
overwrite unread data in the input buffer.

Update receive_room atomically with the calculation of the
available buffer space.

Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/n_tty.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index b2f621f..61a55f4 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -115,13 +115,14 @@ static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
}

/**
- * n_tty_set__room - receive space
+ * n_tty_set_room - receive space
* @tty: terminal
*
- * Called by the driver to find out how much data it is
- * permitted to feed to the line discipline without any being lost
- * and thus to manage flow control. Not serialized. Answers for the
- * "instant".
+ * Sets tty->receive_room to reflect the currently available space
+ * in the input buffer, and re-schedules the flip buffer work if space
+ * just became available.
+ *
+ * Locks: Concurrent update is protected with read_lock
*/

static void n_tty_set_room(struct tty_struct *tty)
@@ -129,8 +130,10 @@ static void n_tty_set_room(struct tty_struct *tty)
struct n_tty_data *ldata = tty->disc_data;
int left;
int old_left;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&ldata->read_lock, flags);

- /* ldata->read_cnt is not read locked ? */
if (I_PARMRK(tty)) {
/* Multiply read_cnt by 3, since each byte might take up to
* three times as many spaces when PARMRK is set (depending on
@@ -150,6 +153,8 @@ static void n_tty_set_room(struct tty_struct *tty)
old_left = tty->receive_room;
tty->receive_room = left;

+ raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
+
/* Did this open up the receive buffer? We may need to flip */
if (left && !old_left) {
WARN_RATELIMIT(tty->port->itty == NULL,
@@ -1885,7 +1890,6 @@ do_it_again:
retval = -ERESTARTSYS;
break;
}
- /* FIXME: does n_tty_set_room need locking ? */
n_tty_set_room(tty);
timeout = schedule_timeout(timeout);
continue;
--
1.8.1.2

2013-03-06 13:39:11

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 5/7] n_tty: Untangle read completion variables

Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/n_tty.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 3fd657b..b2f621f 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1814,20 +1814,16 @@ do_it_again:
minimum = time = 0;
timeout = MAX_SCHEDULE_TIMEOUT;
if (!ldata->icanon) {
- time = (HZ / 10) * TIME_CHAR(tty);
minimum = MIN_CHAR(tty);
if (minimum) {
+ time = (HZ / 10) * TIME_CHAR(tty);
if (time)
ldata->minimum_to_wake = 1;
else if (!waitqueue_active(&tty->read_wait) ||
(ldata->minimum_to_wake > minimum))
ldata->minimum_to_wake = minimum;
} else {
- timeout = 0;
- if (time) {
- timeout = time;
- time = 0;
- }
+ timeout = (HZ / 10) * TIME_CHAR(tty);
ldata->minimum_to_wake = minimum = 1;
}
}
--
1.8.1.2

2013-03-06 13:39:05

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 4/7] n_tty: Encapsulate minimum_to_wake within N_TTY

minimum_to_wake is unique to N_TTY processing, and belongs in
per-ldisc data.

Add the ldisc method, ldisc_ops::fasync(), to notify line disciplines
when signal-driven I/O is enabled or disabled. When enabled for N_TTY
(by fcntl(F_SETFL, O_ASYNC)), blocking reader/polls will be woken
for any readable input. When disabled, blocking reader/polls are not
woken until the read buffer is full.

Canonical mode (L_ICANON(tty), n_tty_data::icanon) is not affected by
the minimum_to_wake setting.

Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/n_tty.c | 39 +++++++++++++++++++++++++++------------
drivers/tty/tty_io.c | 17 +++++++++--------
include/linux/tty.h | 1 -
include/linux/tty_ldisc.h | 6 ++++++
4 files changed, 42 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 61797c4..3fd657b 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -89,6 +89,7 @@ struct n_tty_data {
int read_head;
int read_tail;
int read_cnt;
+ int minimum_to_wake;

unsigned char *echo_buf;
unsigned int echo_pos;
@@ -1471,7 +1472,7 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,

n_tty_set_room(tty);

- if ((!ldata->icanon && (ldata->read_cnt >= tty->minimum_to_wake)) ||
+ if ((!ldata->icanon && (ldata->read_cnt >= ldata->minimum_to_wake)) ||
L_EXTPROC(tty)) {
kill_fasync(&tty->fasync, SIGIO, POLL_IN);
if (waitqueue_active(&tty->read_wait))
@@ -1649,7 +1650,7 @@ static int n_tty_open(struct tty_struct *tty)
tty->disc_data = ldata;
reset_buffer_flags(tty->disc_data);
ldata->column = 0;
- tty->minimum_to_wake = 1;
+ ldata->minimum_to_wake = 1;
tty->closing = 0;
/* indicate buffer work may resume */
clear_bit(TTY_LDISC_HALTED, &tty->flags);
@@ -1817,17 +1818,17 @@ do_it_again:
minimum = MIN_CHAR(tty);
if (minimum) {
if (time)
- tty->minimum_to_wake = 1;
+ ldata->minimum_to_wake = 1;
else if (!waitqueue_active(&tty->read_wait) ||
- (tty->minimum_to_wake > minimum))
- tty->minimum_to_wake = minimum;
+ (ldata->minimum_to_wake > minimum))
+ ldata->minimum_to_wake = minimum;
} else {
timeout = 0;
if (time) {
timeout = time;
time = 0;
}
- tty->minimum_to_wake = minimum = 1;
+ ldata->minimum_to_wake = minimum = 1;
}
}

@@ -1867,9 +1868,9 @@ do_it_again:
TASK_RUNNING. */
set_current_state(TASK_INTERRUPTIBLE);

- if (((minimum - (b - buf)) < tty->minimum_to_wake) &&
+ if (((minimum - (b - buf)) < ldata->minimum_to_wake) &&
((minimum - (b - buf)) >= 1))
- tty->minimum_to_wake = (minimum - (b - buf));
+ ldata->minimum_to_wake = (minimum - (b - buf));

if (!input_available_p(tty, 0)) {
if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
@@ -1979,7 +1980,7 @@ do_it_again:
remove_wait_queue(&tty->read_wait, &wait);

if (!waitqueue_active(&tty->read_wait))
- tty->minimum_to_wake = minimum;
+ ldata->minimum_to_wake = minimum;

__set_current_state(TASK_RUNNING);
size = b - buf;
@@ -2111,6 +2112,7 @@ break_out:
static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
poll_table *wait)
{
+ struct n_tty_data *ldata = tty->disc_data;
unsigned int mask = 0;

poll_wait(file, &tty->read_wait, wait);
@@ -2125,9 +2127,9 @@ static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
mask |= POLLHUP;
if (!(mask & (POLLHUP | POLLIN | POLLRDNORM))) {
if (MIN_CHAR(tty) && !TIME_CHAR(tty))
- tty->minimum_to_wake = MIN_CHAR(tty);
+ ldata->minimum_to_wake = MIN_CHAR(tty);
else
- tty->minimum_to_wake = 1;
+ ldata->minimum_to_wake = 1;
}
if (tty->ops->write && !tty_is_writelocked(tty) &&
tty_chars_in_buffer(tty) < WAKEUP_CHARS &&
@@ -2175,6 +2177,18 @@ static int n_tty_ioctl(struct tty_struct *tty, struct file *file,
}
}

+static void n_tty_fasync(struct tty_struct *tty, int on)
+{
+ struct n_tty_data *ldata = tty->disc_data;
+
+ if (!waitqueue_active(&tty->read_wait)) {
+ if (on)
+ ldata->minimum_to_wake = 1;
+ else if (!tty->fasync)
+ ldata->minimum_to_wake = N_TTY_BUF_SIZE;
+ }
+}
+
struct tty_ldisc_ops tty_ldisc_N_TTY = {
.magic = TTY_LDISC_MAGIC,
.name = "n_tty",
@@ -2188,7 +2202,8 @@ struct tty_ldisc_ops tty_ldisc_N_TTY = {
.set_termios = n_tty_set_termios,
.poll = n_tty_poll,
.receive_buf = n_tty_receive_buf,
- .write_wakeup = n_tty_write_wakeup
+ .write_wakeup = n_tty_write_wakeup,
+ .fasync = n_tty_fasync,
};

/**
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index adc1d01..525c915 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2077,6 +2077,7 @@ static unsigned int tty_poll(struct file *filp, poll_table *wait)
static int __tty_fasync(int fd, struct file *filp, int on)
{
struct tty_struct *tty = file_tty(filp);
+ struct tty_ldisc *ldisc;
unsigned long flags;
int retval = 0;

@@ -2087,11 +2088,17 @@ static int __tty_fasync(int fd, struct file *filp, int on)
if (retval <= 0)
goto out;

+ ldisc = tty_ldisc_ref(tty);
+ if (ldisc) {
+ if (ldisc->ops->fasync)
+ ldisc->ops->fasync(tty, on);
+ tty_ldisc_deref(ldisc);
+ }
+
if (on) {
enum pid_type type;
struct pid *pid;
- if (!waitqueue_active(&tty->read_wait))
- tty->minimum_to_wake = 1;
+
spin_lock_irqsave(&tty->ctrl_lock, flags);
if (tty->pgrp) {
pid = tty->pgrp;
@@ -2104,13 +2111,7 @@ static int __tty_fasync(int fd, struct file *filp, int on)
spin_unlock_irqrestore(&tty->ctrl_lock, flags);
retval = __f_setown(filp, pid, type, 0);
put_pid(pid);
- if (retval)
- goto out;
- } else {
- if (!tty->fasync && !waitqueue_active(&tty->read_wait))
- tty->minimum_to_wake = N_TTY_BUF_SIZE;
}
- retval = 0;
out:
return retval;
}
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 2c109a3..39f0317 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -272,7 +272,6 @@ struct tty_struct {
#define N_TTY_BUF_SIZE 4096

unsigned char closing:1;
- unsigned short minimum_to_wake;
unsigned char *write_buf;
int write_cnt;
/* If the tty has a pending do_SAK, queue it here - akpm */
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index 1ef449e..44584d9 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -100,6 +100,11 @@
* seek to perform this action quickly but should wait until
* any pending driver I/O is completed.
*
+ * void (*fasync)(struct tty_struct *, int on)
+ *
+ * Notify line discipline when signal-driven I/O is enabled or
+ * disabled.
+ *
* void (*dcd_change)(struct tty_struct *tty, unsigned int status)
*
* Tells the discipline that the DCD pin has changed its status.
@@ -190,6 +195,7 @@ struct tty_ldisc_ops {
char *fp, int count);
void (*write_wakeup)(struct tty_struct *);
void (*dcd_change)(struct tty_struct *, unsigned int);
+ void (*fasync)(struct tty_struct *tty, int on);

struct module *owner;

--
1.8.1.2

2013-03-06 13:40:32

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 3/7] tty: Fix checkpatch errors in tty_ldisc.h

Signed-off-by: Peter Hurley <[email protected]>
---
include/linux/tty_ldisc.h | 132 +++++++++++++++++++++++-----------------------
1 file changed, 66 insertions(+), 66 deletions(-)

diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index a8c58c2..1ef449e 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -9,89 +9,89 @@
*
* int (*open)(struct tty_struct *);
*
- * This function is called when the line discipline is associated
- * with the tty. The line discipline can use this as an
- * opportunity to initialize any state needed by the ldisc routines.
- *
+ * This function is called when the line discipline is associated
+ * with the tty. The line discipline can use this as an
+ * opportunity to initialize any state needed by the ldisc routines.
+ *
* void (*close)(struct tty_struct *);
*
* This function is called when the line discipline is being
- * shutdown, either because the tty is being closed or because
- * the tty is being changed to use a new line discipline
- *
+ * shutdown, either because the tty is being closed or because
+ * the tty is being changed to use a new line discipline
+ *
* void (*flush_buffer)(struct tty_struct *tty);
*
- * This function instructs the line discipline to clear its
- * buffers of any input characters it may have queued to be
- * delivered to the user mode process.
- *
+ * This function instructs the line discipline to clear its
+ * buffers of any input characters it may have queued to be
+ * delivered to the user mode process.
+ *
* ssize_t (*chars_in_buffer)(struct tty_struct *tty);
*
- * This function returns the number of input characters the line
+ * This function returns the number of input characters the line
* discipline may have queued up to be delivered to the user mode
* process.
- *
+ *
* ssize_t (*read)(struct tty_struct * tty, struct file * file,
* unsigned char * buf, size_t nr);
*
- * This function is called when the user requests to read from
- * the tty. The line discipline will return whatever characters
- * it has buffered up for the user. If this function is not
- * defined, the user will receive an EIO error.
- *
+ * This function is called when the user requests to read from
+ * the tty. The line discipline will return whatever characters
+ * it has buffered up for the user. If this function is not
+ * defined, the user will receive an EIO error.
+ *
* ssize_t (*write)(struct tty_struct * tty, struct file * file,
- * const unsigned char * buf, size_t nr);
- *
- * This function is called when the user requests to write to the
- * tty. The line discipline will deliver the characters to the
- * low-level tty device for transmission, optionally performing
- * some processing on the characters first. If this function is
- * not defined, the user will receive an EIO error.
- *
+ * const unsigned char * buf, size_t nr);
+ *
+ * This function is called when the user requests to write to the
+ * tty. The line discipline will deliver the characters to the
+ * low-level tty device for transmission, optionally performing
+ * some processing on the characters first. If this function is
+ * not defined, the user will receive an EIO error.
+ *
* int (*ioctl)(struct tty_struct * tty, struct file * file,
- * unsigned int cmd, unsigned long arg);
+ * unsigned int cmd, unsigned long arg);
*
* This function is called when the user requests an ioctl which
- * is not handled by the tty layer or the low-level tty driver.
- * It is intended for ioctls which affect line discpline
- * operation. Note that the search order for ioctls is (1) tty
- * layer, (2) tty low-level driver, (3) line discpline. So a
- * low-level driver can "grab" an ioctl request before the line
- * discpline has a chance to see it.
- *
+ * is not handled by the tty layer or the low-level tty driver.
+ * It is intended for ioctls which affect line discpline
+ * operation. Note that the search order for ioctls is (1) tty
+ * layer, (2) tty low-level driver, (3) line discpline. So a
+ * low-level driver can "grab" an ioctl request before the line
+ * discpline has a chance to see it.
+ *
* long (*compat_ioctl)(struct tty_struct * tty, struct file * file,
- * unsigned int cmd, unsigned long arg);
+ * unsigned int cmd, unsigned long arg);
*
- * Process ioctl calls from 32-bit process on 64-bit system
+ * Process ioctl calls from 32-bit process on 64-bit system
*
* void (*set_termios)(struct tty_struct *tty, struct ktermios * old);
*
- * This function notifies the line discpline that a change has
- * been made to the termios structure.
- *
+ * This function notifies the line discpline that a change has
+ * been made to the termios structure.
+ *
* int (*poll)(struct tty_struct * tty, struct file * file,
- * poll_table *wait);
+ * poll_table *wait);
*
- * This function is called when a user attempts to select/poll on a
- * tty device. It is solely the responsibility of the line
- * discipline to handle poll requests.
+ * This function is called when a user attempts to select/poll on a
+ * tty device. It is solely the responsibility of the line
+ * discipline to handle poll requests.
*
* void (*receive_buf)(struct tty_struct *, const unsigned char *cp,
- * char *fp, int count);
- *
- * This function is called by the low-level tty driver to send
- * characters received by the hardware to the line discpline for
- * processing. <cp> is a pointer to the buffer of input
- * character received by the device. <fp> is a pointer to a
- * pointer of flag bytes which indicate whether a character was
- * received with a parity error, etc.
- *
+ * char *fp, int count);
+ *
+ * This function is called by the low-level tty driver to send
+ * characters received by the hardware to the line discpline for
+ * processing. <cp> is a pointer to the buffer of input
+ * character received by the device. <fp> is a pointer to a
+ * pointer of flag bytes which indicate whether a character was
+ * received with a parity error, etc.
+ *
* void (*write_wakeup)(struct tty_struct *);
*
- * This function is called by the low-level tty driver to signal
- * that line discpline should try to send more characters to the
- * low-level driver for transmission. If the line discpline does
- * not have any more data to send, it can just return.
+ * This function is called by the low-level tty driver to signal
+ * that line discpline should try to send more characters to the
+ * low-level driver for transmission. If the line discpline does
+ * not have any more data to send, it can just return.
*
* int (*hangup)(struct tty_struct *)
*
@@ -162,7 +162,7 @@ struct tty_ldisc_ops {
char *name;
int num;
int flags;
-
+
/*
* The following routines are called from above.
*/
@@ -170,19 +170,19 @@ struct tty_ldisc_ops {
void (*close)(struct tty_struct *);
void (*flush_buffer)(struct tty_struct *tty);
ssize_t (*chars_in_buffer)(struct tty_struct *tty);
- ssize_t (*read)(struct tty_struct * tty, struct file * file,
- unsigned char __user * buf, size_t nr);
- ssize_t (*write)(struct tty_struct * tty, struct file * file,
- const unsigned char * buf, size_t nr);
- int (*ioctl)(struct tty_struct * tty, struct file * file,
+ ssize_t (*read)(struct tty_struct *tty, struct file *file,
+ unsigned char __user *buf, size_t nr);
+ ssize_t (*write)(struct tty_struct *tty, struct file *file,
+ const unsigned char *buf, size_t nr);
+ int (*ioctl)(struct tty_struct *tty, struct file *file,
unsigned int cmd, unsigned long arg);
- long (*compat_ioctl)(struct tty_struct * tty, struct file * file,
+ long (*compat_ioctl)(struct tty_struct *tty, struct file *file,
unsigned int cmd, unsigned long arg);
- void (*set_termios)(struct tty_struct *tty, struct ktermios * old);
+ void (*set_termios)(struct tty_struct *tty, struct ktermios *old);
unsigned int (*poll)(struct tty_struct *, struct file *,
struct poll_table_struct *);
int (*hangup)(struct tty_struct *tty);
-
+
/*
* The following routines are called from below.
*/
@@ -192,7 +192,7 @@ struct tty_ldisc_ops {
void (*dcd_change)(struct tty_struct *, unsigned int);

struct module *owner;
-
+
int refcount;
};

--
1.8.1.2

2013-03-06 13:40:53

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 1/7] n_tty: Fix unsafe driver-side signals

An ldisc reference is insufficient guarantee the foreground process
group is not in the process of being signalled from a hangup.

1) Reads of tty->pgrp must be locked with ctrl_lock
2) The group pid must be referenced for the duration of signalling.
Because the driver-side is not process-context, a pid reference
must be acquired.

Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/n_tty.c | 34 ++++++++++++++--------------------
1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 66ce178..cd9ba3d 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1038,23 +1038,19 @@ static void eraser(unsigned char c, struct tty_struct *tty)
* isig - handle the ISIG optio
* @sig: signal
* @tty: terminal
- * @flush: force flush
*
- * Called when a signal is being sent due to terminal input. This
- * may caus terminal flushing to take place according to the termios
- * settings and character used. Called from the driver receive_buf
- * path so serialized.
+ * Called when a signal is being sent due to terminal input.
+ * Called from the driver receive_buf path so serialized.
*
- * Locking: ctrl_lock, read_lock (both via flush buffer)
+ * Locking: ctrl_lock
*/

-static inline void isig(int sig, struct tty_struct *tty, int flush)
+static inline void isig(int sig, struct tty_struct *tty)
{
- if (tty->pgrp)
- kill_pgrp(tty->pgrp, sig, 1);
- if (flush || !L_NOFLSH(tty)) {
- n_tty_flush_buffer(tty);
- tty_driver_flush_buffer(tty);
+ struct pid *tty_pgrp = tty_get_pgrp(tty);
+ if (tty_pgrp) {
+ kill_pgrp(tty_pgrp, sig, 1);
+ put_pid(tty_pgrp);
}
}

@@ -1075,7 +1071,11 @@ static inline void n_tty_receive_break(struct tty_struct *tty)
if (I_IGNBRK(tty))
return;
if (I_BRKINT(tty)) {
- isig(SIGINT, tty, 1);
+ isig(SIGINT, tty);
+ if (!L_NOFLSH(tty)) {
+ n_tty_flush_buffer(tty);
+ tty_driver_flush_buffer(tty);
+ }
return;
}
if (I_PARMRK(tty)) {
@@ -1242,11 +1242,6 @@ static inline void n_tty_receive_char(struct tty_struct *tty, unsigned char c)
signal = SIGTSTP;
if (c == SUSP_CHAR(tty)) {
send_signal:
- /*
- * Note that we do not use isig() here because we want
- * the order to be:
- * 1) flush, 2) echo, 3) signal
- */
if (!L_NOFLSH(tty)) {
n_tty_flush_buffer(tty);
tty_driver_flush_buffer(tty);
@@ -1257,8 +1252,7 @@ send_signal:
echo_char(c, tty);
process_echoes(tty);
}
- if (tty->pgrp)
- kill_pgrp(tty->pgrp, signal, 1);
+ isig(signal, tty);
return;
}
}
--
1.8.1.2

2013-03-18 23:14:24

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH 4/7] n_tty: Encapsulate minimum_to_wake within N_TTY

On Wed, Mar 06, 2013 at 08:38:22AM -0500, Peter Hurley wrote:
> minimum_to_wake is unique to N_TTY processing, and belongs in
> per-ldisc data.
>
> Add the ldisc method, ldisc_ops::fasync(), to notify line disciplines
> when signal-driven I/O is enabled or disabled. When enabled for N_TTY
> (by fcntl(F_SETFL, O_ASYNC)), blocking reader/polls will be woken
> for any readable input. When disabled, blocking reader/polls are not
> woken until the read buffer is full.
>
> Canonical mode (L_ICANON(tty), n_tty_data::icanon) is not affected by
> the minimum_to_wake setting.
>
> Signed-off-by: Peter Hurley <[email protected]>

For some reason, this patch doesn't apply. Care to refresh this one,
and the rest in this series, and resend?

thanks,

greg k-h

2013-03-19 13:57:17

by Peter Hurley

[permalink] [raw]

Subject: Re: [PATCH 4/7] n_tty: Encapsulate minimum_to_wake within N_TTY

On Mon, 2013-03-18 at 16:15 -0700, Greg Kroah-Hartman wrote:
> On Wed, Mar 06, 2013 at 08:38:22AM -0500, Peter Hurley wrote:
> > minimum_to_wake is unique to N_TTY processing, and belongs in
> > per-ldisc data.
> >
> > Add the ldisc method, ldisc_ops::fasync(), to notify line disciplines
> > when signal-driven I/O is enabled or disabled. When enabled for N_TTY
> > (by fcntl(F_SETFL, O_ASYNC)), blocking reader/polls will be woken
> > for any readable input. When disabled, blocking reader/polls are not
> > woken until the read buffer is full.
> >
> > Canonical mode (L_ICANON(tty), n_tty_data::icanon) is not affected by
> > the minimum_to_wake setting.
> >
> > Signed-off-by: Peter Hurley <[email protected]>
>
> For some reason, this patch doesn't apply. Care to refresh this one,
> and the rest in this series, and resend?

Sorry. There was probably some accidental dependency on one of the other
patchsets of mine you did apply.

This and patch 5 now apply without error to tty-next.

2013-03-19 14:09:02

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH 4/7] n_tty: Encapsulate minimum_to_wake within N_TTY

On Tue, Mar 19, 2013 at 09:57:09AM -0400, Peter Hurley wrote:
> On Mon, 2013-03-18 at 16:15 -0700, Greg Kroah-Hartman wrote:
> > On Wed, Mar 06, 2013 at 08:38:22AM -0500, Peter Hurley wrote:
> > > minimum_to_wake is unique to N_TTY processing, and belongs in
> > > per-ldisc data.
> > >
> > > Add the ldisc method, ldisc_ops::fasync(), to notify line disciplines
> > > when signal-driven I/O is enabled or disabled. When enabled for N_TTY
> > > (by fcntl(F_SETFL, O_ASYNC)), blocking reader/polls will be woken
> > > for any readable input. When disabled, blocking reader/polls are not
> > > woken until the read buffer is full.
> > >
> > > Canonical mode (L_ICANON(tty), n_tty_data::icanon) is not affected by
> > > the minimum_to_wake setting.
> > >
> > > Signed-off-by: Peter Hurley <[email protected]>
> >
> > For some reason, this patch doesn't apply. Care to refresh this one,
> > and the rest in this series, and resend?
>
> Sorry. There was probably some accidental dependency on one of the other
> patchsets of mine you did apply.
>
> This and patch 5 now apply without error to tty-next.

Ok, but they are now long gone from my queue. Can you please resend
what I haven't applied?

thanks,

greg k-h

2013-03-19 14:27:36

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 5/7] n_tty: Untangle read completion variables

2013-03-19 14:27:39

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 6/7] n_tty: Fix unsafe update of available buffer space

2013-03-19 14:27:52

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 7/7] n_tty: Buffer work should not reschedule itself

2013-03-19 14:28:34

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 4/7] n_tty: Encapsulate minimum_to_wake within N_TTY

2013-03-19 20:35:48

by Ilya Zykov

[permalink] [raw]

Subject: Re: [PATCH 6/7] n_tty: Fix unsafe update of available buffer space

On 19.03.2013 18:26, Peter Hurley wrote:
> receive_room is used to control the amount of data the flip
> buffer work can push to the read buffer. This update is unsafe:
>
> CPU 0 | CPU 1
> |
> | n_tty_read()
> | n_tty_set_room()
> | left = <calc of space>
> n_tty_receive_buf() |
> <push data to buffer> |
> n_tty_set_room() |
> left = <calc of space> |
> tty->receive_room = left |
> | tty->receive_room = left
>
> receive_room is now updated with a stale calculation of the
> available buffer space, and the subsequent work loop will likely
> overwrite unread data in the input buffer.
>

Sounds reasonable to me.
Thank you.
Ilya Zykov <[email protected]>

2013-03-19 20:35:47

by Ilya Zykov

[permalink] [raw]

Subject: Re: [PATCH 7/7] n_tty: Buffer work should not reschedule itself

On 19.03.2013 18:26, Peter Hurley wrote:
> Although the driver-side input path must update the available
> buffer space, it should not reschedule itself. If space is still
> available and the flip buffers are not empty, flush_to_ldisc()
> will loop again.
>

Sounds reasonable to me.
Thank you.
Ilya Zykov <[email protected]>

2013-06-15 11:28:50

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 0/4] [resend] n_tty fixes

On 03/19/2013 10:10 AM, Greg Kroah-Hartman wrote:> On Tue, Mar 19, 2013 at 09:57:09AM -0400, Peter Hurley wrote:
>> On Mon, 2013-03-18 at 16:15 -0700, Greg Kroah-Hartman wrote:
>>> On Wed, Mar 06, 2013 at 08:38:22AM -0500, Peter Hurley wrote:
>>>> minimum_to_wake is unique to N_TTY processing, and belongs in
>>>> per-ldisc data.
>>>>
>>>> Add the ldisc method, ldisc_ops::fasync(), to notify line disciplines
>>>> when signal-driven I/O is enabled or disabled. When enabled for N_TTY
>>>> (by fcntl(F_SETFL, O_ASYNC)), blocking reader/polls will be woken
>>>> for any readable input. When disabled, blocking reader/polls are not
>>>> woken until the read buffer is full.
>>>>
>>>> Canonical mode (L_ICANON(tty), n_tty_data::icanon) is not affected by
>>>> the minimum_to_wake setting.
>>>>
>>>> Signed-off-by: Peter Hurley <[email protected]>
>>>
>>> For some reason, this patch doesn't apply. Care to refresh this one,
>>> and the rest in this series, and resend?
>>
>> Sorry. There was probably some accidental dependency on one of the other
>> patchsets of mine you did apply.
>>
>> This and patch 5 now apply without error to tty-next.
>
> Ok, but they are now long gone from my queue. Can you please resend
> what I haven't applied?

Greg,

I resent these back on 19 Mar but they never got applied. (maybe because
I resent them as 4,5,6 & 7/7 ??)

Anyway, these apply cleanly to tty-next.

Regards,
Peter Hurley

Peter Hurley (4):
n_tty: Encapsulate minimum_to_wake within N_TTY
n_tty: Untangle read completion variables
n_tty: Fix unsafe update of available buffer space
n_tty: Buffer work should not reschedule itself

drivers/tty/n_tty.c | 76 ++++++++++++++++++++++++++++++-----------------
drivers/tty/tty_io.c | 17 ++++++-----
include/linux/tty.h | 1 -
include/linux/tty_ldisc.h | 6 ++++
4 files changed, 63 insertions(+), 37 deletions(-)

--
1.8.1.2

2013-06-15 11:29:07

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 1/4] n_tty: Encapsulate minimum_to_wake within N_TTY

minimum_to_wake is unique to N_TTY processing, and belongs in
per-ldisc data.

Add the ldisc method, ldisc_ops::fasync(), to notify line disciplines
when signal-driven I/O is enabled or disabled. When enabled for N_TTY
(by fcntl(F_SETFL, O_ASYNC)), blocking reader/polls will be woken
for any readable input. When disabled, blocking reader/polls are not
woken until the read buffer is full.

Canonical mode (L_ICANON(tty), n_tty_data::icanon) is not affected by
the minimum_to_wake setting.

Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/n_tty.c | 39 +++++++++++++++++++++++++++------------
drivers/tty/tty_io.c | 17 +++++++++--------
include/linux/tty.h | 1 -
include/linux/tty_ldisc.h | 6 ++++++
4 files changed, 42 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index cdcdb0e..f1806de 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -89,6 +89,7 @@ struct n_tty_data {
int read_head;
int read_tail;
int read_cnt;
+ int minimum_to_wake;

unsigned char *echo_buf;
unsigned int echo_pos;
@@ -1455,7 +1456,7 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,

n_tty_set_room(tty);

- if ((!ldata->icanon && (ldata->read_cnt >= tty->minimum_to_wake)) ||
+ if ((!ldata->icanon && (ldata->read_cnt >= ldata->minimum_to_wake)) ||
L_EXTPROC(tty)) {
kill_fasync(&tty->fasync, SIGIO, POLL_IN);
if (waitqueue_active(&tty->read_wait))
@@ -1636,7 +1637,7 @@ static int n_tty_open(struct tty_struct *tty)
tty->disc_data = ldata;
reset_buffer_flags(tty->disc_data);
ldata->column = 0;
- tty->minimum_to_wake = 1;
+ ldata->minimum_to_wake = 1;
tty->closing = 0;
/* indicate buffer work may resume */
clear_bit(TTY_LDISC_HALTED, &tty->flags);
@@ -1804,17 +1805,17 @@ do_it_again:
minimum = MIN_CHAR(tty);
if (minimum) {
if (time)
- tty->minimum_to_wake = 1;
+ ldata->minimum_to_wake = 1;
else if (!waitqueue_active(&tty->read_wait) ||
- (tty->minimum_to_wake > minimum))
- tty->minimum_to_wake = minimum;
+ (ldata->minimum_to_wake > minimum))
+ ldata->minimum_to_wake = minimum;
} else {
timeout = 0;
if (time) {
timeout = time;
time = 0;
}
- tty->minimum_to_wake = minimum = 1;
+ ldata->minimum_to_wake = minimum = 1;
}
}

@@ -1854,9 +1855,9 @@ do_it_again:
TASK_RUNNING. */
set_current_state(TASK_INTERRUPTIBLE);

- if (((minimum - (b - buf)) < tty->minimum_to_wake) &&
+ if (((minimum - (b - buf)) < ldata->minimum_to_wake) &&
((minimum - (b - buf)) >= 1))
- tty->minimum_to_wake = (minimum - (b - buf));
+ ldata->minimum_to_wake = (minimum - (b - buf));

if (!input_available_p(tty, 0)) {
if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
@@ -1973,7 +1974,7 @@ do_it_again:
remove_wait_queue(&tty->read_wait, &wait);

if (!waitqueue_active(&tty->read_wait))
- tty->minimum_to_wake = minimum;
+ ldata->minimum_to_wake = minimum;

__set_current_state(TASK_RUNNING);
size = b - buf;
@@ -2105,6 +2106,7 @@ break_out:
static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
poll_table *wait)
{
+ struct n_tty_data *ldata = tty->disc_data;
unsigned int mask = 0;

poll_wait(file, &tty->read_wait, wait);
@@ -2119,9 +2121,9 @@ static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
mask |= POLLHUP;
if (!(mask & (POLLHUP | POLLIN | POLLRDNORM))) {
if (MIN_CHAR(tty) && !TIME_CHAR(tty))
- tty->minimum_to_wake = MIN_CHAR(tty);
+ ldata->minimum_to_wake = MIN_CHAR(tty);
else
- tty->minimum_to_wake = 1;
+ ldata->minimum_to_wake = 1;
}
if (tty->ops->write && !tty_is_writelocked(tty) &&
tty_chars_in_buffer(tty) < WAKEUP_CHARS &&
@@ -2169,6 +2171,18 @@ static int n_tty_ioctl(struct tty_struct *tty, struct file *file,
}
}

+static void n_tty_fasync(struct tty_struct *tty, int on)
+{
+ struct n_tty_data *ldata = tty->disc_data;
+
+ if (!waitqueue_active(&tty->read_wait)) {
+ if (on)
+ ldata->minimum_to_wake = 1;
+ else if (!tty->fasync)
+ ldata->minimum_to_wake = N_TTY_BUF_SIZE;
+ }
+}
+
struct tty_ldisc_ops tty_ldisc_N_TTY = {
.magic = TTY_LDISC_MAGIC,
.name = "n_tty",
@@ -2182,7 +2196,8 @@ struct tty_ldisc_ops tty_ldisc_N_TTY = {
.set_termios = n_tty_set_termios,
.poll = n_tty_poll,
.receive_buf = n_tty_receive_buf,
- .write_wakeup = n_tty_write_wakeup
+ .write_wakeup = n_tty_write_wakeup,
+ .fasync = n_tty_fasync,
};

/**
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 5ee51ba..6cd08d9 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2137,6 +2137,7 @@ static unsigned int tty_poll(struct file *filp, poll_table *wait)
static int __tty_fasync(int fd, struct file *filp, int on)
{
struct tty_struct *tty = file_tty(filp);
+ struct tty_ldisc *ldisc;
unsigned long flags;
int retval = 0;

@@ -2147,11 +2148,17 @@ static int __tty_fasync(int fd, struct file *filp, int on)
if (retval <= 0)
goto out;

+ ldisc = tty_ldisc_ref(tty);
+ if (ldisc) {
+ if (ldisc->ops->fasync)
+ ldisc->ops->fasync(tty, on);
+ tty_ldisc_deref(ldisc);
+ }
+
if (on) {
enum pid_type type;
struct pid *pid;
- if (!waitqueue_active(&tty->read_wait))
- tty->minimum_to_wake = 1;
+
spin_lock_irqsave(&tty->ctrl_lock, flags);
if (tty->pgrp) {
pid = tty->pgrp;
@@ -2164,13 +2171,7 @@ static int __tty_fasync(int fd, struct file *filp, int on)
spin_unlock_irqrestore(&tty->ctrl_lock, flags);
retval = __f_setown(filp, pid, type, 0);
put_pid(pid);
- if (retval)
- goto out;
- } else {
- if (!tty->fasync && !waitqueue_active(&tty->read_wait))
- tty->minimum_to_wake = N_TTY_BUF_SIZE;
}
- retval = 0;
out:
return retval;
}
diff --git a/include/linux/tty.h b/include/linux/tty.h
index a665c7e..7269daf 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -272,7 +272,6 @@ struct tty_struct {
#define N_TTY_BUF_SIZE 4096

unsigned char closing:1;
- unsigned short minimum_to_wake;
unsigned char *write_buf;
int write_cnt;
/* If the tty has a pending do_SAK, queue it here - akpm */
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index 22ee107..23bdd9d 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -100,6 +100,11 @@
* seek to perform this action quickly but should wait until
* any pending driver I/O is completed.
*
+ * void (*fasync)(struct tty_struct *, int on)
+ *
+ * Notify line discipline when signal-driven I/O is enabled or
+ * disabled.
+ *
* void (*dcd_change)(struct tty_struct *tty, unsigned int status)
*
* Tells the discipline that the DCD pin has changed its status.
@@ -189,6 +194,7 @@ struct tty_ldisc_ops {
char *fp, int count);
void (*write_wakeup)(struct tty_struct *);
void (*dcd_change)(struct tty_struct *, unsigned int);
+ void (*fasync)(struct tty_struct *tty, int on);

struct module *owner;

--
1.8.1.2

2013-06-15 11:29:32

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 3/4] n_tty: Fix unsafe update of available buffer space

receive_room is used to control the amount of data the flip
buffer work can push to the read buffer. This update is unsafe:

CPU 0 | CPU 1
|
| n_tty_read()
| n_tty_set_room()
| left = <calc of space>
n_tty_receive_buf() |
<push data to buffer> |
n_tty_set_room() |
left = <calc of space> |
tty->receive_room = left |
| tty->receive_room = left

receive_room is now updated with a stale calculation of the
available buffer space, and the subsequent work loop will likely
overwrite unread data in the input buffer.

Update receive_room atomically with the calculation of the
available buffer space.

Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/n_tty.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index fa5cb46..0646165 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -115,13 +115,14 @@ static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
}

/**
- * n_tty_set__room - receive space
+ * n_tty_set_room - receive space
* @tty: terminal
*
- * Called by the driver to find out how much data it is
- * permitted to feed to the line discipline without any being lost
- * and thus to manage flow control. Not serialized. Answers for the
- * "instant".
+ * Sets tty->receive_room to reflect the currently available space
+ * in the input buffer, and re-schedules the flip buffer work if space
+ * just became available.
+ *
+ * Locks: Concurrent update is protected with read_lock
*/

static void n_tty_set_room(struct tty_struct *tty)
@@ -129,8 +130,10 @@ static void n_tty_set_room(struct tty_struct *tty)
struct n_tty_data *ldata = tty->disc_data;
int left;
int old_left;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&ldata->read_lock, flags);

- /* ldata->read_cnt is not read locked ? */
if (I_PARMRK(tty)) {
/* Multiply read_cnt by 3, since each byte might take up to
* three times as many spaces when PARMRK is set (depending on
@@ -150,6 +153,8 @@ static void n_tty_set_room(struct tty_struct *tty)
old_left = tty->receive_room;
tty->receive_room = left;

+ raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
+
/* Did this open up the receive buffer? We may need to flip */
if (left && !old_left) {
WARN_RATELIMIT(tty->port->itty == NULL,
@@ -1872,7 +1877,6 @@ do_it_again:
retval = -ERESTARTSYS;
break;
}
- /* FIXME: does n_tty_set_room need locking ? */
n_tty_set_room(tty);
timeout = schedule_timeout(timeout);
continue;
--
1.8.1.2

2013-06-15 11:29:29

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 2/4] n_tty: Untangle read completion variables

Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/n_tty.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index f1806de..fa5cb46 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1801,20 +1801,16 @@ do_it_again:
minimum = time = 0;
timeout = MAX_SCHEDULE_TIMEOUT;
if (!ldata->icanon) {
- time = (HZ / 10) * TIME_CHAR(tty);
minimum = MIN_CHAR(tty);
if (minimum) {
+ time = (HZ / 10) * TIME_CHAR(tty);
if (time)
ldata->minimum_to_wake = 1;
else if (!waitqueue_active(&tty->read_wait) ||
(ldata->minimum_to_wake > minimum))
ldata->minimum_to_wake = minimum;
} else {
- timeout = 0;
- if (time) {
- timeout = time;
- time = 0;
- }
+ timeout = (HZ / 10) * TIME_CHAR(tty);
ldata->minimum_to_wake = minimum = 1;
}
}
--
1.8.1.2

2013-06-15 11:29:35

by Peter Hurley

[permalink] [raw]

Subject: [PATCH 4/4] n_tty: Buffer work should not reschedule itself

Although the driver-side input path must update the available
buffer space, it should not reschedule itself. If space is still
available and the flip buffers are not empty, flush_to_ldisc()
will loop again.

Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/n_tty.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 0646165..4bf0fc0 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -118,14 +118,14 @@ static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
* n_tty_set_room - receive space
* @tty: terminal
*
- * Sets tty->receive_room to reflect the currently available space
+ * Updates tty->receive_room to reflect the currently available space
* in the input buffer, and re-schedules the flip buffer work if space
* just became available.
*
* Locks: Concurrent update is protected with read_lock
*/

-static void n_tty_set_room(struct tty_struct *tty)
+static int set_room(struct tty_struct *tty)
{
struct n_tty_data *ldata = tty->disc_data;
int left;
@@ -155,8 +155,13 @@ static void n_tty_set_room(struct tty_struct *tty)

raw_spin_unlock_irqrestore(&ldata->read_lock, flags);

+ return left && !old_left;
+}
+
+static void n_tty_set_room(struct tty_struct *tty)
+{
/* Did this open up the receive buffer? We may need to flip */
- if (left && !old_left) {
+ if (set_room(tty)) {
WARN_RATELIMIT(tty->port->itty == NULL,
"scheduling with invalid itty\n");
/* see if ldisc has been killed - if so, this means that
@@ -1459,7 +1464,7 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
tty->ops->flush_chars(tty);
}

- n_tty_set_room(tty);
+ set_room(tty);

if ((!ldata->icanon && (ldata->read_cnt >= ldata->minimum_to_wake)) ||
L_EXTPROC(tty)) {
--
1.8.1.2

2013-06-17 19:58:54

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH 0/4] [resend] n_tty fixes

On Sat, Jun 15, 2013 at 07:28:27AM -0400, Peter Hurley wrote:
> On 03/19/2013 10:10 AM, Greg Kroah-Hartman wrote:> On Tue, Mar 19, 2013 at 09:57:09AM -0400, Peter Hurley wrote:
> >> On Mon, 2013-03-18 at 16:15 -0700, Greg Kroah-Hartman wrote:
> >>> On Wed, Mar 06, 2013 at 08:38:22AM -0500, Peter Hurley wrote:
> >>>> minimum_to_wake is unique to N_TTY processing, and belongs in
> >>>> per-ldisc data.
> >>>>
> >>>> Add the ldisc method, ldisc_ops::fasync(), to notify line disciplines
> >>>> when signal-driven I/O is enabled or disabled. When enabled for N_TTY
> >>>> (by fcntl(F_SETFL, O_ASYNC)), blocking reader/polls will be woken
> >>>> for any readable input. When disabled, blocking reader/polls are not
> >>>> woken until the read buffer is full.
> >>>>
> >>>> Canonical mode (L_ICANON(tty), n_tty_data::icanon) is not affected by
> >>>> the minimum_to_wake setting.
> >>>>
> >>>> Signed-off-by: Peter Hurley <[email protected]>
> >>>
> >>> For some reason, this patch doesn't apply. Care to refresh this one,
> >>> and the rest in this series, and resend?
> >>
> >> Sorry. There was probably some accidental dependency on one of the other
> >> patchsets of mine you did apply.
> >>
> >> This and patch 5 now apply without error to tty-next.
> >
> > Ok, but they are now long gone from my queue. Can you please resend
> > what I haven't applied?
>
> Greg,
>
> I resent these back on 19 Mar but they never got applied. (maybe because
> I resent them as 4,5,6 & 7/7 ??)

Yes, they were burried, sorry about that :)

> Anyway, these apply cleanly to tty-next.

Now applied, thanks.

greg k-h