[LTP] [PATCH v4 0/2] SysV IPC bug reproducer

[LTP] [PATCH v4 0/2] SysV IPC bug reproducer

[Andrea Cervesato]

From: Andrea Cervesato <andrea.cervesato@suse.com>

This patch series provides a bug reproducer for SysV IPC, which has been
written by Michal Hocko.
It also includes the SAFE_MPROTECT macro, useful for many other tests as well.

Andrea Cervesato (2):
  Add SAFE_MPROTECT() macro
  Add shmat04 SysV IPC bug reproducer

 include/tst_safe_macros.h                     |  5 +
 lib/safe_macros.c                             | 40 ++++++++
 runtest/syscalls                              |  1 +
 runtest/syscalls-ipc                          |  1 +
 .../kernel/syscalls/ipc/shmat/.gitignore      |  1 +
 testcases/kernel/syscalls/ipc/shmat/shmat04.c | 92 +++++++++++++++++++
 6 files changed, 140 insertions(+)
 create mode 100644 testcases/kernel/syscalls/ipc/shmat/shmat04.c

-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v4 1/2] Add SAFE_MPROTECT() macro

[Andrea Cervesato]

From: Andrea Cervesato <andrea.cervesato@suse.com>

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
TDEBUG for mprotect

 include/tst_safe_macros.h |  5 +++++
 lib/safe_macros.c         | 40 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index f2ce8919b..bd401f094 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -625,6 +625,11 @@ int safe_munlock(const char *file, const int lineno, const char *addr,
 	size_t len);
 #define SAFE_MUNLOCK(addr, len) safe_munlock(__FILE__, __LINE__, (addr), (len))
 
+int safe_mprotect(const char *file, const int lineno, const char *addr,
+	size_t len, int prot);
+#define SAFE_MPROTECT(addr, len, prot) \
+	safe_mprotect(__FILE__, __LINE__, (addr), (len), (prot))
+
 int safe_mincore(const char *file, const int lineno, void *start,
 	size_t length, unsigned char *vec);
 #define SAFE_MINCORE(start, length, vec) \
diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index 951e1b064..07d15c9a7 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -1317,6 +1317,46 @@ int safe_munlock(const char *file, const int lineno, const void *addr,
 	return rval;
 }
 
+int safe_mprotect(const char *file, const int lineno, void *addr,
+	size_t len, int prot)
+{
+	int rval;
+	char prot_buf[16];
+
+	switch (prot) {
+	case PROT_NONE:
+		snprintf(prot_buf, 16, "PROT_NONE");
+		break;
+	case PROT_WRITE:
+		snprintf(prot_buf, 16, "PROT_WRITE");
+		break;
+	case PROT_READ:
+		snprintf(prot_buf, 16, "PROT_READ");
+		break;
+	case PROT_EXEC:
+		snprintf(prot_buf, 16, "PROT_EXEC");
+		break;
+	default:
+		snprintf(prot_buf, 16, "UNKNOWN");
+		break;
+	}
+
+	tst_res_(file, lineno, TDEBUG,
+		"mprotect(%p, %d, %s)", addr, len, prot_buf);
+
+	rval = mprotect(addr, len, prot);
+
+	if (rval == -1) {
+		tst_brkm_(file, lineno, TBROK | TERRNO, NULL,
+			"mprotect() failed");
+	} else if (rval) {
+		tst_brkm_(file, lineno, TBROK | TERRNO, NULL,
+			"Invalid mprotect() return value %d", rval);
+	}
+
+	return rval;
+}
+
 int safe_mincore(const char *file, const int lineno, void *start,
 	size_t length, unsigned char *vec)
 {
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v4 1/2] Add SAFE_MPROTECT() macro

[Cyril Hrubis]

