[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[Richard Palethorpe]

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---

V4: Include this commit which was missing in v3

 testcases/kernel/pty/pty04.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/pty/pty04.c b/testcases/kernel/pty/pty04.c
index 0273bf902..d730d6834 100644
--- a/testcases/kernel/pty/pty04.c
+++ b/testcases/kernel/pty/pty04.c
@@ -27,6 +27,7 @@
 
 #define _GNU_SOURCE
 #include "tst_test.h"
+#include "tst_buffers.h"
 #include "config.h"
 
 #if defined(HAVE_LINUX_IF_PACKET_H) && defined(HAVE_LINUX_IF_ETHER_H)
@@ -110,7 +111,7 @@ static ssize_t try_write(int fd, char *data, ssize_t size, ssize_t *written)
 
 static void write_pty(void)
 {
-	char *data = SAFE_MALLOC(mtu);
+	char *data = tst_alloc(mtu);
 	ssize_t written, ret;
 
 	memset(data, '_', mtu - 1);
@@ -137,7 +138,7 @@ static void write_pty(void)
 
 	tst_res(TPASS, "Writing to PTY interrupted by hangup");
 
-	free(data);
+	tst_free_all();
 }
 
 static void open_netdev(struct ldisc_info *ldisc)
@@ -203,7 +204,7 @@ static void check_data(const char *data, ssize_t len)
 static void read_netdev(void)
 {
 	int rlen, plen = mtu - 1;
-	char *data = SAFE_MALLOC(plen);
+	char *data = tst_alloc(plen);
 
 	tst_res(TINFO, "Reading from socket %d", sk);
 
@@ -222,7 +223,7 @@ static void read_netdev(void)
 
 	tst_res(TPASS, "Reading data from netdev interrupted by hangup");
 
-	free(data);
+	tst_free_all();
 }
 
 static void do_test(unsigned int n)
-- 
2.26.1

[LTP] [PATCH v4 2/2] pty04: Add SLCAN ldisc and check for CVE-2020-11494

[Richard Palethorpe]

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 testcases/kernel/pty/pty04.c | 156 ++++++++++++++++++++++++++++-------
 1 file changed, 126 insertions(+), 30 deletions(-)

diff --git a/testcases/kernel/pty/pty04.c b/testcases/kernel/pty/pty04.c
index d730d6834..eaf172504 100644
--- a/testcases/kernel/pty/pty04.c
+++ b/testcases/kernel/pty/pty04.c
@@ -7,6 +7,9 @@
  * data is in flight to try to cause a race between the netdev being deleted
  * and the discipline receive function writing to the netdev.
  *
+ * For SLCAN we check stack data is not leaked in the frame padding
+ * (CVE-2020-11494).
+ *
  * Test flow:
  * 1. Create PTY with ldisc X which creates netdev Y
  * 2. Open raw packet socket and bind to netdev Y
@@ -23,6 +26,11 @@
  * until we are in the netdev code which can have a bigger buffer. Of course
  * the MTU still decides exactly where the packet delimiter goes, this just
  * concerns choosing the best packet size to cause a race.
+ *
+ * Note on line discipline encapsulation formats:
+ * - For SLIP frames we just write the data followed by a delimiter char
+ * - SLCAN we write some ASCII described in drivers/net/can/slcan.c which is
+ *   converted to the actual frame by the kernel
  */
 
 #define _GNU_SOURCE
@@ -36,6 +44,25 @@
 #include <linux/if_ether.h>
 #include <linux/tty.h>
 
+#ifdef HAVE_LINUX_CAN_H
+# include <linux/can.h>
+#else
+# define CAN_MTU 16
+# define CAN_MAX_DLEN 8
+
+typedef uint32_t canid_t;
+
+struct can_frame {
+	canid_t can_id;
+	uint32_t can_dlc;
+	uint32_t __pad;
+	uint32_t __res0;
+	uint32_t __res1;
+	uint32_t data[CAN_MAX_DLEN] __attribute__((aligned(8)));
+};
+#endif
+
+#include <stddef.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <errno.h>
@@ -48,19 +75,23 @@
 
 #include "tst_safe_stdio.h"
 
+#define str(s) #s
+#define SLCAN_FRAME "t00185f5f5f5f5f5f5f5f\r"
+
 struct ldisc_info {
 	int n;
 	char *name;
-	int max_mtu;
+	int mtu;
 };
 
 static struct ldisc_info ldiscs[] = {
 	{N_SLIP, "N_SLIP", 8192},
+	{N_SLCAN, "N_SLCAN", CAN_MTU},
 };
 
 static volatile int ptmx, pts, sk, mtu, no_check;
 
-static int set_ldisc(int tty, struct ldisc_info *ldisc)
+static int set_ldisc(int tty, const struct ldisc_info *ldisc)
 {
 	TEST(ioctl(tty, TIOCSETD, &ldisc->n));
 
@@ -79,7 +110,7 @@ static int set_ldisc(int tty, struct ldisc_info *ldisc)
 	return 1;
 }
 
-static int open_pty(struct ldisc_info *ldisc)
+static int open_pty(const struct ldisc_info *ldisc)
 {
 	char pts_path[PATH_MAX];
 
@@ -99,7 +130,8 @@ static int open_pty(struct ldisc_info *ldisc)
 	return set_ldisc(pts, ldisc);
 }
 
-static ssize_t try_write(int fd, char *data, ssize_t size, ssize_t *written)
+static ssize_t try_write(int fd, const char *data,
+			 ssize_t size, ssize_t *written)
 {
 	ssize_t ret = write(fd, data, size);
 
@@ -109,22 +141,42 @@ static ssize_t try_write(int fd, char *data, ssize_t size, ssize_t *written)
 	return !written || (*written += ret) >= size;
 }
 
-static void write_pty(void)
+static void write_pty(const struct ldisc_info *ldisc)
 {
-	char *data = tst_alloc(mtu);
+	char *data;
 	ssize_t written, ret;
+	size_t len = 0;
+
+	switch (ldisc->n) {
+	case N_SLIP:
+		len = mtu;
+		break;
+	case N_SLCAN:
+		len = sizeof(SLCAN_FRAME) - 1;
+		break;
+	}
+
+	data = tst_alloc(len);
+
+	switch (ldisc->n) {
+	case N_SLIP:
+		memset(data, '_', len - 1);
+		data[len - 1] = 0300;
+		break;
+	case N_SLCAN:
+		memcpy(data, SLCAN_FRAME, len);
+		break;
+	}
 
-	memset(data, '_', mtu - 1);
-	data[mtu - 1] = 0300;
 
 	written = 0;
-	ret = TST_RETRY_FUNC(try_write(ptmx, data, mtu, &written), TST_RETVAL_NOTNULL);
+	ret = TST_RETRY_FUNC(try_write(ptmx, data, len, &written), TST_RETVAL_NOTNULL);
 	if (ret < 0)
 		tst_brk(TBROK | TERRNO, "Failed 1st write to PTY");
 	tst_res(TPASS, "Wrote PTY 1");
 
 	written = 0;
-	ret = TST_RETRY_FUNC(try_write(ptmx, data, mtu, &written), TST_RETVAL_NOTNULL);
+	ret = TST_RETRY_FUNC(try_write(ptmx, data, len, &written), TST_RETVAL_NOTNULL);
 	if (ret < 0)
 		tst_brk(TBROK | TERRNO, "Failed 2nd write to PTY");
 
@@ -133,7 +185,7 @@ static void write_pty(void)
 
 	tst_res(TPASS, "Wrote PTY 2");
 
-	while (try_write(ptmx, data, mtu, NULL) >= 0)
+	while (try_write(ptmx, data, len, NULL) >= 0)
 		;
 
 	tst_res(TPASS, "Writing to PTY interrupted by hangup");
@@ -141,7 +193,7 @@ static void write_pty(void)
 	tst_free_all();
 }
 
-static void open_netdev(struct ldisc_info *ldisc)
+static void open_netdev(const struct ldisc_info *ldisc)
 {
 	struct ifreq ifreq = { 0 };
 	struct sockaddr_ll lla = { 0 };
@@ -151,12 +203,12 @@ static void open_netdev(struct ldisc_info *ldisc)
 
 	sk = SAFE_SOCKET(PF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
 
-	ifreq.ifr_mtu = ldisc->max_mtu;
+	ifreq.ifr_mtu = ldisc->mtu;
 	if (ioctl(sk, SIOCSIFMTU, &ifreq))
 		tst_res(TWARN | TERRNO, "Failed to set netdev MTU to maximum");
 	SAFE_IOCTL(sk, SIOCGIFMTU, &ifreq);
 	mtu = ifreq.ifr_mtu;
-	tst_res(TINFO, "Netdev MTU is %d (we set %d)", mtu, ldisc->max_mtu);
+	tst_res(TINFO, "Netdev MTU is %d (we set %d)", mtu, ldisc->mtu);
 
 	SAFE_IOCTL(sk, SIOCGIFFLAGS, &ifreq);
 	ifreq.ifr_flags |= IFF_UP | IFF_RUNNING;
@@ -176,13 +228,41 @@ static void open_netdev(struct ldisc_info *ldisc)
 	tst_res(TINFO, "Bound netdev %d to socket %d", ifreq.ifr_ifindex, sk);
 }
 
-static void check_data(const char *data, ssize_t len)
+static void check_data(const struct ldisc_info *ldisc,
+		       const char *data, ssize_t len)
 {
 	ssize_t i = 0, j;
+	struct can_frame frm;
 
 	if (no_check)
 		return;
 
+	if (ldisc->n == N_SLCAN) {
+		memcpy(&frm, data, len);
+
+		if (frm.can_id != 1) {
+			tst_res(TFAIL, "can_id = %d != 1",
+				frm.can_id);
+			no_check = 1;
+		}
+
+		if (frm.can_dlc != CAN_MAX_DLEN) {
+			tst_res(TFAIL, "can_dlc = %d != " str(CAN_MAX_DLEN),
+				frm.can_dlc);
+			no_check = 1;
+		}
+
+		i = offsetof(struct can_frame, __pad);
+		if (frm.__pad != frm.__res0 || frm.__res0 != frm.__res1) {
+			tst_res_hexd(TFAIL, data + i,
+				     offsetof(struct can_frame, data) - i,
+				     "Padding bytes may contain stack data");
+			no_check = 1;
+		}
+
+		i = offsetof(struct can_frame, data);
+	}
+
 	do {
 		if (i >= len)
 			return;
@@ -195,31 +275,42 @@ static void check_data(const char *data, ssize_t len)
 	j--;
 
 	tst_res_hexd(TFAIL, data + i, j - i,
-		     "Corrupt data (max 64 bytes shown): data[%ld..%ld] = ",
-		     i, j);
-	tst_res(TINFO, "Will continue test without data checking");
+		     "Corrupt data (max 64 of %ld bytes shown): data[%ld..%ld] = ",
+		     len, i, j);
 	no_check = 1;
+
+	if (no_check)
+		tst_res(TINFO, "Will continue test without data checking");
 }
 
-static void read_netdev(void)
+static void read_netdev(const struct ldisc_info *ldisc)
 {
-	int rlen, plen = mtu - 1;
-	char *data = tst_alloc(plen);
+	int rlen, plen = 0;
+	char *data;
+
+	switch (ldisc->n) {
+	case N_SLIP:
+		plen = mtu - 1;
+		break;
+	case N_SLCAN:
+		plen = CAN_MTU;
+		break;
+	}
+	data = tst_alloc(plen);
 
 	tst_res(TINFO, "Reading from socket %d", sk);
 
 	SAFE_READ(1, sk, data, plen);
-	check_data(data, plen);
-
+	check_data(ldisc, data, plen);
 	tst_res(TPASS, "Read netdev 1");
-	SAFE_READ(1, sk, data, plen);
-	check_data(data, plen);
 
+	SAFE_READ(1, sk, data, plen);
+	check_data(ldisc, data, plen);
 	tst_res(TPASS, "Read netdev 2");
 
 	TST_CHECKPOINT_WAKE(0);
 	while((rlen = read(sk, data, plen)) > 0)
-		check_data(data, rlen);
+		check_data(ldisc, data, rlen);
 
 	tst_res(TPASS, "Reading data from netdev interrupted by hangup");
 
@@ -236,12 +327,12 @@ static void do_test(unsigned int n)
 	open_netdev(ldisc);
 
 	if (!SAFE_FORK()) {
-		read_netdev();
+		read_netdev(ldisc);
 		return;
 	}
 
 	if (!SAFE_FORK()) {
-		write_pty();
+		write_pty(ldisc);
 		return;
 	}
 
@@ -268,11 +359,16 @@ static void cleanup(void)
 static struct tst_test test = {
 	.test = do_test,
 	.cleanup = cleanup,
-	.tcnt = 1,
+	.tcnt = 2,
 	.forks_child = 1,
 	.needs_checkpoints = 1,
 	.needs_root = 1,
-	.min_kver = "4.10"
+	.min_kver = "4.10",
+	.tags = (const struct tst_tag[]){
+		{"linux-git", "b9258a2cece4ec1f020715fe3554bc2e360f6264"},
+		{"CVE", "CVE-2020-11494"},
+		{}
+	}
 };
 
 #else
-- 
2.26.1

[LTP] [PATCH v4 2/2] pty04: Add SLCAN ldisc and check for CVE-2020-11494

[Cyril Hrubis]

Hi!
Both pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[Petr Vorel]

Hi Richard,

> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---

Reviewed-by: Petr Vorel <pvorel@suse.cz>

BTW Every second run with this patch it blocks after pty04.c:214: PASS: Read netdev 1
and then:
tst_checkpoint.c:147: BROK: pty04.c:249: tst_checkpoint_wait(0, 10000): ETIMEDOUT (110)
tst_test.c:373: BROK: Reported by child (26650)
safe_macros.c:258: BROK: pty04.c:215: read(5,0x7efebc306001,8191) failed, returned -1: ENETDOWN (100)
pty04.c:139: PASS: Writing to PTY interrupted by hangup
tst_test.c:373: WARN: Reported by child (26648)

Tested on 5.7.0-rc3 in Tumbleweed.
But it looks this is not caused by this change, but was here before, because the
same behavior I see when testing pty04 *without* this patch on various kernels
(5.3.7, 5.6.0-rc5) and some of the never SLES (4.12 based).

Kind regards,
Petr

[LTP] [PATCH v4 2/2] pty04: Add SLCAN ldisc and check for CVE-2020-11494

[Petr Vorel]

Hi Cyril, Richard,

> Hi!
> Both pushed, thanks.

This test does not compile on kernel older than v4.2-rc1 (2015),
because __pad, __res0 and __res1 are missing. They were added in
a2f11835994e ("can.h: make padding given by gcc explicit").

This got propagated on CentOS 6. I'd be for dropping it after release (most of
the problems are due old gcc 4), but v4.1 is still too new, so I'll send some
autotools based fix for this.

Kind regards,
Petr

[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[Richard Palethorpe]

Hello,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Richard,
>
>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>> ---
>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>
> BTW Every second run with this patch it blocks after pty04.c:214: PASS: Read netdev 1
> and then:
> tst_checkpoint.c:147: BROK: pty04.c:249: tst_checkpoint_wait(0, 10000): ETIMEDOUT (110)
> tst_test.c:373: BROK: Reported by child (26650)
> safe_macros.c:258: BROK: pty04.c:215: read(5,0x7efebc306001,8191) failed, returned -1: ENETDOWN (100)
> pty04.c:139: PASS: Writing to PTY interrupted by hangup
> tst_test.c:373: WARN: Reported by child (26648)
>
> Tested on 5.7.0-rc3 in Tumbleweed.
> But it looks this is not caused by this change, but was here before, because the
> same behavior I see when testing pty04 *without* this patch on various kernels
> (5.3.7, 5.6.0-rc5) and some of the never SLES (4.12 based).
>
> Kind regards,
> Petr

This looks similar to the issue reported by Jan:

https://github.com/linux-test-project/ltp/issues/674

Is this the full output?

Thinking aloud: the following (probably) happens when writing to the PTY

write() -> PTY -> SLIP/SLCAN -> netdev -> read()

Writing to the PTY causes the PTY to write to the line discipline. What
I found was that when the line discipline receive buffer got full and the PTY
send buffer got full. The write would go to sleep and never wake up
because the line discipline drained the receive buffer, but doesn't
signal it is ready for more data (with tty_unthrottle). So I used
nonblocking writes which just retry writing.

From Jan's errors it looks like it might just be reading that is failing
in one case and that writing is also failing in the other until we
cancel the read. I doubt this is anything to do with the netdev code
because it is generic networking code AFAICT and should work correctly
with blocking reads...

-- 
Thank you,
Richard.

[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[Petr Vorel]

Hi Richard,

> This looks similar to the issue reported by Jan:

> https://github.com/linux-test-project/ltp/issues/674

> Is this the full output?
Yes, it's exactly the same issue. Thanks for info, I overlooked this bug.

> Thinking aloud: the following (probably) happens when writing to the PTY

> write() -> PTY -> SLIP/SLCAN -> netdev -> read()

> Writing to the PTY causes the PTY to write to the line discipline. What
> I found was that when the line discipline receive buffer got full and the PTY
> send buffer got full. The write would go to sleep and never wake up
> because the line discipline drained the receive buffer, but doesn't
> signal it is ready for more data (with tty_unthrottle). So I used
> nonblocking writes which just retry writing.

> From Jan's errors it looks like it might just be reading that is failing
> in one case and that writing is also failing in the other until we
> cancel the read. I doubt this is anything to do with the netdev code
> because it is generic networking code AFAICT and should work correctly
> with blocking reads...
Hm, I'm not familiar with the code, but also thing it's not netdev related.

Kind regards,
Petr

[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[Petr Vorel]

Hi Richard,

> This looks similar to the issue reported by Jan:

> https://github.com/linux-test-project/ltp/issues/674

> Is this the full output?

Here it is:

# ./pty04
tst_test.c:1244: INFO: Timeout per run is 0h 05m 00s
pty04.c:127: INFO: PTS path is /dev/pts/29
pty04.c:202: INFO: Netdev is sl0
pty04.c:211: INFO: Netdev MTU is 8192 (we set 8192)
pty04.c:228: INFO: Bound netdev 51 to socket 5
tst_buffers.c:55: INFO: Test is using guarded buffers
pty04.c:301: INFO: Reading from socket 5
tst_buffers.c:55: INFO: Test is using guarded buffers
pty04.c:176: PASS: Wrote PTY 1
pty04.c:186: PASS: Wrote PTY 2
pty04.c:305: PASS: Read netdev 1
pty04.c:309: PASS: Read netdev 2
pty04.c:315: PASS: Reading data from netdev interrupted by hangup
pty04.c:342: INFO: Sent hangup ioctl to PTS
pty04.c:191: PASS: Writing to PTY interrupted by hangup
pty04.c:344: INFO: Sent hangup ioctl to PTM
pty04.c:127: INFO: PTS path is /dev/pts/32
pty04.c:202: INFO: Netdev is slcan0
pty04.c:211: INFO: Netdev MTU is 16 (we set 16)
pty04.c:228: INFO: Bound netdev 52 to socket 8
tst_buffers.c:55: INFO: Test is using guarded buffers
pty04.c:301: INFO: Reading from socket 8
tst_buffers.c:55: INFO: Test is using guarded buffers
pty04.c:176: PASS: Wrote PTY 1
pty04.c:186: PASS: Wrote PTY 2
pty04.c:305: PASS: Read netdev 1
pty04.c:309: PASS: Read netdev 2
pty04.c:315: PASS: Reading data from netdev interrupted by hangup
pty04.c:342: INFO: Sent hangup ioctl to PTS
pty04.c:191: PASS: Writing to PTY interrupted by hangup
pty04.c:344: INFO: Sent hangup ioctl to PTM

Summary:
passed   12
failed   0
skipped  0
warnings 0

# ./pty04
tst_test.c:1244: INFO: Timeout per run is 0h 05m 00s
pty04.c:127: INFO: PTS path is /dev/pts/29
pty04.c:202: INFO: Netdev is sl0
pty04.c:211: INFO: Netdev MTU is 8192 (we set 8192)
pty04.c:228: INFO: Bound netdev 53 to socket 5
tst_buffers.c:55: INFO: Test is using guarded buffers
pty04.c:301: INFO: Reading from socket 5
tst_buffers.c:55: INFO: Test is using guarded buffers
pty04.c:176: PASS: Wrote PTY 1
pty04.c:186: PASS: Wrote PTY 2
pty04.c:305: PASS: Read netdev 1
# BLOCKS
tst_checkpoint.c:147: BROK: pty04.c:340: tst_checkpoint_wait(0, 10000): ETIMEDOUT (110)
tst_test.c:373: BROK: Reported by child (10018)
safe_macros.c:258: BROK: pty04.c:307: read(5,0x7f3920c4a001,8191) failed, returned -1: ENETDOWN (100)
pty04.c:191: PASS: Writing to PTY interrupted by hangup
tst_test.c:373: WARN: Reported by child (10016)

Summary:
passed   4
failed   0
skipped  0
warnings 1

It looked like it regularly changes working and failing, but sometimes it fails
several times. Also sometimes it works when -i2 (but mostly fails).

Kind regards,
Petr

[LTP] [PATCH v4 2/2] pty04: Add SLCAN ldisc and check for CVE-2020-11494

[Richard Palethorpe]

Hello,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Cyril, Richard,
>
>> Hi!
>> Both pushed, thanks.
>
> This test does not compile on kernel older than v4.2-rc1 (2015),
> because __pad, __res0 and __res1 are missing. They were added in
> a2f11835994e ("can.h: make padding given by gcc explicit").

I suppose I can just drop the header and define it inline as we need the
fallback anyway.

>
> This got propagated on CentOS 6. I'd be for dropping it after release (most of
> the problems are due old gcc 4), but v4.1 is still too new, so I'll send some
> autotools based fix for this.
>
> Kind regards,
> Petr

-- 
Thank you,
Richard.

[LTP] [PATCH v4 2/2] pty04: Add SLCAN ldisc and check for CVE-2020-11494

[Petr Vorel]

Hi Richard,

> >> Hi!
> >> Both pushed, thanks.

> > This test does not compile on kernel older than v4.2-rc1 (2015),
> > because __pad, __res0 and __res1 are missing. They were added in
> > a2f11835994e ("can.h: make padding given by gcc explicit").

> I suppose I can just drop the header and define it inline as we need the
> fallback anyway.
+1. Thanks!

Kind regards,
Petr

[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[Richard Palethorpe]

Hi,

Richard Palethorpe <rpalethorpe@suse.de> writes:

> Hello,
>
> Petr Vorel <pvorel@suse.cz> writes:
>
>> Hi Richard,
>>
>>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>>> ---
>>
>> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>>
>> BTW Every second run with this patch it blocks after pty04.c:214: PASS: Read netdev 1
>> and then:
>> tst_checkpoint.c:147: BROK: pty04.c:249: tst_checkpoint_wait(0, 10000): ETIMEDOUT (110)
>> tst_test.c:373: BROK: Reported by child (26650)
>> safe_macros.c:258: BROK: pty04.c:215: read(5,0x7efebc306001,8191) failed, returned -1: ENETDOWN (100)
>> pty04.c:139: PASS: Writing to PTY interrupted by hangup
>> tst_test.c:373: WARN: Reported by child (26648)
>>
>> Tested on 5.7.0-rc3 in Tumbleweed.
>> But it looks this is not caused by this change, but was here before, because the
>> same behavior I see when testing pty04 *without* this patch on various kernels
>> (5.3.7, 5.6.0-rc5) and some of the never SLES (4.12 based).
>>
>> Kind regards,
>> Petr
>
> This looks similar to the issue reported by Jan:
>
> https://github.com/linux-test-project/ltp/issues/674
>
> Is this the full output?
>
> Thinking aloud: the following (probably) happens when writing to the PTY
>
> write() -> PTY -> SLIP/SLCAN -> netdev -> read()
>
> Writing to the PTY causes the PTY to write to the line discipline. What
> I found was that when the line discipline receive buffer got full and the PTY
> send buffer got full. The write would go to sleep and never wake up
> because the line discipline drained the receive buffer, but doesn't
> signal it is ready for more data (with tty_unthrottle). So I used
> nonblocking writes which just retry writing.
>
> From Jan's errors it looks like it might just be reading that is failing
> in one case and that writing is also failing in the other until we
> cancel the read. I doubt this is anything to do with the netdev code
> because it is generic networking code AFAICT and should work correctly
> with blocking reads...

Probably the best thing todo for now is to remove the test before the
release as this requires some more investigation.


-- 
Thank you,
Richard.

[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[Jan Stancek]

----- Original Message -----
> Hi,
> 
> Richard Palethorpe <rpalethorpe@suse.de> writes:
> 
> > Hello,
> >
> > Petr Vorel <pvorel@suse.cz> writes:
> >
> >> Hi Richard,
> >>
> >>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> >>> ---
> >>
> >> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> >>
> >> BTW Every second run with this patch it blocks after pty04.c:214: PASS:
> >> Read netdev 1
> >> and then:
> >> tst_checkpoint.c:147: BROK: pty04.c:249: tst_checkpoint_wait(0, 10000):
> >> ETIMEDOUT (110)
> >> tst_test.c:373: BROK: Reported by child (26650)
> >> safe_macros.c:258: BROK: pty04.c:215: read(5,0x7efebc306001,8191) failed,
> >> returned -1: ENETDOWN (100)
> >> pty04.c:139: PASS: Writing to PTY interrupted by hangup
> >> tst_test.c:373: WARN: Reported by child (26648)
> >>
> >> Tested on 5.7.0-rc3 in Tumbleweed.
> >> But it looks this is not caused by this change, but was here before,
> >> because the
> >> same behavior I see when testing pty04 *without* this patch on various
> >> kernels
> >> (5.3.7, 5.6.0-rc5) and some of the never SLES (4.12 based).
> >>
> >> Kind regards,
> >> Petr
> >
> > This looks similar to the issue reported by Jan:
> >
> > https://github.com/linux-test-project/ltp/issues/674
> >
> > Is this the full output?
> >
> > Thinking aloud: the following (probably) happens when writing to the PTY
> >
> > write() -> PTY -> SLIP/SLCAN -> netdev -> read()
> >
> > Writing to the PTY causes the PTY to write to the line discipline. What
> > I found was that when the line discipline receive buffer got full and the
> > PTY
> > send buffer got full. The write would go to sleep and never wake up
> > because the line discipline drained the receive buffer, but doesn't
> > signal it is ready for more data (with tty_unthrottle). So I used
> > nonblocking writes which just retry writing.
> >
> > From Jan's errors it looks like it might just be reading that is failing
> > in one case and that writing is also failing in the other until we
> > cancel the read. I doubt this is anything to do with the netdev code
> > because it is generic networking code AFAICT and should work correctly
> > with blocking reads...
> 
> Probably the best thing todo for now is to remove the test before the
> release as this requires some more investigation.

We can keep it in tree, I'd just disable it in runtest file(s), so it's not
run by default.

[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[Li Wang]

On Wed, May 6, 2020 at 6:50 PM Jan Stancek <jstancek@redhat.com> wrote:

>
>
> ----- Original Message -----
> > Hi,
> >
> > Richard Palethorpe <rpalethorpe@suse.de> writes:
> >
> > > Hello,
> > >
> > > Petr Vorel <pvorel@suse.cz> writes:
> > >
> > >> Hi Richard,
> > >>
> > >>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> > >>> ---
> > >>
> > >> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> > >>
> > >> BTW Every second run with this patch it blocks after pty04.c:214:
> PASS:
> > >> Read netdev 1
> > >> and then:
> > >> tst_checkpoint.c:147: BROK: pty04.c:249: tst_checkpoint_wait(0,
> 10000):
> > >> ETIMEDOUT (110)
> > >> tst_test.c:373: BROK: Reported by child (26650)
> > >> safe_macros.c:258: BROK: pty04.c:215: read(5,0x7efebc306001,8191)
> failed,
> > >> returned -1: ENETDOWN (100)
> > >> pty04.c:139: PASS: Writing to PTY interrupted by hangup
> > >> tst_test.c:373: WARN: Reported by child (26648)
> > >>
> > >> Tested on 5.7.0-rc3 in Tumbleweed.
> > >> But it looks this is not caused by this change, but was here before,
> > >> because the
> > >> same behavior I see when testing pty04 *without* this patch on various
> > >> kernels
> > >> (5.3.7, 5.6.0-rc5) and some of the never SLES (4.12 based).
> > >>
> > >> Kind regards,
> > >> Petr
> > >
> > > This looks similar to the issue reported by Jan:
> > >
> > > https://github.com/linux-test-project/ltp/issues/674
> > >
> > > Is this the full output?
> > >
> > > Thinking aloud: the following (probably) happens when writing to the
> PTY
> > >
> > > write() -> PTY -> SLIP/SLCAN -> netdev -> read()
> > >
> > > Writing to the PTY causes the PTY to write to the line discipline. What
> > > I found was that when the line discipline receive buffer got full and
> the
> > > PTY
> > > send buffer got full. The write would go to sleep and never wake up
> > > because the line discipline drained the receive buffer, but doesn't
> > > signal it is ready for more data (with tty_unthrottle). So I used
> > > nonblocking writes which just retry writing.
> > >
> > > From Jan's errors it looks like it might just be reading that is
> failing
> > > in one case and that writing is also failing in the other until we
> > > cancel the read. I doubt this is anything to do with the netdev code
> > > because it is generic networking code AFAICT and should work correctly
> > > with blocking reads...
> >
> > Probably the best thing todo for now is to remove the test before the
> > release as this requires some more investigation.
>
> We can keep it in tree, I'd just disable it in runtest file(s), so it's not
> run by default.
>

But we still facing the compiled errors in the old kernels, that will break
the LTP build in the compiling phase.

RHEL-7:

pty04.c: In function ?check_data?:
pty04.c:255:7: error: ?struct can_frame? has no member named ?__pad?
   i = offsetof(struct can_frame, __pad);
       ^
pty04.c:256:10: error: ?struct can_frame? has no member named ?__pad?
   if (frm.__pad != frm.__res0 || frm.__res0 != frm.__res1) {
          ^
pty04.c:256:23: error: ?struct can_frame? has no member named ?__res0?
   if (frm.__pad != frm.__res0 || frm.__res0 != frm.__res1) {
                       ^
pty04.c:256:37: error: ?struct can_frame? has no member named ?__res0?
   if (frm.__pad != frm.__res0 || frm.__res0 != frm.__res1) {
                                     ^
pty04.c:256:51: error: ?struct can_frame? has no member named ?__res1?
   if (frm.__pad != frm.__res0 || frm.__res0 != frm.__res1) {


-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200506/8066fcb2/attachment.htm>

[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[Jan Stancek]

----- Original Message -----
> > > Probably the best thing todo for now is to remove the test before the
> > > release as this requires some more investigation.
> >
> > We can keep it in tree, I'd just disable it in runtest file(s), so it's not
> > run by default.
> >
> 
> But we still facing the compiled errors in the old kernels, that will break
> the LTP build in the compiling phase.

I see, that will fail on anything older than 4.10. I don't have strong
preference how to deal with that, just want to avoid running pty04 for now.

diff --git a/testcases/kernel/pty/pty04.c b/testcases/kernel/pty/pty04.c
index eaf172504a64..55923a0af006 100644
--- a/testcases/kernel/pty/pty04.c
+++ b/testcases/kernel/pty/pty04.c
@@ -38,7 +38,10 @@
 #include "tst_buffers.h"
 #include "config.h"

-#if defined(HAVE_LINUX_IF_PACKET_H) && defined(HAVE_LINUX_IF_ETHER_H)
+#include <linux/version.h>
+
+#if defined(HAVE_LINUX_IF_PACKET_H) && defined(HAVE_LINUX_IF_ETHER_H) \
+       && LINUX_VERSION_CODE >= KERNEL_VERSION(4, 10, 0)

 #include <linux/if_packet.h>
 #include <linux/if_ether.h>
@@ -373,6 +376,6 @@ static struct tst_test test = {

 #else

-TST_TEST_TCONF("Need <linux/if_packet.h> and <linux/if_ether.h>");
+TST_TEST_TCONF("Need <linux/if_packet.h> and <linux/if_ether.h> and 4.10+");

[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[Petr Vorel]

Hi Jan,

> > > > Probably the best thing todo for now is to remove the test before the
> > > > release as this requires some more investigation.

> > > We can keep it in tree, I'd just disable it in runtest file(s), so it's not
> > > run by default.


> > But we still facing the compiled errors in the old kernels, that will break
> > the LTP build in the compiling phase.

> I see, that will fail on anything older than 4.10. I don't have strong
> preference how to deal with that, just want to avoid running pty04 for now.

> diff --git a/testcases/kernel/pty/pty04.c b/testcases/kernel/pty/pty04.c
> index eaf172504a64..55923a0af006 100644
> --- a/testcases/kernel/pty/pty04.c
> +++ b/testcases/kernel/pty/pty04.c
> @@ -38,7 +38,10 @@
>  #include "tst_buffers.h"
>  #include "config.h"

> -#if defined(HAVE_LINUX_IF_PACKET_H) && defined(HAVE_LINUX_IF_ETHER_H)
> +#include <linux/version.h>
Shouldn't we also check for <linux/version.h> with autotools? Or are kernel
headers mandatory for LTP build?

> +
> +#if defined(HAVE_LINUX_IF_PACKET_H) && defined(HAVE_LINUX_IF_ETHER_H) \
> +       && LINUX_VERSION_CODE >= KERNEL_VERSION(4, 10, 0)

>  #include <linux/if_packet.h>
>  #include <linux/if_ether.h>
> @@ -373,6 +376,6 @@ static struct tst_test test = {

>  #else

> -TST_TEST_TCONF("Need <linux/if_packet.h> and <linux/if_ether.h>");
> +TST_TEST_TCONF("Need <linux/if_packet.h> and <linux/if_ether.h> and 4.10+");

+1.

BTW, why this tests requires 4.10 for runtime?


Kind regards,
Petr

[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[Jan Stancek]

----- Original Message -----
> Hi Jan,
> 
> > > > > Probably the best thing todo for now is to remove the test before the
> > > > > release as this requires some more investigation.
> 
> > > > We can keep it in tree, I'd just disable it in runtest file(s), so it's
> > > > not
> > > > run by default.
> 
> 
> > > But we still facing the compiled errors in the old kernels, that will
> > > break
> > > the LTP build in the compiling phase.
> 
> > I see, that will fail on anything older than 4.10. I don't have strong
> > preference how to deal with that, just want to avoid running pty04 for now.
> 
> > diff --git a/testcases/kernel/pty/pty04.c b/testcases/kernel/pty/pty04.c
> > index eaf172504a64..55923a0af006 100644
> > --- a/testcases/kernel/pty/pty04.c
> > +++ b/testcases/kernel/pty/pty04.c
> > @@ -38,7 +38,10 @@
> >  #include "tst_buffers.h"
> >  #include "config.h"
> 
> > -#if defined(HAVE_LINUX_IF_PACKET_H) && defined(HAVE_LINUX_IF_ETHER_H)
> > +#include <linux/version.h>
> Shouldn't we also check for <linux/version.h> with autotools? Or are kernel
> headers mandatory for LTP build?

We already include this header in some syscalls tests, so I assumed it won't be an issue.

> 
> > +
> > +#if defined(HAVE_LINUX_IF_PACKET_H) && defined(HAVE_LINUX_IF_ETHER_H) \
> > +       && LINUX_VERSION_CODE >= KERNEL_VERSION(4, 10, 0)
> 
> >  #include <linux/if_packet.h>
> >  #include <linux/if_ether.h>
> > @@ -373,6 +376,6 @@ static struct tst_test test = {
> 
> >  #else
> 
> > -TST_TEST_TCONF("Need <linux/if_packet.h> and <linux/if_ether.h>");
> > +TST_TEST_TCONF("Need <linux/if_packet.h> and <linux/if_ether.h> and
> > 4.10+");
> 
> +1.
> 
> BTW, why this tests requires 4.10 for runtime?

It's for compilation. struct can_frame fields it's using were introduced in 4.10.

[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[Petr Vorel]

Hi Jan,

> > > -#if defined(HAVE_LINUX_IF_PACKET_H) && defined(HAVE_LINUX_IF_ETHER_H)
> > > +#include <linux/version.h>
> > Shouldn't we also check for <linux/version.h> with autotools? Or are kernel
> > headers mandatory for LTP build?

> We already include this header in some syscalls tests, so I assumed it won't be an issue.

OK, no problem.

> > > +
> > > +#if defined(HAVE_LINUX_IF_PACKET_H) && defined(HAVE_LINUX_IF_ETHER_H) \
> > > +       && LINUX_VERSION_CODE >= KERNEL_VERSION(4, 10, 0)

> > >  #include <linux/if_packet.h>
> > >  #include <linux/if_ether.h>
> > > @@ -373,6 +376,6 @@ static struct tst_test test = {

> > >  #else

> > > -TST_TEST_TCONF("Need <linux/if_packet.h> and <linux/if_ether.h>");
> > > +TST_TEST_TCONF("Need <linux/if_packet.h> and <linux/if_ether.h> and
> > > 4.10+");

> > +1.

> > BTW, why this tests requires 4.10 for runtime?

> It's for compilation. struct can_frame fields it's using were introduced in 4.10.

Do you mean a2f11835994e ("can.h: make padding given by gcc explicit") from
v4.2-rc1? + there is no change for include/uapi/linux/can.h in 4.10.

This is just a separate question to this fix (thank you for finding a solution).

Kind regards,
Petr

[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[Jan Stancek]

----- Original Message -----
> Hi Jan,
> 
> > > > -#if defined(HAVE_LINUX_IF_PACKET_H) && defined(HAVE_LINUX_IF_ETHER_H)
> > > > +#include <linux/version.h>
> > > Shouldn't we also check for <linux/version.h> with autotools? Or are
> > > kernel
> > > headers mandatory for LTP build?
> 
> > We already include this header in some syscalls tests, so I assumed it
> > won't be an issue.
> 
> OK, no problem.
> 
> > > > +
> > > > +#if defined(HAVE_LINUX_IF_PACKET_H) && defined(HAVE_LINUX_IF_ETHER_H)
> > > > \
> > > > +       && LINUX_VERSION_CODE >= KERNEL_VERSION(4, 10, 0)
> 
> > > >  #include <linux/if_packet.h>
> > > >  #include <linux/if_ether.h>
> > > > @@ -373,6 +376,6 @@ static struct tst_test test = {
> 
> > > >  #else
> 
> > > > -TST_TEST_TCONF("Need <linux/if_packet.h> and <linux/if_ether.h>");
> > > > +TST_TEST_TCONF("Need <linux/if_packet.h> and <linux/if_ether.h> and
> > > > 4.10+");
> 
> > > +1.
> 
> > > BTW, why this tests requires 4.10 for runtime?
> 
> > It's for compilation. struct can_frame fields it's using were introduced in
> > 4.10.
> 
> Do you mean a2f11835994e ("can.h: make padding given by gcc explicit") from
> v4.2-rc1? + there is no change for include/uapi/linux/can.h in 4.10.

You're right, I didn't sort the tags correctly.

[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[Cyril Hrubis]

Hi!
> -#if defined(HAVE_LINUX_IF_PACKET_H) && defined(HAVE_LINUX_IF_ETHER_H)
> +#include <linux/version.h>
> +
> +#if defined(HAVE_LINUX_IF_PACKET_H) && defined(HAVE_LINUX_IF_ETHER_H) \
> +       && LINUX_VERSION_CODE >= KERNEL_VERSION(4, 10, 0)

Wouldn't it be just easier to define the structure in the test instead?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v4 1/2] pty04: Use guarded buffers for transmission

[Petr Vorel]

> Hi!
> > -#if defined(HAVE_LINUX_IF_PACKET_H) && defined(HAVE_LINUX_IF_ETHER_H)
> > +#include <linux/version.h>
> > +
> > +#if defined(HAVE_LINUX_IF_PACKET_H) && defined(HAVE_LINUX_IF_ETHER_H) \
> > +       && LINUX_VERSION_CODE >= KERNEL_VERSION(4, 10, 0)

> Wouldn't it be just easier to define the structure in the test instead?
That's what Richard suggested.
I've sent a patch for that.

Kind regards,
Petr