diff mbox series

pty/pty02: new test for hang involving EXTPROC|ICANON terminal mode

Message ID 20180513000101.13027-1-ebiggers3@gmail.com
State Accepted
Headers show
Series pty/pty02: new test for hang involving EXTPROC|ICANON terminal mode | expand

Commit Message

Eric Biggers May 13, 2018, 12:01 a.m. UTC
From: Eric Biggers <ebiggers@google.com>

Test for a bug in the kernel's tty layer where an infinite loop could
occur if the EXTPROC and ICANON terminal modes were combined.  This bug
was found by the syzkaller fuzzer and was fixed in v4.15.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 runtest/pty                     |  1 +
 testcases/kernel/pty/.gitignore |  1 +
 testcases/kernel/pty/pty02.c    | 58 +++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+)
 create mode 100644 testcases/kernel/pty/pty02.c

Comments

Cyril Hrubis May 18, 2018, 1:16 p.m. UTC | #1
Hi!
> +	tst_res(TINFO, "Calling FIONREAD, this will hang in n_tty_ioctl() if the bug is present...");
> +	SAFE_IOCTL(pts, FIONREAD, &nbytes);

I've added two SAFE_CLOSE() here so that the test works with the -i 1000
as well and pushed, thanks.

> +	tst_res(TPASS, "Got to the end without hanging");
> +}
Jan Stancek May 31, 2018, 1:35 p.m. UTC | #2
----- Original Message -----
> + * Regression test for commit 966031f340185 ("n_tty: fix EXTPROC vs ICANON
> + * interaction with TIOCINQ (aka FIONREAD)").  The test reproduces a hang
> + * (infinite loop in the kernel) after a pseudoterminal is put in both
> canonical
> + * (ICANON) and external processing (EXTPROC) mode, some data is written to
> the
> + * master and read from the slave, and the FIONREAD ioctl is called on the
> + * slave.  This is simplified from a syzkaller-generated reproducer.
> + */
> +
> +#include <stdlib.h>
> +#include <termio.h>
> +
> +#include "tst_test.h"
> +
> +static void do_test(void)
> +{
> +	struct termios io = { .c_lflag = EXTPROC | ICANON };

Hi,

I'm running into compilation errors on older distros (RHEL5/6)
with this test:

pty02.c: In function ‘do_test’:
pty02.c:34: error: ‘EXTPROC’ undeclared (first use in this function)
pty02.c:34: error: (Each undeclared identifier is reported only once
pty02.c:34: error: for each function it appears in.)
make: *** [pty02] Error 1

We should probably ifdef the test, because adding define to LAPI
still makes it fail:

tst_test.c:1015: INFO: Timeout per run is 0h 05m 00s
pty02.c:44: BROK: tcsetattr() failed: EINVAL

Regards,
Jan
Li Wang June 1, 2018, 2:09 a.m. UTC | #3
Hi Jan,

On Thu, May 31, 2018 at 9:35 PM, Jan Stancek <jstancek@redhat.com> wrote:

>
>
> ----- Original Message -----
> > + * Regression test for commit 966031f340185 ("n_tty: fix EXTPROC vs
> ICANON
> > + * interaction with TIOCINQ (aka FIONREAD)").  The test reproduces a
> hang
> > + * (infinite loop in the kernel) after a pseudoterminal is put in both
> > canonical
> > + * (ICANON) and external processing (EXTPROC) mode, some data is
> written to
> > the
> > + * master and read from the slave, and the FIONREAD ioctl is called on
> the
> > + * slave.  This is simplified from a syzkaller-generated reproducer.
> > + */
> > +
> > +#include <stdlib.h>
> > +#include <termio.h>
> > +
> > +#include "tst_test.h"
> > +
> > +static void do_test(void)
> > +{
> > +     struct termios io = { .c_lflag = EXTPROC | ICANON };
>
> Hi,
>
> I'm running into compilation errors on older distros (RHEL5/6)
> with this test:
>
> pty02.c: In function ‘do_test’:
> pty02.c:34: error: ‘EXTPROC’ undeclared (first use in this function)
> pty02.c:34: error: (Each undeclared identifier is reported only once
> pty02.c:34: error: for each function it appears in.)
> make: *** [pty02] Error 1
>
> We should probably ifdef the test, because adding define to LAPI
> still makes it fail:
>


​Xiao and I have also noticed this, ​beside the undefine issue, there are
still
other problems in pty02 test.

The discuss is here:
http://lists.linux.it/pipermail/ltp/2018-May/008253.html



>
> tst_test.c:1015: INFO: Timeout per run is 0h 05m 00s
> pty02.c:44: BROK: tcsetattr() failed: EINVAL
>
> Regards,
> Jan
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
Cyril Hrubis June 4, 2018, 11:40 a.m. UTC | #4
Hi!
> I'm running into compilation errors on older distros (RHEL5/6)
> with this test:
> 
> pty02.c: In function ???do_test???:
> pty02.c:34: error: ???EXTPROC??? undeclared (first use in this function)
> pty02.c:34: error: (Each undeclared identifier is reported only once
> pty02.c:34: error: for each function it appears in.)
> make: *** [pty02] Error 1
> 
> We should probably ifdef the test, because adding define to LAPI
> still makes it fail:
> 
> tst_test.c:1015: INFO: Timeout per run is 0h 05m 00s
> pty02.c:44: BROK: tcsetattr() failed: EINVAL

Or we can return TCONF on EINVAL since EINVAL means unsupported value
anyway...
Li Wang June 5, 2018, 2:44 a.m. UTC | #5
On Mon, Jun 4, 2018 at 7:40 PM, Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > I'm running into compilation errors on older distros (RHEL5/6)
> > with this test:
> >
> > pty02.c: In function ???do_test???:
> > pty02.c:34: error: ???EXTPROC??? undeclared (first use in this function)
> > pty02.c:34: error: (Each undeclared identifier is reported only once
> > pty02.c:34: error: for each function it appears in.)
> > make: *** [pty02] Error 1
> >
> > We should probably ifdef the test, because adding define to LAPI
> > still makes it fail:
> >
> > tst_test.c:1015: INFO: Timeout per run is 0h 05m 00s
> > pty02.c:44: BROK: tcsetattr() failed: EINVAL
>
> Or we can return TCONF on EINVAL since EINVAL means unsupported value
> anyway...
>

​Maybe at least we should do more try before skipping it when getting
EINVAL. Because sometimes it also fails with EINVAL when using a termios
structure that was not obtained using tcgeaatr(). tcsetattr() should use
only a termios structure that was obtained by tcgetattr().

 pty02.c works fine on mainline kernel-v4.17 after applying this:
  http://lists.linux.it/pipermail/ltp/2018-May/008253.html

What do you think?
diff mbox series

Patch

diff --git a/runtest/pty b/runtest/pty
index 92c89ab31..52e2c07f1 100644
--- a/runtest/pty
+++ b/runtest/pty
@@ -1,5 +1,6 @@ 
 #DESCRIPTION:Terminal type stress
 pty01 pty01
+pty02 pty02
 ptem01 ptem01
 hangup01 hangup01
 
diff --git a/testcases/kernel/pty/.gitignore b/testcases/kernel/pty/.gitignore
index db16401dc..131b8a077 100644
--- a/testcases/kernel/pty/.gitignore
+++ b/testcases/kernel/pty/.gitignore
@@ -1,3 +1,4 @@ 
 /hangup01
 /ptem01
 /pty01
+/pty02
diff --git a/testcases/kernel/pty/pty02.c b/testcases/kernel/pty/pty02.c
new file mode 100644
index 000000000..d96eec211
--- /dev/null
+++ b/testcases/kernel/pty/pty02.c
@@ -0,0 +1,58 @@ 
+/*
+ * Copyright (c) 2018 Google, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program, if not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * Regression test for commit 966031f340185 ("n_tty: fix EXTPROC vs ICANON
+ * interaction with TIOCINQ (aka FIONREAD)").  The test reproduces a hang
+ * (infinite loop in the kernel) after a pseudoterminal is put in both canonical
+ * (ICANON) and external processing (EXTPROC) mode, some data is written to the
+ * master and read from the slave, and the FIONREAD ioctl is called on the
+ * slave.  This is simplified from a syzkaller-generated reproducer.
+ */
+
+#include <stdlib.h>
+#include <termio.h>
+
+#include "tst_test.h"
+
+static void do_test(void)
+{
+	struct termios io = { .c_lflag = EXTPROC | ICANON };
+	int ptmx, pts;
+	char c = 'A';
+	int nbytes;
+
+	ptmx = SAFE_OPEN("/dev/ptmx", O_WRONLY);
+
+	if (tcsetattr(ptmx, TCSANOW, &io) != 0)
+		tst_brk(TBROK | TERRNO, "tcsetattr() failed");
+
+	if (unlockpt(ptmx) != 0)
+		tst_brk(TBROK | TERRNO, "unlockpt() failed");
+
+	pts = SAFE_OPEN(ptsname(ptmx), O_RDONLY);
+	SAFE_WRITE(1, ptmx, &c, 1);
+	SAFE_READ(1, pts, &c, 1);
+
+	tst_res(TINFO, "Calling FIONREAD, this will hang in n_tty_ioctl() if the bug is present...");
+	SAFE_IOCTL(pts, FIONREAD, &nbytes);
+	tst_res(TPASS, "Got to the end without hanging");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+};