Hi!
> +	int rval;
> +	char prot_buf[16];
> +
> +	switch (prot) {
> +	case PROT_NONE:
> +		snprintf(prot_buf, 16, "PROT_NONE");
> +		break;
> +	case PROT_WRITE:
> +		snprintf(prot_buf, 16, "PROT_WRITE");
> +		break;
> +	case PROT_READ:
> +		snprintf(prot_buf, 16, "PROT_READ");
> +		break;
> +	case PROT_EXEC:
> +		snprintf(prot_buf, 16, "PROT_EXEC");
> +		break;
> +	default:
> +		snprintf(prot_buf, 16, "UNKNOWN");
> +		break;
> +	}

This is ugly and does not work.

First of all we can just do:

	char *prot_name;


	switch (prot) {
	case PROT_NONE:
		prot_name = "PROT_NONE";
	break;
	...
	}

And secondly it does not work for common combinations, like (PROT_READ | PROT_WRITE).

So I guess that the easiest solution is to walk the bits and append the
string something as:

#define PROT_READ_NAME "PROT_READ | "
#define PROT_WRITE_NAME "PROT_WRITE | "
#define PROT_EXEC_NAME "PROT_EXEC | "

static const char *prot_to_str(int prot, char *buf, size_t buf_len)
{
	char *orig_buf = buf;

	if (buf_len < sizeof(PROT_READ_NAME) + sizeof(PROT_WRITE_NAME) + sizeof(PROT_EXEC_NAME))
		return "BUFFER TOO SMALL!!!";

	if (prot == 0)
		return "PROT_NONE";

	*buf = 0;

	if (prot & PROT_READ) {
		strcpy(buf, PROT_READ_NAME);
		buf += sizeof(PROT_READ_NAME)-1;
	}

	if (prot & PROT_WRITE) {
		strcpy(buf, PROT_WRITE_NAME);
		buf += sizeof(PROT_WRITE_NAME)-1;
	}

	if (prot & PROT_EXEC) {
		strcpy(buf, PROT_EXEC_NAME);
		buf += sizeof(PROT_EXEC_NAME)-1;
	}

	if (orig_buf != buf)
		buf[-3] = 0;

	return buf;
}

Also I would put the code that translates the prot into string into a
separate function because that code should be used in the safe_mmap() as
well.

> +	tst_res_(file, lineno, TDEBUG,
> +		"mprotect(%p, %d, %s)", addr, len, prot_buf);

Can we print hexadecimal value of the prot here as well?

		"mprotect(%p, %d, %s(%x))", addr, len, prot_to_str(prot, buf, sizeof(buf)), prot);

> +	rval = mprotect(addr, len, prot);
> +
> +	if (rval == -1) {
> +		tst_brkm_(file, lineno, TBROK | TERRNO, NULL,
> +			"mprotect() failed");
> +	} else if (rval) {
> +		tst_brkm_(file, lineno, TBROK | TERRNO, NULL,
> +			"Invalid mprotect() return value %d", rval);
> +	}
> +
> +	return rval;
> +}
> +
>  int safe_mincore(const char *file, const int lineno, void *start,
>  	size_t length, unsigned char *vec)
>  {
> -- 
> 2.35.3
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v4 1/2] Add SAFE_MPROTECT() macro

[Petr Vorel]

Hi,

> Hi!
> > +	int rval;
> > +	char prot_buf[16];
> > +
> > +	switch (prot) {
> > +	case PROT_NONE:
> > +		snprintf(prot_buf, 16, "PROT_NONE");
> > +		break;
> > +	case PROT_WRITE:
> > +		snprintf(prot_buf, 16, "PROT_WRITE");
> > +		break;
> > +	case PROT_READ:
> > +		snprintf(prot_buf, 16, "PROT_READ");
> > +		break;
> > +	case PROT_EXEC:
> > +		snprintf(prot_buf, 16, "PROT_EXEC");
> > +		break;
> > +	default:
> > +		snprintf(prot_buf, 16, "UNKNOWN");
> > +		break;
> > +	}

> This is ugly and does not work.

> First of all we can just do:

