[LTP] [PATCH v5 0/3] SysV IPC bug reproducer

[LTP] [PATCH v5 0/3] 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 (3):
  Add SAFE_MPROTECT() macro
  Print prot flag when SAFE_MMAP() fails
  Add shmat04 SysV IPC bug reproducer

 include/tst_safe_macros.h                     | 70 +++++++++++++-
 runtest/syscalls                              |  1 +
 runtest/syscalls-ipc                          |  1 +
 .../kernel/syscalls/ipc/shmat/.gitignore      |  1 +
 testcases/kernel/syscalls/ipc/shmat/shmat04.c | 92 +++++++++++++++++++
 5 files changed, 163 insertions(+), 2 deletions(-)
 create mode 100644 testcases/kernel/syscalls/ipc/shmat/shmat04.c

-- 
2.35.3


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

[LTP] [PATCH v5 1/3] Add SAFE_MPROTECT() macro

[Andrea Cervesato]

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

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
Moved SAFE_MPROTECT() macro into tst_safe_macros.h

 include/tst_safe_macros.h | 60 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index 2d497f344..15f914619 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -6,6 +6,7 @@
 #ifndef TST_SAFE_MACROS_H__
 #define TST_SAFE_MACROS_H__
 
+#include <stdlib.h>
 #include <sys/mman.h>
 #include <sys/types.h>
 #include <sys/time.h>
@@ -268,6 +269,36 @@ int safe_getgroups(const char *file, const int lineno, int size, gid_t list[]);
  * -D_FILE_OFFSET_BITS=64 compile flag
  */
 
+#define PROT_FLAG_STR(f) #f " | "
+
+static void prot_to_str(const int prot, char *buf)
+{
+	char *ptr = buf;
+
+	if (prot == PROT_NONE) {
+		strcpy(buf, "PROT_NONE");
+		return;
+	}
+
+	if (prot & PROT_READ) {
+		strcpy(ptr, PROT_FLAG_STR(PROT_READ));
+		ptr += sizeof(PROT_FLAG_STR(PROT_READ)) - 1;
+	}
+
+	if (prot & PROT_WRITE) {
+		strcpy(ptr, PROT_FLAG_STR(PROT_WRITE));
+		ptr += sizeof(PROT_FLAG_STR(PROT_WRITE)) - 1;
+	}
+
+	if (prot & PROT_EXEC) {
+		strcpy(ptr, PROT_FLAG_STR(PROT_EXEC));
+		ptr += sizeof(PROT_FLAG_STR(PROT_EXEC)) - 1;
+	}
+
+	if (buf != ptr)
+		ptr[-3] = 0;
+}
+
 static inline void *safe_mmap(const char *file, const int lineno,
                               void *addr, size_t length,
                               int prot, int flags, int fd, off_t offset)
