16 Apr, 2019
1 commit
-
The return from tty_write_room could potentially be negative if
a tty write_room driver returns an error number (not that any seem
to do). Rather than just check for a zero return, also check for
a -ve return. This avoids the unsigned nr being set to a large unsigned
value on the assignment from variable space and can lead to overflowing
the buffer buf. Better to be safe than assume all write_room
implementations in tty drivers are going to do the right thing.Signed-off-by: Colin Ian King
Signed-off-by: Greg Kroah-Hartman
18 Jan, 2019
1 commit
-
Give a better descriptions of what WAKEUP_CHARS represents.
Signed-off-by: Valentin Vidic
Acked-by: Jiri Slaby
Signed-off-by: Greg Kroah-Hartman
05 Dec, 2018
1 commit
-
There might be situations where tty_ldisc_lock() has blocked, but there
is already IO on tty and it prevents line discipline changes.
It might theoretically turn into dead-lock.Basically, provide more priority to pending tty_ldisc_lock() than to
servicing reads/writes over tty.User-visible issue was reported by Mikulas where on pa-risc with
Debian 5 reboot took either 80 seconds, 3 minutes or 3:25 after proper
locking in tty_reopen().Cc: Jiri Slaby
Reported-by: Mikulas Patocka
Signed-off-by: Dmitry Safonov
Signed-off-by: Greg Kroah-Hartman
12 Oct, 2018
1 commit
-
If we are not echoing the data to userspace or the console is in icanon
mode, then perhaps it is a "secret" so we should wipe it once we are
done with it.This mirrors the logic that the audit code has.
Reported-by: aszlig
Tested-by: Milan Broz
Tested-by: Daniel Zatovic
Tested-by: aszlig
Cc: Willy Tarreau
Signed-off-by: Greg Kroah-Hartman
28 Jun, 2018
2 commits
-
syzbot is reporting stalls at __process_echoes() [1]. This is because
since ldata->echo_commit < ldata->echo_tail becomes true for some reason,
the discard loop is serving as almost infinite loop. This patch tries to
avoid falling into ldata->echo_commit < ldata->echo_tail situation by
making access to echo_* variables more carefully.Since reset_buffer_flags() is called without output_lock held, it should
not touch echo_* variables. And omit a call to reset_buffer_flags() from
n_tty_open() by using vzalloc().Since add_echo_byte() is called without output_lock held, it needs memory
barrier between storing into echo_buf[] and incrementing echo_head counter.
echo_buf() needs corresponding memory barrier before reading echo_buf[].
Lack of handling the possibility of not-yet-stored multi-byte operation
might be the reason of falling into ldata->echo_commit < ldata->echo_tail
situation, for if I do WARN_ON(ldata->echo_commit == tail + 1) prior to
echo_buf(ldata, tail + 1), the WARN_ON() fires.Also, explicitly masking with buffer for the former "while" loop, and
use ldata->echo_commit > tail for the latter "while" loop.[1] https://syzkaller.appspot.com/bug?id=17f23b094cd80df750e5b0f8982c521ee6bcbf40
Signed-off-by: Tetsuo Handa
Reported-by: syzbot
Cc: Peter Hurley
Cc: stable
Signed-off-by: Greg Kroah-Hartman -
syzbot is reporting stalls at n_tty_receive_char_special() [1]. This is
because comparison is not working as expected since ldata->read_head can
change at any moment. Mitigate this by explicitly masking with buffer size
when checking condition for "while" loops.[1] https://syzkaller.appspot.com/bug?id=3d7481a346958d9469bebbeb0537d5f056bdd6e8
Signed-off-by: Tetsuo Handa
Reported-by: syzbot
Fixes: bc5a5e3f45d04784 ("n_tty: Don't wrap input buffer indices at buffer size")
Cc: stable
Cc: Peter Hurley
Signed-off-by: Greg Kroah-Hartman
28 Feb, 2018
1 commit
-
A tty is hung up by __tty_hangup() setting file->f_op to
hung_up_tty_fops, which is skipped on ttys whose write operation isn't
tty_write(). This means that, for example, /dev/console whose write
op is redirected_tty_write() is never actually marked hung up.Because n_tty_read() uses the hung up status to decide whether to
abort the waiting readers, the lack of hung-up marking can lead to the
following scenario.1. A session contains two processes. The leader and its child. The
child ignores SIGHUP.2. The leader exits and starts disassociating from the controlling
terminal (/dev/console).3. __tty_hangup() skips setting f_op to hung_up_tty_fops.
4. SIGHUP is delivered and ignored.
5. tty_ldisc_hangup() is invoked. It wakes up the waits which should
clear the read lockers of tty->ldisc_sem.6. The reader wakes up but because tty_hung_up_p() is false, it
doesn't abort and goes back to sleep while read-holding
tty->ldisc_sem.7. The leader progresses to tty_ldisc_lock() in tty_ldisc_hangup()
and is now stuck in D sleep indefinitely waiting for
tty->ldisc_sem.The following is Alan's explanation on why some ttys aren't hung up.
http://lkml.kernel.org/r/20171101170908.6ad08580@alans-desktop
1. It broke the serial consoles because they would hang up and close
down the hardware. With tty_port that *should* be fixable properly
for any cases remaining.2. The console layer was (and still is) completely broken and doens't
refcount properly. So if you turn on console hangups it breaks (as
indeed does freeing consoles and half a dozen other things).As neither can be fixed quickly, this patch works around the problem
by introducing a new flag, TTY_HUPPING, which is used solely to tell
n_tty_read() that hang-up is in progress for the console and the
readers should be aborted regardless of the hung-up status of the
device.The following is a sample hung task warning caused by this issue.
INFO: task agetty:2662 blocked for more than 120 seconds.
Not tainted 4.11.3-dbg-tty-lockup-02478-gfd6c7ee-dirty #28
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
0 2662 1 0x00000086
Call Trace:
__schedule+0x267/0x890
schedule+0x36/0x80
schedule_timeout+0x23c/0x2e0
ldsem_down_write+0xce/0x1f6
tty_ldisc_lock+0x16/0x30
tty_ldisc_hangup+0xb3/0x1b0
__tty_hangup+0x300/0x410
disassociate_ctty+0x6c/0x290
do_exit+0x7ef/0xb00
do_group_exit+0x3f/0xa0
get_signal+0x1b3/0x5d0
do_signal+0x28/0x660
exit_to_usermode_loop+0x46/0x86
do_syscall_64+0x9c/0xb0
entry_SYSCALL64_slow_path+0x25/0x25The following is the repro. Run "$PROG /dev/console". The parent
process hangs in D state.#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#includeint main(int argc, char **argv)
{
struct sigaction sact = { .sa_handler = SIG_IGN };
struct timespec ts1s = { .tv_sec = 1 };
pid_t pid;
int fd;if (argc < 2) {
fprintf(stderr, "test-hung-tty /dev/$TTY\n");
return 1;
}/* fork a child to ensure that it isn't already the session leader */
pid = fork();
if (pid < 0) {
perror("fork");
return 1;
}if (pid > 0) {
/* top parent, wait for everyone */
while (waitpid(-1, NULL, 0) >= 0)
;
if (errno != ECHILD)
perror("waitpid");
return 0;
}/* new session, start a new session and set the controlling tty */
if (setsid() < 0) {
perror("setsid");
return 1;
}fd = open(argv[1], O_RDWR);
if (fd < 0) {
perror("open");
return 1;
}if (ioctl(fd, TIOCSCTTY, 1) < 0) {
perror("ioctl");
return 1;
}/* fork a child, sleep a bit and exit */
pid = fork();
if (pid < 0) {
perror("fork");
return 1;
}if (pid > 0) {
nanosleep(&ts1s, NULL);
printf("Session leader exiting\n");
exit(0);
}/*
* The child ignores SIGHUP and keeps reading from the controlling
* tty. Because SIGHUP is ignored, the child doesn't get killed on
* parent exit and the bug in n_tty makes the read(2) block the
* parent's control terminal hangup attempt. The parent ends up in
* D sleep until the child is explicitly killed.
*/
sigaction(SIGHUP, &sact, NULL);
printf("Child reading tty\n");
while (1) {
char buf[1024];if (read(fd, buf, sizeof(buf)) < 0) {
perror("read");
return 1;
}
}return 0;
}Signed-off-by: Tejun Heo
Cc: Alan Cox
Cc: stable@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman
12 Feb, 2018
1 commit
-
This is the mindless scripted replacement of kernel use of POLL*
variables as described by Al, done by this script:for V in IN OUT PRI ERR RDNORM RDBAND WRNORM WRBAND HUP RDHUP NVAL MSG; do
L=`git grep -l -w POLL$V | grep -v '^t' | grep -v /um/ | grep -v '^sa' | grep -v '/poll.h$'|grep -v '^D'`
for f in $L; do sed -i "-es/^\([^\"]*\)\(\\)/\\1E\\2/" $f; done
donewith de-mangling cleanups yet to come.
NOTE! On almost all architectures, the EPOLL* constants have the same
values as the POLL* constants do. But they keyword here is "almost".
For various bad reasons they aren't the same, and epoll() doesn't
actually work quite correctly in some cases due to this on Sparc et al.The next patch from Al will sort out the final differences, and we
should be all done.Scripted-by: Al Viro
Signed-off-by: Linus Torvalds
31 Jan, 2018
1 commit
-
Pull poll annotations from Al Viro:
"This introduces a __bitwise type for POLL### bitmap, and propagates
the annotations through the tree. Most of that stuff is as simple as
'make ->poll() instances return __poll_t and do the same to local
variables used to hold the future return value'.Some of the obvious brainos found in process are fixed (e.g. POLLIN
misspelled as POLL_IN). At that point the amount of sparse warnings is
low and most of them are for genuine bugs - e.g. ->poll() instance
deciding to return -EINVAL instead of a bitmap. I hadn't touched those
in this series - it's large enough as it is.Another problem it has caught was eventpoll() ABI mess; select.c and
eventpoll.c assumed that corresponding POLL### and EPOLL### were
equal. That's true for some, but not all of them - EPOLL### are
arch-independent, but POLL### are not.The last commit in this series separates userland POLL### values from
the (now arch-independent) kernel-side ones, converting between them
in the few places where they are copied to/from userland. AFAICS, this
is the least disruptive fix preserving poll(2) ABI and making epoll()
work on all architectures.As it is, it's simply broken on sparc - try to give it EPOLLWRNORM and
it will trigger only on what would've triggered EPOLLWRBAND on other
architectures. EPOLLWRBAND and EPOLLRDHUP, OTOH, are never triggered
at all on sparc. With this patch they should work consistently on all
architectures"* 'misc.poll' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs: (37 commits)
make kernel-side POLL... arch-independent
eventpoll: no need to mask the result of epi_item_poll() again
eventpoll: constify struct epoll_event pointers
debugging printk in sg_poll() uses %x to print POLL... bitmap
annotate poll(2) guts
9p: untangle ->poll() mess
->si_band gets POLL... bitmap stored into a user-visible long field
ring_buffer_poll_wait() return value used as return value of ->poll()
the rest of drivers/*: annotate ->poll() instances
media: annotate ->poll() instances
fs: annotate ->poll() instances
ipc, kernel, mm: annotate ->poll() instances
net: annotate ->poll() instances
apparmor: annotate ->poll() instances
tomoyo: annotate ->poll() instances
sound: annotate ->poll() instances
acpi: annotate ->poll() instances
crypto: annotate ->poll() instances
block: annotate ->poll() instances
x86: annotate ->poll() instances
...
21 Dec, 2017
1 commit
-
We added support for EXTPROC back in 2010 in commit 26df6d13406d ("tty:
Add EXTPROC support for LINEMODE") and the intent was to allow it to
override some (all?) ICANON behavior. Quoting from that original commit
message:There is a new bit in the termios local flag word, EXTPROC.
When this bit is set, several aspects of the terminal driver
are disabled. Input line editing, character echo, and mapping
of signals are all disabled. This allows the telnetd to turn
off these functions when in linemode, but still keep track of
what state the user wants the terminal to be in.but the problem turns out that "several aspects of the terminal driver
are disabled" is a bit ambiguous, and you can really confuse the n_tty
layer by setting EXTPROC and then causing some of the ICANON invariants
to no longer be maintained.This fixes at least one such case (TIOCINQ) becoming unhappy because of
the confusion over whether ICANON really means ICANON when EXTPROC is set.This basically makes TIOCINQ match the case of read: if EXTPROC is set,
we ignore ICANON. Also, make sure to reset the ICANON state ie EXTPROC
changes, not just if ICANON changes.Fixes: 26df6d13406d ("tty: Add EXTPROC support for LINEMODE")
Reported-by: Tetsuo Handa
Reported-by: syzkaller
Cc: Jiri Slaby
Signed-off-by: Linus Torvalds
Signed-off-by: Greg Kroah-Hartman
29 Nov, 2017
1 commit
-
Signed-off-by: Al Viro
08 Nov, 2017
2 commits
-
Now that the SPDX tag is in all tty files, that identifies the license
in a specific and legally-defined manner. So the extra GPL text wording
can be removed as it is no longer needed at all.This is done on a quest to remove the 700+ different ways that files in
the kernel describe the GPL license text. And there's unneeded stuff
like the address (sometimes incorrect) for the FSF which is never
needed.No copyright headers or other non-license-description text was removed.
Cc: Jiri Slaby
Cc: James Hogan
Signed-off-by: Greg Kroah-Hartman -
It's good to have SPDX identifiers in all files to make it easier to
audit the kernel tree for correct licenses.Update the drivers/tty files files with the correct SPDX license
identifier based on the license text in the file itself. The SPDX
identifier is a legally binding shorthand, which can be used instead of
the full boiler plate text.This work is based on a script and data from Thomas Gleixner, Philippe
Ombredanne, and Kate Stewart.Cc: Jiri Slaby
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: Michael Ellerman
Cc: Chris Metcalf
Cc: Jiri Kosina
Cc: David Sterba
Cc: James Hogan
Cc: Rob Herring
Cc: Eric Anholt
Cc: Stefan Wahren
Cc: Florian Fainelli
Cc: Ray Jui
Cc: Scott Branden
Cc: bcm-kernel-feedback-list@broadcom.com
Cc: "James E.J. Bottomley"
Cc: Helge Deller
Cc: Joachim Eastwood
Cc: Matthias Brugger
Cc: Masahiro Yamada
Cc: Tobias Klauser
Cc: Russell King
Cc: Vineet Gupta
Cc: Richard Genoud
Cc: Alexander Shiyan
Cc: Baruch Siach
Cc: "Maciej W. Rozycki"
Cc: "Uwe Kleine-König"
Cc: Pat Gefre
Cc: "Guilherme G. Piccoli"
Cc: Jason Wessel
Cc: Vladimir Zapolskiy
Cc: Sylvain Lemieux
Cc: Carlo Caione
Cc: Kevin Hilman
Cc: Liviu Dudau
Cc: Sudeep Holla
Cc: Lorenzo Pieralisi
Cc: Andy Gross
Cc: David Brown
Cc: "Andreas Färber"
Cc: Kevin Cernekee
Cc: Laxman Dewangan
Cc: Thierry Reding
Cc: Jonathan Hunter
Cc: Barry Song
Cc: Patrice Chotard
Cc: Maxime Coquelin
Cc: Alexandre Torgue
Cc: "David S. Miller"
Cc: Peter Korsgaard
Cc: Timur Tabi
Cc: Tony Prisk
Cc: Michal Simek
Cc: "Sören Brinkmann"
Cc: Thomas Gleixner
Cc: Kate Stewart
Cc: Philippe Ombredanne
Cc: Jiri Slaby
Signed-off-by: Greg Kroah-Hartman
02 May, 2016
1 commit
-
OpenSSH expects the (non-blocking) read() of pty master to return
EAGAIN only if it has received all of the slave-side output after
it has received SIGCHLD. This used to work on pre-3.12 kernels.This fix effectively forces non-blocking read() and poll() to
block for parallel i/o to complete for all ttys. It also unwinds
these changes:1) f8747d4a466ab2cafe56112c51b3379f9fdb7a12
tty: Fix pty master read() after slave closes2) 52bce7f8d4fc633c9a9d0646eef58ba6ae9a3b73
pty, n_tty: Simplify input processing on final close3) 1a48632ffed61352a7810ce089dc5a8bcd505a60
pty: Fix input race when closingInspired by analysis and patch from Marc Aurele La France
Reported-by: Volth
Reported-by: Marc Aurele La France
BugLink: https://bugzilla.mindrot.org/show_bug.cgi?id=52
BugLink: https://bugzilla.mindrot.org/show_bug.cgi?id=2492
Signed-off-by: Brian Bloniarz
Reviewed-by: Peter Hurley
Cc: stable
Signed-off-by: Greg Kroah-Hartman
29 Jan, 2016
6 commits
-
On final port close (and thus final tty close), only output flow
control requests in the input data should be processed. Ignore all
other input data, including parity errors, overruns and breaks.Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman -
According to fcntl(2), "a SIGIO signal is sent whenever input
or output becomes possible on that file descriptor", i.e.
after the output buffer was full and now has space for new data.
But in fact SIGIO is sent after every write.n_tty_write() should set TTY_DO_WRITE_WAKEUP only when
not all data could be written to the buffer.[pjh: Also fixes missed SIGIO if amt written just happens to be
[ amount still to writeSigned-off-by: Johannes Stezenbach
[pjh: minor patch edits and re-submit]Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman -
Since n_tty_check_unthrottle() is only called from n_tty_read()
which only originates from a userspace read(), the tty count cannot
be 0; the read() guarantees the file descriptor has not yet been
released.Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman -
If signal-driven i/o is disabled while write wakeup is pending (ie.,
n_tty_write() has set TTY_DO_WRITE_WAKEUP but then signal-driven i/o
is disabled), the TTY_DO_WRITE_WAKEUP bit will never be cleared and
will cause tty_wakeup() to always call n_tty_write_wakeup.Unconditionally clear the write wakeup, and since kill_fasync()
already checks if the fasync ptr is null, call kill_fasync()
unconditionally as well.Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman -
Only the N_TTY line discipline implements the signal-driven i/o
notification enabled/disabled by fcntl(F_SETFL, O_ASYNC). The ldisc
fasync() notification is sent to the ldisc when the enable state has
changed (the tty core is notified via the fasync() VFS file operation).The N_TTY line discipline used the enable state to change the wakeup
condition (minimum_to_wake = 1) for notifying the signal handler i/o is
available. However, just the presence of data is sufficient and necessary
to signal i/o is available, so changing minimum_to_wake is unnecessary
(and creates a race condition with read() and poll() which may be
concurrently updating minimum_to_wake).Furthermore, since the kill_fasync() VFS helper performs no action if
the fasync list is empty, calling unconditionally is preferred; if
signal driven i/o just has been disabled, no signal will be sent by
kill_fasync() anyway so notification of the change via the ldisc
fasync() method is superfluous.Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman -
A read() in non-canonical mode when VMIN > 0 and VTIME == 0 does not
complete until at least VMIN chars have been read (or the user buffer is
full). In this infrequent read mode, n_tty_read() attempts to reduce
wakeups by computing the amount of data still necessary to complete the
read (minimum_to_wake) and only waking the read()/poll() when that much
unread data has been processed. This is the only read mode for which
new data does not necessarily generate a wakeup.However, this optimization is broken and commonly leads to hung reads
even though the necessary amount of data has been received. Since the
optimization is of marginal value anyway, just remove the whole
thing. This also remedies a race between a concurrent poll() and
read() in this mode, where the poll() can reset the minimum_to_wake
of the read() (and vice versa).Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman
28 Jan, 2016
6 commits
-
In canonical read mode, each line read and logged is pushed separately
with tty_audit_push(). For all single-threaded processes and multi-threaded
processes reading from only one tty, this patch has no effect; the last line
read will still be the entry pushed to the audit log because the tty
association cannot have changed between tty_audit_add_data() and
tty_audit_push().For multi-threaded processes reading from different ttys concurrently,
the audit log will have mixed log entries anyway. Consider two ttys
audited concurrently:CPU0 CPU1
---------- ------------
tty_audit_add_data(ttyA)
tty_audit_add_data(ttyB)
tty_audit_push()
tty_audit_add_data(ttyB)
tty_audit_push()This patch will now cause the ttyB output to be split into separate
audit log entries.However, this possibility is equally likely without this patch:
CPU0 CPU1
---------- ------------
tty_audit_add_data(ttyB)
tty_audit_add_data(ttyA)
tty_audit_push()
tty_audit_add_data(ttyB)
tty_audit_push()Mixed canonical and non-canonical reads have similar races.
Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman -
The tty termios bits cannot change while n_tty_read() is in the
i/o loop; the termios_rwsem ensures mutual exclusion with termios
changes in n_tty_set_termios(). Check L_ICANON() directly and
eliminate icanon parameter.NB: tty_audit_add_data() => tty_audit_buf_get() => tty_audit_buf_alloc()
is a single path; ie., tty_audit_buf_get() and tty_audit_buf_alloc()
have no other callers.Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman -
tty audit never logs pty master reads, but packet mode only works for
pty masters, so tty_audit_add_data() was never logging packet mode
anyway.Don't audit packet mode data. As those are the lone call sites, remove
tty_put_user().Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman -
Move is_ignored() to drivers/tty/tty_io.c and re-declare in file
scope.Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman -
Reduce global tty symbols; move and rename tty_ldisc_begin() as
n_tty_init() and redefine the N_TTY ldisc ops as file scope.Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman -
The chars_in_buffer() line discipline method serves no functional
purpose, other than as a (dubious) debugging aid for mostly bit-rotting
drivers. Despite being documented as an optional method, every caller
is unconditionally executed (although conditionally compiled).
Furthermore, direct tty->ldisc access without an ldisc ref is unsafe.
Lastly, N_TTY's chars_in_buffer() has warned of removal since 3.12.Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman
27 Jan, 2016
1 commit
-
Although n_tty_check_unthrottle() has a valid ldisc reference (since
the tty core gets the ldisc ref in tty_read() before calling the line
discipline read() method), it does not have a valid ldisc reference to
the "other" pty of a pty pair. Since getting an ldisc reference for
tty->link essentially open-codes tty_wakeup(), just replace with the
equivalent tty_wakeup().Cc:
Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman
22 Dec, 2015
1 commit
-
We want the serial/tty fixes in here as well.
Signed-off-by: Greg Kroah-Hartman
14 Dec, 2015
4 commits
-
Instead of compare-and-set, just compute 'found'.
Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman -
Add a temporary for the computed source address and substitute
where appropriate. No functional change.Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman -
Merge the multiple tty_copy_to_user() calls into a single copy
sequence within tty_copy_to_user().Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman -
Since not all ttys are devices (eg., SysV ptys), dev_*() printk macros
cannot be used. Define tty_*() printk macros that output in similar
format to dev_*() macros (ie., : .....).Transform the most-trivial printk( LEVEL ...) usage to tty_*() usage.
NB: The function name has been eliminated from messages with unique
context, or prefixed to the format when given.Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman
13 Dec, 2015
1 commit
-
commit 40d5e0905a03 ("n_tty: Fix EOF push handling") fixed EOF push
for reads. However, that approach still allows a condition mismatch
between poll() and read(), where poll() returns POLLIN but read()
blocks. This state can happen when a previous read() returned because
the user buffer was full and the next character was an EOF not at the
beginning of the line. While the next read() will properly identify
the condition and advance the read buffer tail without improperly
indicating an EOF file condition (ie., read() will not mistakenly
return 0), poll() will mistakenly indicate POLLIN.Although a possible solution would be to peek at the input buffer
in n_tty_poll(), the better solution in this patch is to eat the
EOF during the previous read() (ie., fix the problem by eliminating
the condition).The current canon line buffer copy limits the scan for next end-of-line
to the smaller of either,
a. the remaining user buffer size
b. completed lines in the input buffer
When the remaining user buffer size is exactly one less than the
end-of-line marked by EOF push, the EOF is not scanned nor skipped
but left for subsequent reads. In the example below, the scan
index 'eol' has stopped at the EOF because it is past the scan
limit of 5 (not because it has found the next set bit in read_flags)user buffer [*nr = 5] _ _ _ _ _
read_flags 0 0 0 0 0 1
input buffer h e l l o [EOF]
^ ^
/ /
tail eolresult: found = 0, tail += 5, *nr += 5
Instead, allow the scan to peek ahead 1 byte (while still limiting the
scan to completed lines in the input buffer). For the example above,result: found = 1, tail += 6, *nr += 5
Because the scan limit is now bumped +1 byte, when the scan is
completed, the tail advance and the user buffer copy limit is
re-clamped to *nr when EOF is _not_ found.Fixes: 40d5e0905a03 ("n_tty: Fix EOF push handling")
Cc: # 3.12+
Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman
21 Nov, 2015
1 commit
-
The data to audit/record is in the 'from' buffer (ie., the input
read buffer).Fixes: 72586c6061ab ("n_tty: Fix auditing support for cannonical mode")
Cc: stable # 4.1+
Cc: Miloslav Trmač
Signed-off-by: Peter Hurley
Acked-by: Laura Abbott
Signed-off-by: Greg Kroah-Hartman
18 Oct, 2015
3 commits
-
Introduce API functions to restart and cancel tty buffer work, rather
than manipulate buffer work directly.Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman -
The job_control() check in n_tty_read() has nearly identical purpose
and results as tty_check_change(). Both functions' purpose is to
determine if the current task's pgrp is the foreground pgrp for the tty,
and if not, to signal the current pgrp.Introduce __tty_check_change() which takes the signal to send
and performs the shared operations for job control() and
tty_check_change().Signed-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman -
Waking the reader immediately upon receipt of TTY_BREAK or TTY_PARITY
chars has no effect on the outcome of read():
1. Only non-canonical/EXTPROC mode applies since canonical mode
will not return data until a line termination is received anyway
2. EXTPROC mode - the reader will always be woken by the input worker
3. Non-canonical modes
a. MIN == 0, TIME == 0
b. MIN == 0, TIME > 0
c. MIN > 0, TIME > 0
minimum_to_wake is always 1 in these modes so the reader will always
be woken by the input worker
d. MIN > 0, TIME == 0
although the reader will not be woken by the input worker unless the
minimum data is received, the reader would not otherwise have
returned the received dataSigned-off-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman
05 Oct, 2015
1 commit
-
My colleague ran into a program stall on a x86_64 server, where
n_tty_read() was waiting for data even if there was data in the buffer
in the pty. kernel stack for the stuck process looks like below.
#0 [ffff88303d107b58] __schedule at ffffffff815c4b20
#1 [ffff88303d107bd0] schedule at ffffffff815c513e
#2 [ffff88303d107bf0] schedule_timeout at ffffffff815c7818
#3 [ffff88303d107ca0] wait_woken at ffffffff81096bd2
#4 [ffff88303d107ce0] n_tty_read at ffffffff8136fa23
#5 [ffff88303d107dd0] tty_read at ffffffff81368013
#6 [ffff88303d107e20] __vfs_read at ffffffff811a3704
#7 [ffff88303d107ec0] vfs_read at ffffffff811a3a57
#8 [ffff88303d107f00] sys_read at ffffffff811a4306
#9 [ffff88303d107f50] entry_SYSCALL_64_fastpath at ffffffff815c86d7There seems to be two problems causing this issue.
First, in drivers/tty/n_tty.c, __receive_buf() stores the data and
updates ldata->commit_head using smp_store_release() and then checks
the wait queue using waitqueue_active(). However, since there is no
memory barrier, __receive_buf() could return without calling
wake_up_interactive_poll(), and at the same time, n_tty_read() could
start to wait in wait_woken() as in the following chart.__receive_buf() n_tty_read()
------------------------------------------------------------------------
if (waitqueue_active(&tty->read_wait))
/* Memory operations issued after the
RELEASE may be completed before the
RELEASE operation has completed */
add_wait_queue(&tty->read_wait, &wait);
...
if (!input_available_p(tty, 0)) {
smp_store_release(&ldata->commit_head,
ldata->read_head);
...
timeout = wait_woken(&wait,
TASK_INTERRUPTIBLE, timeout);
------------------------------------------------------------------------The second problem is that n_tty_read() also lacks a memory barrier
call and could also cause __receive_buf() to return without calling
wake_up_interactive_poll(), and n_tty_read() to wait in wait_woken()
as in the chart below.__receive_buf() n_tty_read()
------------------------------------------------------------------------
spin_lock_irqsave(&q->lock, flags);
/* from add_wait_queue() */
...
if (!input_available_p(tty, 0)) {
/* Memory operations issued after the
RELEASE may be completed before the
RELEASE operation has completed */
smp_store_release(&ldata->commit_head,
ldata->read_head);
if (waitqueue_active(&tty->read_wait))
__add_wait_queue(q, wait);
spin_unlock_irqrestore(&q->lock,flags);
/* from add_wait_queue() */
...
timeout = wait_woken(&wait,
TASK_INTERRUPTIBLE, timeout);
------------------------------------------------------------------------There are also other places in drivers/tty/n_tty.c which have similar
calls to waitqueue_active(), so instead of adding many memory barrier
calls, this patch simply removes the call to waitqueue_active(),
leaving just wake_up*() behind.This fixes both problems because, even though the memory access before
or after the spinlocks in both wake_up*() and add_wait_queue() can
sneak into the critical section, it cannot go past it and the critical
section assures that they will be serialized (please see "INTER-CPU
ACQUIRING BARRIER EFFECTS" in Documentation/memory-barriers.txt for a
better explanation). Moreover, the resulting code is much simpler.Latency measurement using a ping-pong test over a pty doesn't show any
visible performance drop.Signed-off-by: Kosuke Tatsukawa
Cc: stable@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman
28 Jul, 2015
1 commit
-
Other serial driver work wants to build on patches now in 4.2-rc4 so
merge the branch so this can properly happen.Signed-off-by: Greg Kroah-Hartman
24 Jul, 2015
1 commit
-
task_pgrp requires an rcu or tasklist lock to be obtained if the returned pid
is to be dereferenced, which kill_pgrp does. Obtain an RCU lock for the
duration of use.Signed-off-by: Patrick Donnelly
Reviewed-by: Peter Hurley
Signed-off-by: Greg Kroah-Hartman
