diff mbox series

[v2,2/2] Add shmat04 SysV IPC bug reproducer

Message ID 20240226153754.24998-3-andrea.cervesato@suse.de
State Superseded
Headers show
Series SysV IPC bug reproducer | expand

Commit Message

Andrea Cervesato Feb. 26, 2024, 3:37 p.m. UTC 
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

Comments

Cyril Hrubis Feb. 26, 2024, 4:06 p.m. UTC | #1 
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
Andrea Cervesato March 4, 2024, 2:17 p.m. UTC | #2 
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
Cyril Hrubis March 4, 2024, 2:19 p.m. UTC | #3 
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.
Petr Vorel March 4, 2024, 2:32 p.m. UTC | #4 
> 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
diff mbox series

Patch

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