https://bugs.kde.org/show_bug.cgi?id=514297
--- Comment #23 from [email protected] --- Created attachment 190530 --> https://bugs.kde.org/attachment.cgi?id=190530&action=edit proposed patch Updated patch trying to address all the review comments above. I've also checked this update to a topic branch users/mcermak/try-bug514297 and got buildbot test results for it: https://builder.sourceware.org/buildbot/#/builders/244/builds/139 > if you keep an fd around (please use Int instead of SysRes), then make sure > to call VG_(safe_fd) Done > > I've used the following systemtap script to monitor the madvise syscall > > system-wide: > > > > # stap -ve 'probe syscall.madvise {printdln(" ", advice_str, execname())}' > > Would be interesting to see the print_ustack (); for that to know what user > space code decides to use guard pages. I've triggered the guard page installation like this: f44 x86_64 # python3 -c 'import time, threading; [threading.Thread(None, lambda: time.sleep(1)).start() for x in range(1)]' Here is the stap output: # stap -d kernel -d /usr/lib64/libc.so.6 -d /usr/lib64/libpython3.14.so.1.0 -ve 'probe syscall.madvise {printdln(" ", advice_str, execname()); print_ustack(ubacktrace());}' Pass 1: parsed user script and 486 library scripts using 374948virt/132636res/17572shr/149148data kb, in 250usr/120sys/139real ms. Pass 2: analyzed script: 3 probes, 12 functions, 98 embeds, 4 globals using 501592virt/260972res/19248shr/275792data kb, in 2410usr/180sys/2612real ms. Pass 3: translated to C into "/tmp/stapKm0bBn/stap_a336501c0a7864a8b6be6371f4b58ca3_69324_src.c" using 502012virt/266600res/24816shr/275928data kb, in 940usr/160sys/2507real ms. Pass 4: compiled C into "stap_a336501c0a7864a8b6be6371f4b58ca3_69324.ko" in 11420usr/1240sys/7124real ms. Pass 5: starting run. 0x66 python3 0x7f17a96f8c0b : __madvise+0xb/0x30 [/usr/lib64/libc.so.6] 0x7f17a9679633 : pthread_create@@GLIBC_2.34+0xb23/0x1150 [/usr/lib64/libc.so.6] 0x7f17a9b1d10c : do_start_joinable_thread.constprop.0+0xbc/0x120 [/usr/lib64/libpython3.14.so.1.0] 0x7f17a9b1ce52 : PyThread_start_joinable_thread+0x32/0x60 [/usr/lib64/libpython3.14.so.1.0] 0x7f17a9b1cbfb : do_start_new_thread.lto_priv.0+0x19b/0x230 [/usr/lib64/libpython3.14.so.1.0] [ ... stuff deleted ... ] 0x7f17a9a8cac0 : Py_RunMain+0x540/0x660 [/usr/lib64/libpython3.14.so.1.0] 0x7f17a9a868ac : Py_BytesMain+0x3c/0x60 [/usr/lib64/libpython3.14.so.1.0] 0x7f17a9608681 : __libc_start_call_main+0x81/0x110 [/usr/lib64/libc.so.6] 0x7f17a9608798 : __libc_start_main@GLIBC_2.2.5+0x88/0x150 [/usr/lib64/libc.so.6] 0x5588748003d5 : 0x5588748003d5 [/usr/bin/python3.14+0x3d5/0x4000] WARNING: Missing unwind data for a module, rerun with 'stap -d /usr/bin/python3.14' MADV_DONTNEED Thread-1 (<lamb 0x7f17a96f8c0b : __madvise+0xb/0x30 [/usr/lib64/libc.so.6] 0x7f17a9678aac : start_thread+0x54c/0x5b0 [/usr/lib64/libc.so.6] 0x7f17a96fcc0c : __GI___clone3+0x2c/0x60 [/usr/lib64/libc.so.6] # > You still would like to use /proc/self/pagemaps for sanity checks though > (--sanity-level=3 testing) Done: https://sourceware.org/git/?p=valgrind.git;a=blob;f=coregrind/m_aspacemgr/aspacemgr-linux.c;h=7b449fd7a2a9204ffd6da7b9ca81e1847e08ef24;hb=8727ba9b9edc9ebb053bb63e8eb8c8c49287f77a#l1162 > Note that significant effort was spent to make is_valid_for fast. > ... > So, ideally, checking for guard page should be fast either I've reimplemented the bookkeeping mechanism here to make it perform better: https://sourceware.org/git/?p=valgrind.git;a=blob;f=coregrind/m_aspacemgr/aspacemgr-linux.c;h=7b449fd7a2a9204ffd6da7b9ca81e1847e08ef24;hb=8727ba9b9edc9ebb053bb63e8eb8c8c49287f77a#l1078 The new implementation doesn't check /proc/self/pagemap at all by default. Instead it has a new array guardpages[VG_N_THREADS] keeping an ordered list of start addresses of madvise guard pages. The array is defined together with a description explaining how its max size was set. Function is_guarded() tries to quickly answer the question whether an address belongs to a guard page. > It would be better if cl_pagemap_fd was only defined/used in > coregrind/m_aspacemgr/aspacemgr-linux.c You could set it up in > VG_(am_startup). I think we should avoid that ../pub_core_clientstate.h > include. Done. To make it work I had to move setup_file_descriptors() towards the beginning of valgrind_main(), before the address space manager is initialized. Without that VG_(safe_fd)() would fail the vg_assert(VG_(fd_hard_limit) != -1); > It might be simpler (less duplicate code) to have just notify_madv_guard ( > Addr start, SizeT len, Bool install ) Right! Fixed. > notify_core_and_tool_of_madv_guard should be called in the POST(sys_madvise) > handler Fixed. > I think the safe_to_deref before the PRE_MEM_RASCIIZ might be a > separate/generic issue Right. I've kept it a part of the patch for now to simplify testing. Moreover my patch has one more unrelated bit (initialization of vki_sigset_t saved in m_syswrap/syswrap-main.c) silencing my compiler complaints: https://sourceware.org/git/?p=valgrind.git;a=blob;f=coregrind/m_syswrap/syswrap-main.c;h=7a1bbb40fa22ac33f4d12ac577ab75fd7979f4dc;hb=refs/heads/users/mcermak/try-bug514297#l354 > Could you show how these programs use the guard pages ? Here is how a guard page is created for each new thread: f44 x86_64 # valgrind -d python3 -c 'import time, threading; [threading.Thread(None, lambda: time.sleep(1)).start() for x in range(4)]' 2> >(grep guard) --98531:1: aspacem installing guard pages (addr=0x5a8a000, len=0x1000) --98531:1: aspacem installing guard pages (addr=0x628f000, len=0x1000) --98531:1: aspacem installing guard pages (addr=0x6a94000, len=0x1000) --98531:1: aspacem installing guard pages (addr=0x7299000, len=0x1000) f44 x86_64 # > Could you investigate how things work with > memcheck/tests/descr_belowsp.vgtest ? Seems like the testcase is missing "In stack guard protected page" in the output. This is supposed to come from coregrind/m_addrinfo.c:242: 236 if (seg != NULL && seg->kind == SkAnonC 237 && !seg->hasR && !seg->hasW && !seg->hasX) { 238 /* This looks a plausible guard page. Check if a is close to 239 the start of stack (lowest byte). */ 240 tid = find_tid_with_stack_containing (VG_PGROUNDUP(a+1)); 241 if (tid != VG_INVALID_THREADID) 242 stackPos = StackPos_guard_page; ... this apparently is a place where the new madvise guard page support should be taken into account as well. May I get this new update reviewed / commented pretty please? -- You are receiving this mail because: You are watching all bug changes.
