0

[PA] Make De/RecommitSystemPages consistent between Fuchsia and POSIX

De/RecommitSystemPages() have similar properties under POSIX and
Fuchsia, since both use over-commit, but the API implementations differ
in their optimizations. Update the Fuchsia implementations with the same
optimizations as provided for POSIX.

Bug: 766882
Change-Id: I34b98db27ed85543e53b3231ec118d0a2409b454
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2558122
Commit-Queue: Bartek Nowierski <bartekn@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Auto-Submit: Bartek Nowierski <bartekn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834172}
This commit is contained in:
Bartek Nowierski
2020-12-07 12:37:53 +00:00
committed by Chromium LUCI CQ
parent a5e9783262
commit d434f3cac0
3 changed files with 19 additions and 19 deletions

@ -31,7 +31,8 @@ enum PageAccessibilityDisposition {
// Enforces permission update (Decommit will set to PageInaccessible;
// Recommit will set to whatever was requested, other than PageInaccessible).
PageUpdatePermissions,
// Will not update permissions, if the platform supports that (POSIX only).
// Will not update permissions, if the platform supports that (POSIX & Fuchsia
// only).
PageKeepPermissionsIfPossible,
};
@ -105,8 +106,8 @@ BASE_EXPORT void SetSystemPagesAccess(
//
// |accessibility_disposition| allows to specify whether the pages should be
// made inaccessible (PageUpdatePermissions), or left as is
// (PageKeepPermissionsIfPossible, POSIX only). The latter should only be used
// as an optimization if you really know what you're doing.
// (PageKeepPermissionsIfPossible, POSIX & Fuchsia only). The latter should only
// be used as an optimization if you really know what you're doing.
// TODO(bartekn): Ideally, all callers should use PageUpdatePermissions,
// for better security, but that may lead to a perf regression. Tracked at
// http://crbug.com/766882.
@ -122,7 +123,7 @@ BASE_EXPORT void SetSystemPagesAccess(
//
// Note: "Committed memory" is a Windows Memory Subsystem concept that ensures
// processes will not fault when touching a committed memory region. There is
// no analogue in the POSIX memory API where virtual memory pages are
// no analogue in the POSIX & Fuchsia memory API where virtual memory pages are
// best-effort allocated resources on the first touch. If PageUpdatePermissions
// disposition is used, this API behaves in a platform-agnostic way by
// simulating the Windows "decommit" state by both discarding the region
@ -142,8 +143,8 @@ BASE_EXPORT void DecommitSystemPages(
//
// |accessibility_disposition| allows to specify whether the page permissions
// should be set to |page_accessibility| (PageUpdatePermissions), or left as is
// (PageKeepPermissionsIfPossible, POSIX only). The latter can only be used if
// the pages were previously accessible and decommitted with
// (PageKeepPermissionsIfPossible, POSIX & Fuchsia only). The latter can only be
// used if the pages were previously accessible and decommitted with
// PageKeepPermissionsIfPossible. It is ok, however, to recommit with
// PageUpdatePermissions even if pages were decommitted with
// PageKeepPermissionsIfPossible (merely losing an optimization).

@ -184,10 +184,9 @@ void DecommitSystemPagesInternal(
void* address,
size_t length,
PageAccessibilityDisposition accessibility_disposition) {
// TODO(bartekn): Ignoring accessibility_disposition preserves the behavior
// the API had since its conception, but consider similar optimization to
// POSIX.
SetSystemPagesAccessInternal(address, length, PageInaccessible);
if (accessibility_disposition == PageUpdatePermissions) {
SetSystemPagesAccess(address, length, PageInaccessible);
}
// TODO(https://crbug.com/1022062): Review whether this implementation is
// still appropriate once DiscardSystemPagesInternal() migrates to a "lazy"
@ -200,10 +199,12 @@ void RecommitSystemPagesInternal(
size_t length,
PageAccessibilityConfiguration accessibility,
PageAccessibilityDisposition accessibility_disposition) {
// TODO(bartekn): Ignoring accessibility_disposition preserves the behavior
// the API had since its conception, but consider similar optimization to
// POSIX.
SetSystemPagesAccessInternal(address, length, accessibility);
// On Fuchsia systems, the caller needs to simply read the memory to recommit
// it. However, if decommit changed the permissions, recommit has to change
// them back.
if (accessibility_disposition == PageUpdatePermissions) {
SetSystemPagesAccess(address, length, accessibility);
}
}
} // namespace base

@ -268,11 +268,9 @@ void RecommitSystemPagesInternal(
size_t length,
PageAccessibilityConfiguration accessibility,
PageAccessibilityDisposition accessibility_disposition) {
// On POSIX systems, the caller need simply read the memory to recommit it.
// This has the correct behavior because the API requires the permissions to
// be the same as before decommitting and all configurations can read.
// However, if decommit changed the permissions, recommit has to change them
// back.
// On POSIX systems, the caller needs to simply read the memory to recommit
// it. However, if decommit changed the permissions, recommit has to change
// them back.
if (accessibility_disposition == PageUpdatePermissions) {
SetSystemPagesAccess(address, length, accessibility);
}