commit | b44e46bb047d136bc8977497b6fc2a9f08740321 | [log] [tgz] |
---|---|---|
author | Peter Collingbourne <pcc@google.com> | Tue Jun 08 12:57:27 2021 +1000 |
committer | Hridya Valsaraju <hridya@google.com> | Tue Jun 15 19:33:15 2021 +0000 |
tree | 005da71967c83060b64da1908d7a1f3ad72675aa | |
parent | 54e7412d4ff9af8fc9d2b5c792e0ff6e39dcc045 [diff] |
FROMGIT: mm: improve mprotect(R|W) efficiency on pages referenced once In the Scudo memory allocator [1] we would like to be able to detect use-after-free vulnerabilities involving large allocations by issuing mprotect(PROT_NONE) on the memory region used for the allocation when it is deallocated. Later on, after the memory region has been "quarantined" for a sufficient period of time we would like to be able to use it for another allocation by issuing mprotect(PROT_READ|PROT_WRITE). Before this patch, after removing the write protection, any writes to the memory region would result in page faults and entering the copy-on-write code path, even in the usual case where the pages are only referenced by a single PTE, harming performance unnecessarily. Make it so that any pages in anonymous mappings that are only referenced by a single PTE are immediately made writable during the mprotect so that we can avoid the page faults. This program shows the critical syscall sequence that we intend to use in the allocator: #include <string.h> #include <sys/mman.h> enum { kSize = 131072 }; int main(int argc, char **argv) { char *addr = (char *)mmap(0, kSize, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); for (int i = 0; i != 100000; ++i) { memset(addr, i, kSize); mprotect((void *)addr, kSize, PROT_NONE); mprotect((void *)addr, kSize, PROT_READ | PROT_WRITE); } } The effect of this patch on the above program was measured on a DragonBoard 845c by taking the median real time execution time of 10 runs. Before: 2.94s After: 0.66s The effect was also measured using one of the microbenchmarks that we normally use to benchmark the allocator [2], after modifying it to make the appropriate mprotect calls [3]. With an allocation size of 131072 bytes to trigger the allocator's "large allocation" code path the per-iteration time was measured as follows: Before: 27450ns After: 6010ns This patch means that we do more work during the mprotect call itself in exchange for less work when the pages are accessed. In the worst case, the pages are not accessed at all. The effect of this patch in such cases was measured using the following program: #include <string.h> #include <sys/mman.h> enum { kSize = 131072 }; int main(int argc, char **argv) { char *addr = (char *)mmap(0, kSize, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); memset(addr, 1, kSize); for (int i = 0; i != 100000; ++i) { #ifdef PAGE_FAULT memset(addr + (i * 4096) % kSize, i, 4096); #endif mprotect((void *)addr, kSize, PROT_NONE); mprotect((void *)addr, kSize, PROT_READ | PROT_WRITE); } } With PAGE_FAULT undefined (0 pages touched after removing write protection) the median real time execution time of 100 runs was measured as follows: Before: 0.330260s After: 0.338836s With PAGE_FAULT defined (1 page touched) the measurements were as follows: Before: 0.438048s After: 0.355661s So it seems that even with a single page fault the new approach is faster. I saw similar results if I adjusted the programs to use a larger mapping size. With kSize = 1048576 I get these numbers with PAGE_FAULT undefined: Before: 1.428988s After: 1.512016s i.e. around 5.5%. And these with PAGE_FAULT defined: Before: 1.518559s After: 1.524417s i.e. about the same. What I think we may conclude from these results is that for smaller mappings the advantage of the previous approach, although measurable, is wiped out by a single page fault. I think we may expect that there should be at least one access resulting in a page fault (under the previous approach) after making the pages writable, since the program presumably made the pages writable for a reason. For larger mappings we may guesstimate that the new approach wins if the density of future page faults is > 0.4%. But for the mappings that are large enough for density to matter (not just the absolute number of page faults) it doesn't seem like the increase in mprotect latency would be very large relative to the total mprotect execution time. Link: https://lkml.kernel.org/r/20210527190453.1259020-1-pcc@google.com Link: https://linux-review.googlesource.com/id/I98d75ef90e20330c578871c87494d64b1df3f1b8 Link: [1] https://source.android.com/devices/tech/debug/scudo Link: [2] https://cs.android.com/android/platform/superproject/+/master:bionic/benchmarks/stdlib_benchmark.cpp;l=53;drc=e8693e78711e8f45ccd2b610e4dbe0b94d551cc9 Link: [3] https://github.com/pcc/llvm-project/commit/scudo-mprotect-secondary2 Signed-off-by: Peter Collingbourne <pcc@google.com> Reviewed-by: Peter Xu <peterx@redhat.com> Cc: Kostya Kortchinsky <kostyak@google.com> Cc: Evgenii Stepanov <eugenis@google.com> Cc: Andrea Arcangeli <aarcange@redhat.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au> (cherry picked from commit e2037f9c0c61ed6964bb1291292ae88f073a100c https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git akpm) [pcc: squashed v4->v5 diff which appeared as a separate commit: ec7563ea9f6a470e9bb532b024ce29d9474daf24] Change-Id: Ic3994b2ec914d3f62f95c1ef338986e350e69e36 Bug: 191165850
BEST: Make all of your changes to upstream Linux. If appropriate, backport to the stable releases. These patches will be merged automatically in the corresponding common kernels. If the patch is already in upstream Linux, post a backport of the patch that conforms to the patch requirements below.
EXPORT_SYMBOL_GPL()
require an in-tree modular driver that uses the symbol -- so include the new driver or changes to an existing driver in the same patchset as the export.LESS GOOD: Develop your patches out-of-tree (from an upstream Linux point-of-view). Unless these are fixing an Android-specific bug, these are very unlikely to be accepted unless they have been coordinated with kernel-team@android.com. If you want to proceed, post a patch that conforms to the patch requirements below.
script/checkpatch.pl
UPSTREAM:
, BACKPORT:
, FROMGIT:
, FROMLIST:
, or ANDROID:
.Change-Id:
tag (see https://gerrit-review.googlesource.com/Documentation/user-changeid.html)Bug:
tag.Signed-off-by:
tag by the author and the submitterAdditional requirements are listed below based on patch type
UPSTREAM:
, BACKPORT:
UPSTREAM:
.(cherry picked from commit ...)
lineimportant patch from upstream This is the detailed description of the important patch Signed-off-by: Fred Jones <fred.jones@foo.org>
- then Joe Smith would upload the patch for the common kernel as
UPSTREAM: important patch from upstream This is the detailed description of the important patch Signed-off-by: Fred Jones <fred.jones@foo.org> Bug: 135791357 Change-Id: I4caaaa566ea080fa148c5e768bb1a0b6f7201c01 (cherry picked from commit c31e73121f4c1ec41143423ac6ce3ce6dafdcec1) Signed-off-by: Joe Smith <joe.smith@foo.org>
BACKPORT:
instead of UPSTREAM:
.UPSTREAM:
(cherry picked from commit ...)
lineBACKPORT: important patch from upstream This is the detailed description of the important patch Signed-off-by: Fred Jones <fred.jones@foo.org> Bug: 135791357 Change-Id: I4caaaa566ea080fa148c5e768bb1a0b6f7201c01 (cherry picked from commit c31e73121f4c1ec41143423ac6ce3ce6dafdcec1) [joe: Resolved minor conflict in drivers/foo/bar.c ] Signed-off-by: Joe Smith <joe.smith@foo.org>
FROMGIT:
, FROMLIST:
,FROMGIT:
(cherry picked from commit <sha1> <repo> <branch>)
. This must be a stable maintainer branch (not rebased, so don't use linux-next
for example).BACKPORT: FROMGIT:
important patch from upstream This is the detailed description of the important patch Signed-off-by: Fred Jones <fred.jones@foo.org>
- then Joe Smith would upload the patch for the common kernel as
FROMGIT: important patch from upstream This is the detailed description of the important patch Signed-off-by: Fred Jones <fred.jones@foo.org> Bug: 135791357 (cherry picked from commit 878a2fd9de10b03d11d2f622250285c7e63deace https://git.kernel.org/pub/scm/linux/kernel/git/foo/bar.git test-branch) Change-Id: I4caaaa566ea080fa148c5e768bb1a0b6f7201c01 Signed-off-by: Joe Smith <joe.smith@foo.org>
FROMLIST:
Link:
tag with a link to the submittal on lore.kernel.orgBug:
tag with the Android bug (required for patches not accepted into a maintainer tree)BACKPORT: FROMLIST:
FROMLIST: important patch from upstream This is the detailed description of the important patch Signed-off-by: Fred Jones <fred.jones@foo.org> Bug: 135791357 Link: https://lore.kernel.org/lkml/20190619171517.GA17557@someone.com/ Change-Id: I4caaaa566ea080fa148c5e768bb1a0b6f7201c01 Signed-off-by: Joe Smith <joe.smith@foo.org>
ANDROID:
ANDROID:
Fixes:
tag that cites the patch with the bugANDROID: fix android-specific bug in foobar.c This is the detailed description of the important fix Fixes: 1234abcd2468 ("foobar: add cool feature") Change-Id: I4caaaa566ea080fa148c5e768bb1a0b6f7201c01 Signed-off-by: Joe Smith <joe.smith@foo.org>
ANDROID:
Bug:
tag with the Android bug (required for android-specific features)