hugetlb/hugemmap: add new testcase hugemmap06.c

v1 ---> v2
1. remove the useless include file
2. add huge pages checking in setup()
3. mmap huge pages in reverse order

Description of Problem:

	There is a race condition if we map a same file on different processes.
	Region tracking is protected by mmap_sem and hugetlb_instantiation_mutex.
	When we do mmap, we don't grab a hugetlb_instantiation_mutex, but only
	mmap_sem (exclusively).  This doesn't prevent other tasks from modifying
	the region structure, so it can be modified by two processes concurrently.

	Testcase hugemmap06.c is the trigger to cause system crash:

         crash> bt -s
         PID: 4492   TASK: ffff88033e437520  CPU: 2   COMMAND: "hugemmap06"
          #0 [ffff88033dbb3960] machine_kexec+395 at ffffffff8103d1ab
          #1 [ffff88033dbb39c0] crash_kexec+114 at ffffffff810cc4f2
          #2 [ffff88033dbb3a90] oops_end+192 at ffffffff8153c840
          #3 [ffff88033dbb3ac0] die+91 at ffffffff81010f5b
          #4 [ffff88033dbb3af0] do_general_protection+338 at ffffffff8153c332
          #5 [ffff88033dbb3b20] general_protection+37 at ffffffff8153bb05
             [exception RIP: list_del+40]
             RIP: ffffffff812a3598  RSP: ffff88033dbb3bd8  RFLAGS: 00010292
             RAX: dead000000100100  RBX: ffff88013cf37340  RCX: 0000000000002dc2
             RDX: dead000000200200  RSI: 0000000000000046  RDI: 0000000000000009
             RBP: ffff88033dbb3be8   R8: 0000000000015598   R9: 0000000000000000
             R10: 000000000000000f  R11: 0000000000000009  R12: 000000000000000a
             R13: ffff88033d64b9e8  R14: ffff88033e5b9720  R15: ffff88013cf37340
             ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0000
          #6 [ffff88033dbb3bf0] region_add+154 at ffffffff811698da
          #7 [ffff88033dbb3c40] alloc_huge_page+669 at ffffffff8116a61d
          #8 [ffff88033dbb3ce0] hugetlb_fault+1083 at ffffffff8116b9bb
          #9 [ffff88033dbb3d90] handle_mm_fault+917 at ffffffff81153295
         #10 [ffff88033dbb3e00] __do_page_fault+326 at ffffffff8104f156
         #11 [ffff88033dbb3f20] do_page_fault+62 at ffffffff8153e78e
         #12 [ffff88033dbb3f50] page_fault+37 at ffffffff8153bb35
             RIP: 00000000004027c6  RSP: 00007f7cadef9e80  RFLAGS: 00010297
             RAX: 000000005a49238f  RBX: 00007ffcb2d19320  RCX: 000000357498e084
             RDX: 000000357498e0b0  RSI: 00007f7cadef9e5c  RDI: 000000357498e4e0
             RBP: 0000000000000008   R8: 000000357498e0a0   R9: 000000357498e100
             R10: 00007f7cadefa9d0  R11: 0000000000000206  R12: 0000000000000007
             R13: 0000000000000002  R14: 0000000000000003  R15: 00002aaaac000000
             ORIG_RAX: ffffffffffffffff  CS: 0033  SS: 002b

The fix are all these below commits:

	commit f522c3ac00a49128115f99a5fcb95a447601c1c3
	Author:Joonsoo Kim < iamjoonsoo.kim @ lge.com >
	Date:Wed Sep 11 14:21:53 2013 - 0700

	commit 9119 a41e9091fb3a8204039d595bcdae24193c57
	Author:Joonsoo Kim < iamjoonsoo.kim @ lge.com >
	Date:Thu Apr 3 14:47:25 2014 - 0700

	commit 7 b24d8616be33616efd41ff67d3c76362c60ca84
	Author:Davidlohr Bueso < davidlohr @ hp.com >
	Date:Thu Apr 3 14:47:27 2014 - 0700

	commit 1406ec9 ba6c65cb69e9243bff07ca3f51e2525e0
	Author:Joonsoo Kim < iamjoonsoo.kim @ lge.com >
	Date:Thu Apr 3 14:47:26 2014 - 0700

Signed-off-by Li Wang <[email protected]>

---

testcases/kernel/mem/hugetlb/hugemmap/Makefile +2 -0

+
+hugemmap06: CFLAGS+=-pthread

testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c new file +9 -0

new file mode 100644
index 0000000..77250f2
@@ -0,0 +1,192 @@
+/*
+ *  Copyright (c) Author: Herton R. Krzesinski <[email protected]>
+ *                Modify: Li Wang <[email protected]>
+ *
+ *  This program is free software;  you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *

testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c new file +108 -0

+
+/*
+ *  DESCRIPTION
+ *
+ *    There is a race condition if we map a same file on different processes.
+ *    Region tracking is protected by mmap_sem and hugetlb_instantiation_mutex.
+ *    When we do mmap, we don't grab a hugetlb_instantiation_mutex, but only
+ *    mmap_sem (exclusively).  This doesn't prevent other tasks from modifying
+ *    the region structure, so it can be modified by two processes concurrently.
+ *
+ *    This bug was fixed on stable kernel by commits:
+ *
+ *        commit f522c3ac00a49128115f99a5fcb95a447601c1c3
+ *        Author: Joonsoo Kim <[email protected]>
+ *        Date:   Wed Sep 11 14:21:53 2013 -0700
+ *
+ *        commit 9119a41e9091fb3a8204039d595bcdae24193c57
+ *        Author: Joonsoo Kim <[email protected]>
+ *        Date:   Thu Apr 3 14:47:25 2014 -0700
+ *
+ *        commit 7b24d8616be33616efd41ff67d3c76362c60ca84
+ *        Author: Davidlohr Bueso <[email protected]>
+ *        Date:   Thu Apr 3 14:47:27 2014 -0700
+ *
+ *        commit 1406ec9ba6c65cb69e9243bff07ca3f51e2525e0
+ *        Author: Joonsoo Kim <[email protected]>
+ *        Date:   Thu Apr 3 14:47:26 2014 -0700
+ */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#include "test.h"
+#include "mem.h"
+
+char *TCID = "hugemmap06";
+int TST_TOTAL = 5;
+
+static long hpage_size;
+static long hugepages;
+static long orig_hugepages;
+
+struct mp {
+	void *addr;
+	int sz;
+};
+
+#define ARSZ 50
+
+void setup(void)
+{
+	tst_require_root();
+	check_hugepage();
+
+	hpage_size = read_meminfo("Hugepagesize:") * 1024;
+	orig_hugepages = get_sys_tune("nr_hugepages");
+
+	hugepages = (ARSZ + 1) * TST_TOTAL;
+
+	if (hugepages * read_meminfo("Hugepagesize:") > read_meminfo("MemTotal:"))
+		tst_brkm(TCONF, NULL, "System RAM is not enough to test.");
+
+	set_sys_tune("nr_hugepages", hugepages, 1);
+
+	TEST_PAUSE;
+}
+
+void cleanup(void)
+{
+	set_sys_tune("nr_hugepages", orig_hugepages, 0);
+}
+
+void *thr(void *arg)
+{
+	struct mp *mmap_sz = arg;
+	int i, lim, a, b, c;
+
+	srand(time(NULL));
+	lim = rand() % 10;
+	for (i = 0; i < lim; i++) {
+		a = rand() % mmap_sz->sz;
+		for (c = 0; c <= a; c++) {
+			b = rand() % mmap_sz->sz;
+			*((int *)((char *)mmap_sz->addr + (b * hpage_size))) = rand();
+		}
+	}
+	return NULL;
+}
+
+void do_mmap(void)
+{
+	int i, sz;
+	void *addr, *new_addr;
+	struct mp mmap_sz[ARSZ];
+	pthread_t tid[ARSZ];
+
+	sz = ARSZ;
+	addr = mmap(NULL, sz * hpage_size,
+			PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB,
+			-1, 0);
+

testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c new file +11 -0

+		if (TEST_RETURN)
+			tst_brkm(TBROK | TRERRNO, cleanup,
+					"pthread_create failed");
+
+		new_addr = mmap(addr, (sz - 1) * hpage_size,
+				PROT_READ | PROT_WRITE,
+				MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB | MAP_FIXED,
+				-1, 0);
+
+		if (new_addr == MAP_FAILED) {
+			TEST(pthread_join(tid[i], NULL));

testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c new file +19 -0

+		TEST(pthread_join(tid[i], NULL));
+		if (TEST_RETURN)
+			tst_brkm(TBROK | TRERRNO, cleanup,
+					"pthread_join failed");
+	}
+
+	if (munmap(addr, sz * hpage_size) == -1)
+		tst_brkm(TFAIL | TERRNO, cleanup, "huge munmap failed");
+}
+
+int main(int ac, char **av)
+{
+	int lc, i;
+
+	tst_parse_opts(ac, av, NULL, NULL);
+
+	setup();
+
+	for (lc = 0; TEST_LOOPING(lc); lc++) {

Quoted 5 lines · Li Wang

v1 ---> v2
1. remove the useless include file
2. add huge pages checking in setup()
3. mmap huge pages in reverse order

A paragraph about new version changes must be after:

Signed-off-by Li Wang <[email protected]>

---

as it shouldn't be in a commit message.

Quoted 8 lines

The fix are all these below commits:

        commit f522c3ac00a49128115f99a5fcb95a447601c1c3
        Author:Joonsoo Kim < iamjoonsoo.kim @ lge.com >
        Date:Wed Sep 11 14:21:53 2013 - 0700

This is a more appropriate way of listing commits:

f522c3ac00a4 ("mm, hugetlb: change variable name reservations to resv")

+ * Copyright (c) Author: Herton R. Krzesinski <[email protected]>

Quoted 16 lines

+ * Modify: Li Wang <[email protected]>
+ *

+ *  This program is free software;  you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY;  without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
+ *  the GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program;  if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ *  02110-1301 USA

Quoted 2 lines

+ */

Please change it to

You should have received a copy of the GNU General Public License along with this program. If not, see <http://www.gnu.org/licenses/>.

+ tst_brkm(TBROK | TERRNO, cleanup, "Cannot allocate

testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c line 137

Quoted 5 lines

hugepage");

testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c new file +3 -0

+       }
+
+       for (i = ARSZ - 1; i > 0; i--) {

Why this is done in reverse order?

+ mmap_sz[i].sz = sz;

testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c line 141

Quoted 4 lines

testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c new file +3 -0

+               mmap_sz[i].addr = addr;
+
+               TEST(pthread_create(tid + i, NULL, thr, &mmap_sz[i]));

This is not right, use tid[i] ( = tid + sizeof(pthread_t) * i), not tid + i.

+ }

Review

Quoted 3 lines

(quoted diff) +2 -0

+
+       for (++i; i < ARSZ; i++) {

Could you initialize "i" here explicitly?

+ cleanup();

Quoted 2 lines

+ tst_exit();

Isn't tst_exit() calling cleanup?

Best regards,
Alexey

+ tst_brkm(TBROK | TERRNO, cleanup, "Cannot
allocate hugepage");
+ }
+
+ for (i = ARSZ - 1; i > 0; i--) {

Why this is done in reverse order?

well, as Jan pointed in the previous comments:

"How about allocating the largest area first and then mmaping smaller ones on top of it?

That could prevent situation where first smallest area mmaps a gap between existing libraries/heap/etc. and then larger ones with same start overlap with those - since we write to those areas, bad things could happen."

I think that is very clear to describe the potential issue. since I run the program on some s390x for many times, it is easily to get segment fault. after done in reverse order. the error gone.

here I print the detailed mapings of the program, it show something that.
-----

Program received signal SIGSEGV, Segmentation fault. 0x0000004cf80a68b0 in mmap64 () from /lib64/libc.so.6 (gdb) bt
#0 0x0000004cf80a68b0 in mmap64 () from /lib64/libc.so.6 Cannot access memory at address 0x70

Start Addr End Addr Size Offset objfile
0x80000000 0x80001000 0x1000
0 /root/a.out

          0x80001000         0x80002000     0x1000 
0x1000                              /root/a.out
          0xb7a9b000         0xb7abc000 0x21000          0 [heap]
        0x4cf7f73000       0x4cf7f94000 0x21000          
0                         /lib64/ld-2.12.so <http://ld-2.12.so>
        0x4cf7f94000       0x4cf7f95000     0x1000 
0x20000                         /lib64/ld-2.12.so <http://ld-2.12.so>
        0x4cf7f95000       0x4cf7f96000     0x1000 
0x21000                         /lib64/ld-2.12.so <http://ld-2.12.so>
        0x4cf7f96000       0x4cf7f97000 0x1000          0
        0x4cf7f9d000       0x4cf814d000 0x1b0000          
0                         /lib64/libc-2.12.so <http://libc-2.12.so>
        0x4cf814d000       0x4cf8151000     0x4000 
0x1af000                         /lib64/libc-2.12.so <http://libc-2.12.so>
        0x4cf8151000       0x4cf8152000     0x1000 
0x1b3000                         /lib64/libc-2.12.so <http://libc-2.12.so>
        0x4cf8152000       0x4cf8157000 0x5000          0
        0x4cf8159000       0x4cf8176000 0x1d000          
0                         /lib64/libpthread-2.12.so 
<http://libpthread-2.12.so>
        0x4cf8176000       0x4cf8177000     0x1000 
0x1c000                         /lib64/libpthread-2.12.so 
<http://libpthread-2.12.so>
        0x4cf8177000       0x4cf8178000     0x1000 

0x1d000 /lib64/libpthread-2.12.so
<http://libpthread-2.12.so>

+ mmap_sz[i].sz = sz;
+ mmap_sz[i].addr = addr;
+
+ TEST(pthread_create(tid + i, NULL, thr,
&mmap_sz[i]));

This is not right, use tid[i] ( = tid + sizeof(pthread_t) * i), not tid + i.

sorry, probably you want say this:

        (pthread_t *)((char *)tid + sizeof(pthread_t) * i)

but I think the original way is right.

-----------

# cat a.c
#include <stdio.h>
int main()
{
    int a[2];

    *a = 'a';
    *(a + 1) = 'b';

    printf("a[1] = %d\n", a[1]);

    printf("&a[0] = %p\n", a);
    printf("&a[1] = %p\n", a + 1);
}

--------

# ./a.out
a[1] = 98
&a[0] = 0x3ffffe25f18
&a[1] = 0x3ffffe25f1c

+ }
+
+ for (++i; i < ARSZ; i++) {

Could you initialize "i" here explicitly?

ok, no problem.

+ cleanup();
+ tst_exit();

Isn't tst_exit() calling cleanup?

hmm, I checked the /lib/tst_res.c and got the function:

----
void tst_exit(void)

{
    pthread_mutex_lock(&tmutex);

#if DEBUG
    printf("IN tst_exit\n");
    fflush(stdout);
    fflush(stdout);

#endif

    /* Call tst_flush() flush any output in the buffer. */
    tst_flush();

    /* Mask out TINFO result from the exit status. */
    exit(T_exitval & ~TINFO);
}

I didn't saw there anyplace call the cleanup() function. :(

Hi,
On 11/26/2015 03:25 PM, Li Wang wrote:
...
OK, but I meant that either we need to remove "sz" variable and use "i * hpage_size" or
make it straightforward as "i" doesn't influence on the actual size passed to mmap and threads.

So that both of the loops looks the same.

Also to use the full range of declared arrays, we should do as follows or declare them as ARSZ - 1:

struct mp mmap_sz[ARSZ];
pthread_t tid[ARSZ];

sz = ARSZ + 1;

...

for (i = 0; i < ARSZ; ++i, --sz) {
    mmap_sz[i].sz = sz;
    mmap_sz[i].addr = addr;
    ...
}

for (i = 0; i < ARSZ; ++i) {
    pthread_join(...);

}

Quoted 10 lines

+ mmap_sz[i].sz = sz;

testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c line 141

Quoted 4 lines

testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c new file +3 -0

+               mmap_sz[i].addr = addr;
+
+               TEST(pthread_create(tid + i, NULL, thr, &mmap_sz[i]));

This is not right, use tid[i] ( = tid + sizeof(pthread_t) * i), not tid + i.

sorry, probably you want say this:

        (pthread_t *)((char *)tid + sizeof(pthread_t) * i)

but I think the original way is right.

Yes, you are right, sorry for misleading you.

I would recommend to use the same style in one function, so

TEST(pthread_create(tid + i, NULL, thr, mmap_sz + i)); or
TEST(pthread_create(&tid[i], NULL, thr, &mmap_sz[i]));

...

    /* Call tst_flush() flush any output in the buffer. */
    tst_flush();

    /* Mask out TINFO result from the exit status. */
    exit(T_exitval & ~TINFO);
}

I didn't saw there anyplace call the cleanup() function. :(

Right, I've read the mail thread recently about changing cleanup() to be declared as a signal handler before
and somehow thought it is already implemented in LTP...

Thanks,
Alexey

testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c line 141

Quoted 4 lines

testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c new file +4 -0

+		mmap_sz[i].sz = sz;
+		mmap_sz[i].addr = addr;
+
+		TEST(pthread_create(tid + i, NULL, thr, &mmap_sz[i]));

This is not right, use tid[i] ( = tid + sizeof(pthread_t) * i), not tid + i.