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

[LTP] [PATCH v2 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                             |  18 +++
 runtest/syscalls                              |   1 +
 runtest/syscalls-ipc                          |   1 +
 .../kernel/syscalls/ipc/shmat/.gitignore      |   1 +
 testcases/kernel/syscalls/ipc/shmat/shmat04.c | 115 ++++++++++++++++++
 6 files changed, 141 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 v2 1/2] Add SAFE_MPROTECT() macro

[Andrea Cervesato]

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

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 include/tst_safe_macros.h |  5 +++++
 lib/safe_macros.c         | 18 ++++++++++++++++++
 2 files changed, 23 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..69ca3eabc 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -1317,6 +1317,24 @@ int safe_munlock(const char *file, const int lineno, const void *addr,
 	return rval;
 }
 
+int safe_mprotect(const char *file, const int lineno, const void *addr,
+	size_t len, int prot)
+{
+	int rval;
+
+	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 v2 1/2] Add SAFE_MPROTECT() macro

[Cyril Hrubis]

Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

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

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

[Petr Vorel]

Hi Andrea,

> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  include/tst_safe_macros.h |  5 +++++
>  lib/safe_macros.c         | 18 ++++++++++++++++++
>  2 files changed, 23 insertions(+)

Although Cyril acked this I wonder why we are adding it into the old API.
Shouldn't it be added only into tst_safe_macros.c?

Kind regards,
Petr

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

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

[Cyril Hrubis]

Hi!
> > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> > ---
> >  include/tst_safe_macros.h |  5 +++++
> >  lib/safe_macros.c         | 18 ++++++++++++++++++
> >  2 files changed, 23 insertions(+)
> 
> Although Cyril acked this I wonder why we are adding it into the old API.
> Shouldn't it be added only into tst_safe_macros.c?

To be honest I do not care that much as long as we do not expose the
functions into the headers for old API. We will have to convert the
safe_macros.c with coccinelle once all tests are converted and one more
function in there does not really matter that much.

-- 
Cyril Hrubis
chrubis@suse.cz

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

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

[Petr Vorel]

> Hi!
> > > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> > > ---
> > >  include/tst_safe_macros.h |  5 +++++
> > >  lib/safe_macros.c         | 18 ++++++++++++++++++
> > >  2 files changed, 23 insertions(+)

> > Although Cyril acked this I wonder why we are adding it into the old API.
> > Shouldn't it be added only into tst_safe_macros.c?

> To be honest I do not care that much as long as we do not expose the
> functions into the headers for old API. We will have to convert the
> safe_macros.c with coccinelle once all tests are converted and one more
> function in there does not really matter that much.

OTOH we have tst_* file, moving it there (+ remove 'm' from functions) would be
trivial (and less confusing). Sure, finally coccinelle will be used.

Kind regards,
Petr

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

[LTP] [PATCH v2 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>
---
Simplified the algorithm
Usage of SAFE_MPROTECT
More TINFO messages

 runtest/syscalls                              |   1 +
 runtest/syscalls-ipc                          |   1 +
 .../kernel/syscalls/ipc/shmat/.gitignore      |   1 +
 testcases/kernel/syscalls/ipc/shmat/shmat04.c | 115 ++++++++++++++++++
 4 files changed, 118 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..63daaa73e
--- /dev/null
+++ b/testcases/kernel/syscalls/ipc/shmat/shmat04.c
@@ -0,0 +1,115 @@
+// 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 change_access(void *addr, int size, int prot)
+{
+	switch (prot) {
+	case PROT_NONE:
+		tst_res(TINFO, "Disable memory access. addr: %p - size: %d",
+			addr, size);
+		break;
+	case PROT_WRITE:
+		tst_res(TINFO, "Enable write memory access. addr: %p - size: %d",
+			addr, size);
+		break;
+	default:
+		tst_res(TINFO, "Change memory access. addr: %p - size: %d",
+			addr, size);
+		break;
+	}
+
+	SAFE_MPROTECT(addr, size, prot);
+}
+
+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);
+
+	change_access(sh_mem + page_size, page_size, PROT_NONE);
+	change_access(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;
+
+	if (shmid_ds.shm_nattch)
+		tst_res(TFAIL, "The system is affected by the SysV IPC bug");
+	else
+		tst_res(TPASS, "Test passed");
+}
+
+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

Re: [LTP] [PATCH v2 2/2] Add shmat04 SysV IPC bug reproducer

[Cyril Hrubis]

Hi!
> +static void change_access(void *addr, int size, int prot)
> +{
> +	switch (prot) {
> +	case PROT_NONE:
> +		tst_res(TINFO, "Disable memory access. addr: %p - size: %d",
> +			addr, size);
> +		break;
> +	case PROT_WRITE:
> +		tst_res(TINFO, "Enable write memory access. addr: %p - size: %d",
> +			addr, size);
> +		break;
> +	default:
> +		tst_res(TINFO, "Change memory access. addr: %p - size: %d",
> +			addr, size);
> +		break;
> +	}
> +
> +	SAFE_MPROTECT(addr, size, prot);
> +}

Hmm, it's kind of ugly how we wrap the macro here like that...

What about we instead add debugging messages to all the SAFE_MACROS()?

Given that we added TDEBUG flag recently we can do soemthing as:

	tst_res_(TDEBUG, file, lineno, "mprotect(%p, %d, %s)",
	         addr, size, prot_to_str(prot));

To the SAFE_MPROTECT() and get the verbose output for free with verbose
flag passed to the test.

We can do that with all SAFE_MACROS() then we do not have to print most
of the messages in this test...

> +
> +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);
> +
> +	change_access(sh_mem + page_size, page_size, PROT_NONE);
> +	change_access(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;
> +
> +	if (shmid_ds.shm_nattch)
> +		tst_res(TFAIL, "The system is affected by the SysV IPC bug");
> +	else
> +		tst_res(TPASS, "Test passed");

These messages are not really that useful, we can as well do:

TST_EXP_EQ_LU(shmid_ds.shm_nattach, 0);

That will provide better message than "PASS: Test passed"

> +}
> +
> +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

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v2 2/2] Add shmat04 SysV IPC bug reproducer

[Andrea Cervesato via ltp]

Hi!

On 2/26/24 17:06, Cyril Hrubis wrote:
> Hi!
>> +static void change_access(void *addr, int size, int prot)
>> +{
>> +	switch (prot) {
>> +	case PROT_NONE:
>> +		tst_res(TINFO, "Disable memory access. addr: %p - size: %d",
>> +			addr, size);
>> +		break;
>> +	case PROT_WRITE:
>> +		tst_res(TINFO, "Enable write memory access. addr: %p - size: %d",
>> +			addr, size);
>> +		break;
>> +	default:
>> +		tst_res(TINFO, "Change memory access. addr: %p - size: %d",
>> +			addr, size);
>> +		break;
>> +	}
>> +
>> +	SAFE_MPROTECT(addr, size, prot);
>> +}
> Hmm, it's kind of ugly how we wrap the macro here like that...
>
> What about we instead add debugging messages to all the SAFE_MACROS()?
>
> Given that we added TDEBUG flag recently we can do soemthing as:
>
> 	tst_res_(TDEBUG, file, lineno, "mprotect(%p, %d, %s)",
> 	         addr, size, prot_to_str(prot));
>
> To the SAFE_MPROTECT() and get the verbose output for free with verbose
> flag passed to the test.
>
> We can do that with all SAFE_MACROS() then we do not have to print most
> of the messages in this test...
Is this comment related with the previous patch of the set?
>> +
>> +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);
>> +
>> +	change_access(sh_mem + page_size, page_size, PROT_NONE);
>> +	change_access(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;
>> +
>> +	if (shmid_ds.shm_nattch)
>> +		tst_res(TFAIL, "The system is affected by the SysV IPC bug");
>> +	else
>> +		tst_res(TPASS, "Test passed");
> These messages are not really that useful, we can as well do:
>
> TST_EXP_EQ_LU(shmid_ds.shm_nattach, 0);
>
> That will provide better message than "PASS: Test passed"
>
>> +}
>> +
>> +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

Andrea


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

Re: [LTP] [PATCH v2 2/2] Add shmat04 SysV IPC bug reproducer

[Cyril Hrubis]

Hi!
> > Hmm, it's kind of ugly how we wrap the macro here like that...
> >
> > What about we instead add debugging messages to all the SAFE_MACROS()?
> >
> > Given that we added TDEBUG flag recently we can do soemthing as:
> >
> > 	tst_res_(TDEBUG, file, lineno, "mprotect(%p, %d, %s)",
> > 	         addr, size, prot_to_str(prot));
> >
> > To the SAFE_MPROTECT() and get the verbose output for free with verbose
> > flag passed to the test.
> >
> > We can do that with all SAFE_MACROS() then we do not have to print most
> > of the messages in this test...
> Is this comment related with the previous patch of the set?

Not at all, I'm just complaining that we are adding debuging print to
the test itself when it would be much cleaner to put it into the test
library instead.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v2 2/2] Add shmat04 SysV IPC bug reproducer

[Petr Vorel]

> Hi!
> > > Hmm, it's kind of ugly how we wrap the macro here like that...

> > > What about we instead add debugging messages to all the SAFE_MACROS()?

> > > Given that we added TDEBUG flag recently we can do soemthing as:

> > > 	tst_res_(TDEBUG, file, lineno, "mprotect(%p, %d, %s)",
> > > 	         addr, size, prot_to_str(prot));

> > > To the SAFE_MPROTECT() and get the verbose output for free with verbose
> > > flag passed to the test.

> > > We can do that with all SAFE_MACROS() then we do not have to print most
> > > of the messages in this test...
> > Is this comment related with the previous patch of the set?

> Not at all, I'm just complaining that we are adding debuging print to
> the test itself when it would be much cleaner to put it into the test
> library instead.

+1, setting patchset as changes requested, please send v3.

Kind regards,
Petr

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