[kernel][vm] Forbid zero length pins
Pinning a zero length range results in potentially inconsistent
semantics compared with non zero sized pin. For example, a non-zero
length pin will prevent a VMO from being resized smaller than the pin
offset. Whereas a zero length pin does not prevent this. This might be
okay, but then attemping to Unpin that zero length range can fail, as
it is outside the VMO.
Instead of forbidding zero length pins, unpin could instead be changed
to always consider a zero length unpin to be success. This is only
partially correct though, as we really should only consider a zero
length unpin to be correct if there was an equivalent zero length pin.
Unfortunately tracking this would require additional metadata to be
allocated. This issue, and the fact that it still leaves the
inconsistent resize semantics is why this option was not taken.
In the autocall cleanup of CommitRangeInternal any pinned portion is
automatically unpinned. Previously this would just always call Unpin,
even if the range to Unpin was zero. The range could be zero if the
VMO was resized before the first page was pinned. This can only
happen in the case of a user pager backed VMO where the resize
happens whilst blocked waiting for the page to populate.
The current user facing API already forbids zero length pin requests,
so this change is not visible outside of some existing in kernel
unittests that were updated.
Bug: 53711
Change-Id: Iabf5022bdd2404e25a3253871be90328982dc9d1
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/404534
Commit-Queue: Adrian Danis <adanis@google.com>
Reviewed-by: Rasha Eqbal <rashaeqbal@google.com>
Reviewed-by: Nick Maniscalco <maniscalco@google.com>
Testability-Review: Rasha Eqbal <rashaeqbal@google.com>
Testability-Review: Nick Maniscalco <maniscalco@google.com>
diff --git a/zircon/kernel/vm/include/vm/vm_object.h b/zircon/kernel/vm/include/vm/vm_object.h
index f831520..20f4a9f 100644
--- a/zircon/kernel/vm/include/vm/vm_object.h
+++ b/zircon/kernel/vm/include/vm/vm_object.h
@@ -106,6 +106,7 @@
virtual zx_status_t CommitRange(uint64_t offset, uint64_t len) { return ZX_ERR_NOT_SUPPORTED; }
// find physical pages to back the range of the object and pin them.
+ // |len| must be non-zero.
virtual zx_status_t CommitRangePinned(uint64_t offset, uint64_t len) {
return ZX_ERR_NOT_SUPPORTED;
}
diff --git a/zircon/kernel/vm/vm_object_paged.cc b/zircon/kernel/vm/vm_object_paged.cc
index d901f76..5859292 100644
--- a/zircon/kernel/vm/vm_object_paged.cc
+++ b/zircon/kernel/vm/vm_object_paged.cc
@@ -1927,6 +1927,15 @@
// otherwise we can commit a partial range.
uint64_t new_len = len;
if (pin) {
+ // If pinning we explicitly forbid zero length pins as we cannot guarantee consistent semantics.
+ // For example pinning a zero length range outside the range of the VMO is an error, and so
+ // pinning a zero length range inside the vmo and then resizing the VMO smaller than the pin
+ // region should also be an error. To enforce this without having to have new metadata to track
+ // zero length pin regions is to just forbid them. Note that the user entry points for pinning
+ // already forbid zero length ranges.
+ if (len == 0) {
+ return ZX_ERR_INVALID_ARGS;
+ }
// verify that the range is within the object
if (unlikely(!InRange(offset, len, size_))) {
return ZX_ERR_OUT_OF_RANGE;
@@ -1935,11 +1944,10 @@
if (!TrimRange(offset, len, size_, &new_len)) {
return ZX_ERR_OUT_OF_RANGE;
}
- }
-
- // was in range, just zero length
- if (new_len == 0) {
- return ZX_OK;
+ // was in range, just zero length
+ if (new_len == 0) {
+ return ZX_OK;
+ }
}
// Child slices of VMOs are currently not resizable, nor can they be made
@@ -2008,7 +2016,12 @@
// Should any errors occur we need to unpin everything.
auto pin_cleanup = fbl::MakeAutoCall([this, original_offset = offset, &offset, pin]() {
- if (pin) {
+ // Regardless of any resizes or other things that may have happened any pinned pages *must*
+ // still be within a valid range, and so we know Unpin should succeed. The edge case is if we
+ // had failed to pin *any* pages and so our original offset may be outside the current range of
+ // the vmo. Additionally, as pinning a zero length range is invalid, so is unpinning, and so we
+ // must avoid.
+ if (pin && offset > original_offset) {
AssertHeld(*lock());
UnpinLocked(original_offset, offset - original_offset);
}
@@ -2043,11 +2056,10 @@
pin_cleanup.cancel();
return ZX_OK;
}
- }
-
- if (new_len == 0) {
- pin_cleanup.cancel();
- return ZX_OK;
+ if (new_len == 0) {
+ pin_cleanup.cancel();
+ return ZX_OK;
+ }
}
end = ROUNDUP_PAGE_SIZE(offset + new_len);
@@ -2531,10 +2543,8 @@
// verify that the range is within the object
ASSERT(InRange(offset, len, size_));
-
- if (unlikely(len == 0)) {
- return;
- }
+ // forbid zero length unpins as zero length pins return errors.
+ ASSERT(len != 0);
if (is_slice()) {
uint64_t parent_offset;
diff --git a/zircon/kernel/vm/vm_unittest.cc b/zircon/kernel/vm/vm_unittest.cc
index 7362717..18edd11 100644
--- a/zircon/kernel/vm/vm_unittest.cc
+++ b/zircon/kernel/vm/vm_unittest.cc
@@ -1229,9 +1229,7 @@
status = vmo->CommitRangePinned(PAGE_SIZE, alloc_size);
EXPECT_EQ(ZX_ERR_OUT_OF_RANGE, status, "pinning out of range\n");
status = vmo->CommitRangePinned(PAGE_SIZE, 0);
- EXPECT_EQ(ZX_OK, status, "pinning range of len 0\n");
- status = vmo->CommitRangePinned(alloc_size + PAGE_SIZE, 0);
- EXPECT_EQ(ZX_ERR_OUT_OF_RANGE, status, "pinning out-of-range of len 0\n");
+ EXPECT_EQ(ZX_ERR_INVALID_ARGS, status, "pinning range of len 0\n");
status = vmo->CommitRangePinned(PAGE_SIZE, 3 * PAGE_SIZE);
EXPECT_EQ(ZX_OK, status, "pinning committed range\n");
diff --git a/zircon/system/utest/core/pager/pager.cc b/zircon/system/utest/core/pager/pager.cc
index 6d2953d..6dd7770 100644
--- a/zircon/system/utest/core/pager/pager.cc
+++ b/zircon/system/utest/core/pager/pager.cc
@@ -1488,11 +1488,6 @@
ZX_ERR_INVALID_ARGS);
// Please do not use get_root_resource() in new code. See ZX-1467.
- // The get_root_resource() function is a weak reference here. In the
- // standalone pager-test program, it's not defined because the root
- // resource handle is not available to to the test. In the unified
- // Please do not use get_root_resource() in new code. See ZX-1467.
- // standalone core-tests program, get_root_resource() is available.
if (&get_root_resource) {
// unsupported aux vmo type
zx::vmo physical_vmo;
@@ -2141,4 +2136,52 @@
ASSERT_TRUE(t.WaitForFailure());
}
+// Test that if we resize a vmo while it is waiting on a page to fullfill the commit for a pin
+// request that neither the resize nor the pin cause a crash and fail gracefully.
+TEST(Pager, ResizeBlockedPin) {
+ // Please do not use get_root_resource() in new code. See ZX-1467.
+ if (!&get_root_resource) {
+ printf("Root resource not available, skipping\n");
+ return;
+ }
+
+ UserPager pager;
+ ASSERT_TRUE(pager.Init());
+
+ constexpr uint64_t kNumPages = 2;
+ Vmo* vmo;
+ ASSERT_TRUE(pager.CreateVmo(kNumPages, &vmo));
+
+ zx::iommu iommu;
+ zx::bti bti;
+ zx::pmt pmt;
+ zx::unowned_resource root_res(get_root_resource());
+ zx_iommu_desc_dummy_t desc;
+ ASSERT_EQ(zx_iommu_create(get_root_resource(), ZX_IOMMU_TYPE_DUMMY, &desc, sizeof(desc),
+ iommu.reset_and_get_address()),
+ ZX_OK);
+ ASSERT_EQ(zx::bti::create(iommu, 0, 0xdeadbeef, &bti), ZX_OK);
+
+ // Spin up a thread to do the pin, this will block as it has to wait for pages from the user pager
+ TestThread pin_thread([&bti, &pmt, &vmo]() -> bool {
+ zx_paddr_t addr;
+ // Pin the second page so we can resize such that there is absolutely no overlap in the ranges.
+ // The pin itself is expected to ultimately fail as the resize will complete first.
+ return bti.pin(ZX_BTI_PERM_READ, vmo->vmo(), ZX_PAGE_SIZE, ZX_PAGE_SIZE, &addr, 1, &pmt) ==
+ ZX_ERR_OUT_OF_RANGE;
+ });
+
+ // Wait till the userpager gets the request.
+ ASSERT_TRUE(pin_thread.Start());
+ ASSERT_TRUE(pager.WaitForPageRead(vmo, 1, 1, ZX_TIME_INFINITE));
+
+ // Resize the VMO down such that the pin request is completely out of bounds. This should succeed
+ // as nothing has been pinned yet.
+ ASSERT_TRUE(vmo->Resize(0));
+
+ // The pin request should have been implicitly unblocked from the resize, and should have
+ // ultimately failed. pin_thread returns true if it got the correct failure result from pin.
+ ASSERT_TRUE(pin_thread.Wait());
+}
+
} // namespace pager_tests