> 	char *prot_name;


> 	switch (prot) {
> 	case PROT_NONE:
> 		prot_name = "PROT_NONE";
> 	break;
> 	...
> 	}

> And secondly it does not work for common combinations, like (PROT_READ | PROT_WRITE).

> So I guess that the easiest solution is to walk the bits and append the
> string something as:

> #define PROT_READ_NAME "PROT_READ | "
> #define PROT_WRITE_NAME "PROT_WRITE | "
> #define PROT_EXEC_NAME "PROT_EXEC | "

nit: maybe using stringification?

#define FLAG(f) f, #f " | "

FLAG(PROT_READ)

> static const char *prot_to_str(int prot, char *buf, size_t buf_len)
> {
> 	char *orig_buf = buf;

> 	if (buf_len < sizeof(PROT_READ_NAME) + sizeof(PROT_WRITE_NAME) + sizeof(PROT_EXEC_NAME))
> 		return "BUFFER TOO SMALL!!!";

> 	if (prot == 0)
> 		return "PROT_NONE";

> 	*buf = 0;

> 	if (prot & PROT_READ) {
> 		strcpy(buf, PROT_READ_NAME);
> 		buf += sizeof(PROT_READ_NAME)-1;
> 	}

> 	if (prot & PROT_WRITE) {
> 		strcpy(buf, PROT_WRITE_NAME);
> 		buf += sizeof(PROT_WRITE_NAME)-1;
> 	}

> 	if (prot & PROT_EXEC) {
> 		strcpy(buf, PROT_EXEC_NAME);
> 		buf += sizeof(PROT_EXEC_NAME)-1;
> 	}

> 	if (orig_buf != buf)
> 		buf[-3] = 0;

> 	return buf;
> }

> Also I would put the code that translates the prot into string into a
> separate function because that code should be used in the safe_mmap() as
> well.

+1

> > +	tst_res_(file, lineno, TDEBUG,
> > +		"mprotect(%p, %d, %s)", addr, len, prot_buf);

> Can we print hexadecimal value of the prot here as well?

+1

> 		"mprotect(%p, %d, %s(%x))", addr, len, prot_to_str(prot, buf, sizeof(buf)), prot);

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v4 2/2] Add shmat04 SysV IPC bug reproducer

[Andrea Cervesato]

From: Andrea Cervesato <andrea.cervesato@suse.com>

This is an LTP port of a SysV bug reproducer provided by Michal Hocko.

When debugging issues with a workload using SysV shmem, Michal Hocko has
come up with a reproducer that shows how a series of mprotect()
operations can result in an elevated shm_nattch and thus leak of the
resource.

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
result of TST_EXP_EQ_LU() as test result
removed change_access

 runtest/syscalls                              |  1 +
 runtest/syscalls-ipc                          |  1 +
 .../kernel/syscalls/ipc/shmat/.gitignore      |  1 +
 testcases/kernel/syscalls/ipc/shmat/shmat04.c | 92 +++++++++++++++++++
 4 files changed, 95 insertions(+)
 create mode 100644 testcases/kernel/syscalls/ipc/shmat/shmat04.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 7794f1465..cc0144379 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1445,6 +1445,7 @@ setxattr03 setxattr03
 shmat01 shmat01
 shmat02 shmat02
 shmat03 shmat03
+shmat04 shmat04
 
 shmctl01 shmctl01
 shmctl02 shmctl02
diff --git a/runtest/syscalls-ipc b/runtest/syscalls-ipc
index df41140a7..9860394de 100644
--- a/runtest/syscalls-ipc
+++ b/runtest/syscalls-ipc
@@ -49,6 +49,7 @@ semop03 semop03
 
 shmat01 shmat01
 shmat02 shmat02
+shmat04 shmat04
 
 shmctl01 shmctl01
 shmctl02 shmctl02
