From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Stancek Date: Thu, 17 Dec 2015 05:40:51 -0500 (EST) Subject: [LTP] [PATCH v3] hugetlb/hugemmap: add new testcase hugemmap06.c In-Reply-To: <1448618434-6780-1-git-send-email-liwang@redhat.com> References: <1448618434-6780-1-git-send-email-liwang@redhat.com> Message-ID: <2096168533.29677153.1450348851398.JavaMail.zimbra@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: ltp@lists.linux.it ----- Original Message ----- > From: "Li Wang" > To: "alexey kodanev" , jstancek@redhat.com > Cc: ltp@lists.linux.it > Sent: Friday, 27 November, 2015 11:00:34 AM > Subject: [PATCH v3] hugetlb/hugemmap: add new testcase hugemmap06.c > > 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: > f522c3ac00(mm, hugetlb: change variable name reservations to resv) > 9119a41e90(mm, hugetlb: unify region structure handling) > 7b24d8616b(mm, hugetlb: fix race in region tracking) > 1406ec9ba6(mm, hugetlb: improve, cleanup resv_map parameters) > > Signed-off-by: Li Wang > --- > > Notes: > v2 --> v3 > a. include hugetlb.h > b. listing commits in a short way > c. update the license to GPLv3 > d. make tid[ARSZ] and for(...) simple > e. keep &tid[i] the same style in one function > > runtest/hugetlb | 1 + > testcases/kernel/mem/.gitignore | 1 + > testcases/kernel/mem/hugetlb/hugemmap/Makefile | 2 + > testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c | 180 > +++++++++++++++++++++ > 4 files changed, 184 insertions(+) > create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c > > diff --git a/runtest/hugetlb b/runtest/hugetlb > index 2e9f215..ac24513 100644 > --- a/runtest/hugetlb > +++ b/runtest/hugetlb > @@ -2,6 +2,7 @@ hugemmap01 hugemmap01 > hugemmap02 hugemmap02 > hugemmap04 hugemmap04 > hugemmap05 hugemmap05 > +hugemmap06 hugemmap06 > hugemmap05_1 hugemmap05 -m > hugemmap05_2 hugemmap05 -s > hugemmap05_3 hugemmap05 -s -m > diff --git a/testcases/kernel/mem/.gitignore > b/testcases/kernel/mem/.gitignore > index 4702377..ac8e6d8 100644 > --- a/testcases/kernel/mem/.gitignore > +++ b/testcases/kernel/mem/.gitignore > @@ -3,6 +3,7 @@ > /hugetlb/hugemmap/hugemmap02 > /hugetlb/hugemmap/hugemmap04 > /hugetlb/hugemmap/hugemmap05 > +/hugetlb/hugemmap/hugemmap06 > /hugetlb/hugeshmat/hugeshmat01 > /hugetlb/hugeshmat/hugeshmat02 > /hugetlb/hugeshmat/hugeshmat03 > diff --git a/testcases/kernel/mem/hugetlb/hugemmap/Makefile > b/testcases/kernel/mem/hugetlb/hugemmap/Makefile > index f51f6b9..2473aca 100644 > --- a/testcases/kernel/mem/hugetlb/hugemmap/Makefile > +++ b/testcases/kernel/mem/hugetlb/hugemmap/Makefile > @@ -25,3 +25,5 @@ top_srcdir ?= ../../../../.. > include $(top_srcdir)/include/mk/testcases.mk > include $(abs_srcdir)/../Makefile.inc > include $(top_srcdir)/include/mk/generic_leaf_target.mk > + > +hugemmap06: CFLAGS+=-pthread > diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c > b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c > new file mode 100644 > index 0000000..400d28a > --- /dev/null > +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap06.c > @@ -0,0 +1,180 @@ > +/* > + * Copyright (c) 2015 Red Hat, Inc. > + * > + * 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 3 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, see . > + */ > + > +/* > + * 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: > + * f522c3ac00(mm, hugetlb: change variable name reservations to resv) > + * 9119a41e90(mm, hugetlb: unify region structure handling) > + * 7b24d8616b(mm, hugetlb: fix race in region tracking) > + * 1406ec9ba6(mm, hugetlb: improve, cleanup resv_map parameters) > + * > + * AUTHOR: > + * Herton R. Krzesinski > + * Li Wang > + */ > + > +#define _GNU_SOURCE > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "test.h" > +#include "mem.h" > +#include "hugetlb.h" > + > +char *TCID = "hugemmap06"; > +int TST_TOTAL = 5; > + > +static long hpage_size; > +static long 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(); Hi, v3 looks OK to me, few nits are noted below: If you make "addr" "char *" one cast can go away. And from what I understand, important is to touch the page, so writing char instead of int should work too: *(mmap_sz->addr + b * hpage_size) = rand(); I tried the above and I could still trigger the panic. > + } > + } > + return NULL; > +} > + > +void do_mmap(void) > +{ > + int i, sz; > + void *addr, *new_addr; > + struct mp mmap_sz[ARSZ]; > + pthread_t tid[ARSZ]; > + > + sz = ARSZ + 1; This could go on same line where you declared sz. > + addr = mmap(NULL, sz * hpage_size, > + PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB, > + -1, 0); > + > + if (addr == MAP_FAILED) { > + if (errno == ENOMEM) { > + tst_brkm(TCONF, cleanup, > + "Cannot allocate hugepage, memory too fragmented?"); > + } > + > + tst_brkm(TBROK | TERRNO, cleanup, "Cannot allocate hugepage"); > + } > + > + for (i = 0; i < ARSZ; ++i, --sz) { > + mmap_sz[i].sz = sz; > + mmap_sz[i].addr = addr; > + > + TEST(pthread_create(&tid[i], NULL, thr, &mmap_sz[i])); > + 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)); What is the purpose of pthread_join here? > + if (TEST_RETURN) > + tst_brkm(TBROK | TRERRNO, cleanup, > + "pthread_join failed"); > + tst_brkm(TFAIL | TERRNO, cleanup, "mremap failed"); "mremap" looks like typo Regards, Jan > + } > + addr = new_addr; > + } > + > + for (i = 0; i < ARSZ; ++i) { > + 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++) { > + tst_count = 0; > + > + for (i = 0; i < TST_TOTAL; i++) > + do_mmap(); > + > + tst_resm(TPASS, "No regression found."); > + } > + > + cleanup(); > + tst_exit(); > +} > -- > 1.8.3.1 > >