@@ -287,6 +318,35 @@ static inline void *safe_mmap(const char *file, const int lineno,
 	safe_mmap(__FILE__, __LINE__, (addr), (length), (prot), \
 	(flags), (fd), (offset))
 
+static inline int safe_mprotect(const char *file, const int lineno,
+	char *addr, size_t len, int prot)
+{
+	int rval;
+	char *prot_buf;
+
+	prot_buf = (char*) safe_malloc(file, lineno, 0, 512);
+	prot_to_str(prot, prot_buf);
+
+	tst_res_(file, lineno, TDEBUG,
+		"mprotect(%p, %ld, %s(%x))", addr, len, prot_buf, prot);
+
+	free(prot_buf);
+
+	rval = mprotect(addr, len, prot);
+
+	if (rval == -1) {
+		tst_brk_(file, lineno, TBROK | TERRNO,
+			"mprotect() failed");
+	} else if (rval) {
+		tst_brk_(file, lineno, TBROK | TERRNO,
+			"Invalid mprotect() return value %d", rval);
+	}
+
+	return rval;
+}
+#define SAFE_MPROTECT(addr, len, prot) \
+	safe_mprotect(__FILE__, __LINE__, (addr), (len), (prot))
+
 static inline int safe_ftruncate(const char *file, const int lineno,
                                  int fd, off_t length)
 {
-- 
2.35.3


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

Re: [LTP] [PATCH v5 1/3] Add SAFE_MPROTECT() macro

[Cyril Hrubis]

Hi!
> +static void prot_to_str(const int prot, char *buf)
> +{
> +	char *ptr = buf;
>

I would still put an explicit check for the buffer size here, so that we
are sure that the complete combination of READ|WRITE|EXEC would fit, but
I guess that it's fine as long as this is an internal static function
and not part of the API.

> +	if (prot == PROT_NONE) {
> +		strcpy(buf, "PROT_NONE");
> +		return;
> +	}
> +
> +	if (prot & PROT_READ) {
> +		strcpy(ptr, PROT_FLAG_STR(PROT_READ));
> +		ptr += sizeof(PROT_FLAG_STR(PROT_READ)) - 1;
> +	}
> +
> +	if (prot & PROT_WRITE) {
> +		strcpy(ptr, PROT_FLAG_STR(PROT_WRITE));
> +		ptr += sizeof(PROT_FLAG_STR(PROT_WRITE)) - 1;
> +	}
> +
> +	if (prot & PROT_EXEC) {
> +		strcpy(ptr, PROT_FLAG_STR(PROT_EXEC));
> +		ptr += sizeof(PROT_FLAG_STR(PROT_EXEC)) - 1;
> +	}
> +
> +	if (buf != ptr)
> +		ptr[-3] = 0;
> +}
> +
>  static inline void *safe_mmap(const char *file, const int lineno,
>                                void *addr, size_t length,
>                                int prot, int flags, int fd, off_t offset)
> @@ -287,6 +318,35 @@ static inline void *safe_mmap(const char *file, const int lineno,
>  	safe_mmap(__FILE__, __LINE__, (addr), (length), (prot), \
>  	(flags), (fd), (offset))
>  
> +static inline int safe_mprotect(const char *file, const int lineno,
> +	char *addr, size_t len, int prot)
> +{
> +	int rval;
> +	char *prot_buf;
> +
> +	prot_buf = (char*) safe_malloc(file, lineno, 0, 512);

Why are we allocating the buffer? Why not just prot_buf[512] ?

Also the cast to (char*) is never needed in C as void* is automatically
converted to any type of a pointer without explicit cast.


Otherwise it looks good. You can add my Reviewed-by: if you change the
malloc to an array on the stack.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v5 1/3] Add SAFE_MPROTECT() macro

[Cyril Hrubis]

Hi!
> +static inline int safe_mprotect(const char *file, const int lineno,
> +	char *addr, size_t len, int prot)
> +{
> +	int rval;
> +	char *prot_buf;
> +
> +	prot_buf = (char*) safe_malloc(file, lineno, 0, 512);
> +	prot_to_str(prot, prot_buf);
> +
> +	tst_res_(file, lineno, TDEBUG,
> +		"mprotect(%p, %ld, %s(%x))", addr, len, prot_buf, prot);
> +
> +	free(prot_buf);
> +
> +	rval = mprotect(addr, len, prot);
> +
> +	if (rval == -1) {
> +		tst_brk_(file, lineno, TBROK | TERRNO,
> +			"mprotect() failed");
> +	} else if (rval) {
> +		tst_brk_(file, lineno, TBROK | TERRNO,
> +			"Invalid mprotect() return value %d", rval);
> +	}


Ah, and can we please print the whole parameter list for mprotect() in
these two cases as well?

> +	return rval;
> +}
> +#define SAFE_MPROTECT(addr, len, prot) \
> +	safe_mprotect(__FILE__, __LINE__, (addr), (len), (prot))
> +
>  static inline int safe_ftruncate(const char *file, const int lineno,
>                                   int fd, off_t length)
>  {
> -- 
> 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

[LTP] [PATCH v5 2/3] Print prot flag when SAFE_MMAP() fails

[Andrea Cervesato]

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

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 include/tst_safe_macros.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index 15f914619..63ad46b85 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -304,12 +304,18 @@ static inline void *safe_mmap(const char *file, const int lineno,
                               int prot, int flags, int fd, off_t offset)
 {
 	void *rval;
+	char *prot_buf;
 
 	rval = mmap(addr, length, prot, flags, fd, offset);
 	if (rval == MAP_FAILED) {
+		prot_buf = (char*) safe_malloc(file, lineno, 0, 512);
+		prot_to_str(prot, prot_buf);
+
 		tst_brk_(file, lineno, TBROK | TERRNO,
-			"mmap(%p,%zu,%d,%d,%d,%ld) failed",
-			addr, length, prot, flags, fd, (long) offset);
+			"mmap(%p,%zu,%s(%x),%d,%d,%ld) failed",
+			addr, length, prot_buf, prot, flags, fd, (long) offset);
+
+		free(prot_buf);
 	}
 
 	return rval;
-- 
2.35.3


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

Re: [LTP] [PATCH v5 2/3] Print prot flag when SAFE_MMAP() fails

[Cyril Hrubis]

Hi!
>  include/tst_safe_macros.h | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
> index 15f914619..63ad46b85 100644
> --- a/include/tst_safe_macros.h
> +++ b/include/tst_safe_macros.h
> @@ -304,12 +304,18 @@ static inline void *safe_mmap(const char *file, const int lineno,
>                                int prot, int flags, int fd, off_t offset)
>  {
>  	void *rval;
> +	char *prot_buf;

Shouldn't we add the TDEBUG message here?

>  	rval = mmap(addr, length, prot, flags, fd, offset);
>  	if (rval == MAP_FAILED) {
> +		prot_buf = (char*) safe_malloc(file, lineno, 0, 512);
> +		prot_to_str(prot, prot_buf);
> +
>  		tst_brk_(file, lineno, TBROK | TERRNO,
> -			"mmap(%p,%zu,%d,%d,%d,%ld) failed",
> -			addr, length, prot, flags, fd, (long) offset);
> +			"mmap(%p,%zu,%s(%x),%d,%d,%ld) failed",
> +			addr, length, prot_buf, prot, flags, fd, (long) offset);
> +
> +		free(prot_buf);

This is fine as long as we switch to an on the stack array.

>  	}
>  
>  	return rval;
> -- 
> 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

[LTP] [PATCH v5 3/3] 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>
---
Ensure that GETIPCKEY() will give a free slot given IPC key + 1

 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 fab870ace..4ed2b5602 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1444,6 +1444,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..ed03f8260
--- /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() + 1;
+	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