diff --git a/testcases/kernel/syscalls/ipc/shmat/.gitignore b/testcases/kernel/syscalls/ipc/shmat/.gitignore
index 2b972f8f2..5600b2742 100644
--- a/testcases/kernel/syscalls/ipc/shmat/.gitignore
+++ b/testcases/kernel/syscalls/ipc/shmat/.gitignore
@@ -1,3 +1,4 @@
 /shmat01
 /shmat02
 /shmat03
+/shmat04
diff --git a/testcases/kernel/syscalls/ipc/shmat/shmat04.c b/testcases/kernel/syscalls/ipc/shmat/shmat04.c
new file mode 100644
index 000000000..adbb5a130
--- /dev/null
+++ b/testcases/kernel/syscalls/ipc/shmat/shmat04.c
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024 SUSE LLC
+ * Author: Michal Hocko <mhocko@suse.com>
+ * LTP port: Andrea Cervesato <andrea.cervesato@suse.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * When debugging issues with a workload using SysV shmem, Michal Hocko has
+ * come up with a reproducer that shows how a series of mprotect()
+ * operations can result in an elevated shm_nattch and thus leak of the
+ * resource.
+ *
+ * The problem is caused by wrong assumptions in vma_merge() commit
+ * 714965ca8252 ("mm/mmap: start distinguishing if vma can be removed in
+ * mergeability test"). The shmem vmas have a vma_ops->close callback
+ * that decrements shm_nattch, and we remove the vma without calling it.
+ *
+ * Patch: https://lore.kernel.org/all/20240222215930.14637-2-vbabka@suse.cz/
+ */
+
+#include "tst_test.h"
+#include "tst_safe_sysv_ipc.h"
+#include "libnewipc.h"
+
+static int segment_id = -1;
+static int key_id;
+static int page_size;
+static size_t segment_size;
+
+static void run(void)
+{
+	struct shmid_ds shmid_ds;
+	void *sh_mem;
+
+	segment_id = SAFE_SHMGET(
+		key_id,
+		segment_size,
+		IPC_CREAT | IPC_EXCL | 0600);
+
+	sh_mem = SAFE_SHMAT(segment_id, NULL, 0);
+
+	tst_res(TINFO, "Attached at %p. key: %d - size: %lu",
+		sh_mem, segment_id, segment_size);
+
+	SAFE_SHMCTL(segment_id, IPC_STAT, &shmid_ds);
+
+	tst_res(TINFO, "Number of attaches: %lu", shmid_ds.shm_nattch);
+
+	SAFE_MPROTECT(sh_mem + page_size, page_size, PROT_NONE);
+	SAFE_MPROTECT(sh_mem, 2 * page_size, PROT_WRITE);
+
+	SAFE_SHMCTL(segment_id, IPC_STAT, &shmid_ds);
+
+	tst_res(TINFO, "Number of attaches: %lu", shmid_ds.shm_nattch);
+	tst_res(TINFO, "Delete attached memory");
+
+	SAFE_SHMDT(sh_mem);
+	SAFE_SHMCTL(segment_id, IPC_STAT, &shmid_ds);
+
+	tst_res(TINFO, "Number of attaches: %lu", shmid_ds.shm_nattch);
+
+	SAFE_SHMCTL(segment_id, IPC_RMID, NULL);
+	segment_id = -1;
+
+	TST_EXP_EQ_LU(shmid_ds.shm_nattch, 0);
+}
+
+static void setup(void)
+{
+	key_id = GETIPCKEY();
+	page_size = getpagesize();
+
+	tst_res(TINFO, "Key id: %d", key_id);
+	tst_res(TINFO, "Page size: %d", page_size);
+
+	segment_size = 3 * page_size;
+}
+
+static void cleanup(void)
+{
+	if (segment_id != -1)
+		SAFE_SHMCTL(segment_id, IPC_RMID, NULL);
+}
+
+static struct tst_test test = {
+	.test_all = run,
+	.setup = setup,
+	.cleanup = cleanup,
+};
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp