Per-page capability-dirty tracking, take N+1 #405

nwf · 2020-03-23T18:38:10Z

A better stab at capability dirty tracking atop master's VM bits.

Among other things, this switches from the revocation-based names for the MI bits and uses a more natural (or, well, VM-themed) scheme. While here, it obviates the PMAP_ENTER_NOSTORETAGS flag in favor of per-page MI bits (VPO_FASTCAPDIRTY).

This seems to boot both purecap and hybrid and run cheriabitest -u -a but I'd really enjoy some more eyes on it.

jrtc27 · 2020-03-23T19:36:15Z

Various stylistic comments. There's also inconsistency about !pte_test vs pte_test == 0; upstream seems to like the former (and is what you copied for your pmap_emulate_capdirty), but everything else you've added is using the latter, for better or worse; we should probably be consistent and do the latter.

sys/mips/include/pte.h

sys/vm/pmap.h

sys/mips/mips/pmap.c

sys/mips/mips/trap.c

jrtc27 · 2020-03-23T19:30:06Z

sys/mips/mips/trap.c

+
+		if (try_emulate_capdirty(trapframe, NULL,
+					 &p->p_vmspace->vm_pmap) == 0) {
+			if (!usermode) {


We know we're in a T_USER case, and there's no fallthrough (nor any gotos) to get us here otherwise, so this should always be true. You're also not passing kernel_pmap anyway for !usermode, and maybe this means you can get rid of the if (kpmap) check? Though maybe you can still trigger CP2 TLB faults for kernel VAs from usermode...

Good catch.

I'm not sure whether we can get a C2E from user PCs on kernel VAs (I'd hope the segment checks fire first), but it seems like a reasonable defense in depth to only pass the kpmap in when coming from a kernel PC (in the prior call to try_emulate_capdirty), as there's certainly no reason to use it in the user PC case.

sys/vm/vm_mmap.c

sys/vm/vm_page.h

nwf · 2020-03-24T16:57:06Z

There appears to be some bug in my understanding, still, as when I try to make this slightly lazier (setting VPO_FASTCAPDIRTY only after a C2E fault), I get some very strange errors that look like they're triggered by something in this series. Still debugging, expect revised submission later this week. (Gotta update the Cornucopia paper first.)

brettferdosi · 2020-03-27T05:59:21Z

sys/vm/vm_page.h

+ * that it may begin tracking capability presence.  (This will be more relevant
+ * once the notion of Capability Load Generations land; at the moment,
+ * VPO_FASTCAPDIRTY is expected to reflect the VM object-level permission;
+ * see OBJ_NOSTORETAGS.)


This comment makes it seem like FASTCAPDIRTY corresponds to MCC (the page is permitted to contain capabilities and in fact is known to maybe contain capabilities), but the code seems to be using FASTCAPDIRTY like ASC (the page is permitted to store capabilities) - e.g. in init_pte_prot.

brettferdosi · 2020-03-27T06:01:42Z

sys/mips/mips/pmap.c

+	 * the target page.
+	 */
+	if ((src->a.flags & PGA_CAPDIRTY) || (src->oflags & VPO_FASTCAPDIRTY))
+		vm_page_capdirty(dst);


Do we also want to copy FASTCAPDIRTY if it is set in src?

brettferdosi · 2020-03-27T06:14:56Z

sys/vm/vm_page.h

+ * cleared.  Setting this bit happens without the page lock held, but clearing
+ * it requires synchronization.  Specifically, this bit may be cleared only if
+ * the page is xbusy and the clearer subsequently measures all tag bits on the
+ * page.


Part of this comment makes it seem like CAPDIRTY corresponds to an actual dirty bit (tracks cap stores since the last time it was cleared), but the part about requiring measurement of all tag bits on the page makes it seem like it corresponds to MCC (the page is permitted to contain capabilities and in fact is known to maybe contain capabilities). The code seems to be using CAPDIRTY like an actual dirty bit - e.g. init_pte_prot and pmap_is_capdirty. I think it would be useful to introduce another flag that corresponds to MCC.

sys/mips/mips/pmap.c

brettferdosi · 2020-03-27T16:27:03Z

sys/vm/vm_mmap.c

+				    ((m->a.flags & PGA_CAPDIRTY) != 0 ||
+				    (m->flags & VPO_FASTCAPDIRTY) != 0 ||
+				    pmap_is_capdirty(m) ||
+				    (m->a.flags & PGA_CAPDIRTY) != 0))


Just want to confirm that pmap_is_capdirty() changes the value of m->a.flags because the condition is checked twice.

pmap_is_capdirty takes the pmap lock and so is an opportunity for contention, even if it doesn't explicitly change the flag. This was much more important when mincore was going to be how we did efficient page skipping in userland, so I've just dropped this particular bit of complexity for this PR.

sys/mips/mips/pmap.c

brettferdosi · 2020-03-27T16:31:40Z

sys/vm/pmap.h

@@ -108,7 +108,6 @@ extern vm_offset_t kernel_vm_end;
 #define	PMAP_ENTER_WIRED	0x00000200
 #if __has_feature(capabilities)
 #define	PMAP_ENTER_NOLOADTAGS	0x00000400
-#define	PMAP_ENTER_NOSTORETAGS	0x00000800


Is this flag still potentially useful for setting PTE_CRO and the vm page flag for tracking that permission?

PTE_CRO is going to be taken from VPO_CAPSTORE (what was FASTCAPDIRTY) or the object's NOSTORETAGS flag; the pmap can look up, since it has the vm_page_t rather than the vm needing to pull out duplicate information to pass down.

nwf · 2020-06-30T20:29:29Z

Converting to draft at least until upstream takes and we merge https://reviews.freebsd.org/D25523.

bsdjhb

Clearly I need to start work on the VM_PROT_CAP_* changes.

sys/riscv/riscv/pmap.c

sys/riscv/include/pte.h

sys/riscv/riscv/pmap.c

sys/mips/include/pte.h

sys/riscv/riscv/pmap.c

sys/vm/swap_pager.c

sys/riscv/riscv/pmap.c

+		KASSERT(!(msrca.flags & PGA_CAPSTORE)
+		    || (mdsta.flags & PGA_CAPSTORE),
+		    ("pmap_copy_page_internal CAPSTORE src & !dst"));
+		KASSERT(!(msrca.flags & PGA_CAPDIRTY)


sys/riscv/riscv/pmap.c

@@ -4204,7 +4275,7 @@ pmap_clear_modify(vm_page_t m)
 			va += VM_PAGE_TO_PHYS(m) - PTE_TO_PHYS(oldl2);
 			l3 = pmap_l2_to_l3(l2, va);
 			pmap_clear_bits(l3, PTE_D | PTE_W);
-			vm_page_dirty(m);
+			pmap_page_dirty(oldl2, m);


sys/vm/vm_page.h

sys/vm/swap_pager.c

sys/riscv/riscv/pmap.c

bsdjhb

I've tried to close all the open comments I think are resolved. I think a few of the ones still open are probably stale.

sys/riscv/riscv/trap.c

sys/mips/mips/pmap.c

sys/riscv/riscv/pmap.c

bin/cheribsdtest/cheribsdtest_vm.c

bsdjhb · 2021-02-16T16:19:30Z

sys/vm/vm_page.h

@@ -433,6 +433,21 @@ extern struct mtx_padalign pa_lock[];
 * PGA_SWAP_FREE is used to defer freeing swap space to the pageout daemon
 * when the context that dirties the page does not have the object write lock
 * held.
+ *
+ * PGA_CAPSTORE right now reflects the object's capability store permission.


BTW, this comment might have been the source of my earlier confusion about PGA_CAPSTORE and the swap case with cap_store_on_alloc, etc. I think the comment should talk more about the intended purpose and probably not claim that is identical to OBJ_HASCAP.

jrtc27 · 2021-02-19T18:07:19Z

PGA_CAPSTORE is a bit weird, as it feels wrong to have VM_PROT_WRITE_CAP asserted yet not want to set PTE_CW. Would it be better to have the VM strip out VM_PROT_WRITE_CAP for !PGA_CAPSTORE and hide it as an implementation detail? Similarly can we reuse flags to remove some (all?) of the PGA_DIRTY uses in pmap other than calling into VM functions to dirty? There are very few queries of PGA_* in the pmap code and I don't like adding more.

If ftype is VM_PROT_WRITE | VM_PROT_WRITE_CAP but the PTE has PTE_W clear, we'll accidentally skip over the "goto done" and end up returning 1 as if we did something. For the same ftype, even if PTE_W is set, we won't assert PTE_D. Fix by treating VM_PROT_WRITE as the flag it is rather than a whole value.

…PSTORE

* If we're allocating a page to store capabilities to it, assume we're about to do that in the near future and let hardware mark it CAPDIRTY, rather than requiring another trap to mark it CAPSTORE first. * Do the same thing if we're populating a page to store capabilities to it. * If we're on the soft_fast path, we might be upgrading a page from not storing capabilities to storing capabilities, so mark it CAPSTORE here, too.

The swap pager might directly set tags on physical pages, bypassing the MMU. Go ahead and mark the vm_page as CAPDIRTY (and CAPSTORE) in such cases.

While here, move EXCP_STORE_AMO_CAP_PAGE_FAULT traps to the ordinary data_abort() path. This builds directly on VM_PROT_WRITE_CAP, unlike in the MIPS analogue, for which the equivalent of EXCP_STORE_AMO_CAP_PAGE_FAULT is a C2E trap, not a TLB trap.

Now that we're tracking capdirty, it might be informative to show userland. Once upon a time, we thought this might be part of the revoker implementation, but now we think it's just diagnostics.

When first mapping a page with abstract capability store permission, should we map it CAPSTORE? If we think that it's likely to experience actual capability stores soon, yes; if not, then these cap-devoid pages will create extra work for the (eventual) revoker. Add sysctl so we can experiment; default to minimizing traps (vm.capstore_on_alloc=1).

Co-authored-by: Brett Gutstein <[email protected]>

nwf · 2021-04-23T13:01:31Z

Obviated by #937, or take "N + 2".

Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Kay Pedersen <[email protected]> Signed-off-by: Rob Norris <[email protected]> Sponsored-By: OpenDrives Inc. Sponsored-By: Klara Inc. Closes #15050 Closes CTSRD-CHERI#405 Closes #13349

nwf requested review from jrtc27, qwattash, brooksdavis, brettferdosi, bsdjhb and staceyson March 23, 2020 18:38

jrtc27 reviewed Mar 23, 2020

View reviewed changes

nwf force-pushed the capdirty branch 2 times, most recently from 3912449 to 8ecc0aa Compare March 23, 2020 22:56

nwf requested a review from jrtc27 March 23, 2020 23:19

brettferdosi reviewed Mar 27, 2020

View reviewed changes

sys/mips/mips/pmap.c Outdated Show resolved Hide resolved

brettferdosi reviewed Mar 27, 2020

View reviewed changes

sys/mips/mips/pmap.c Outdated Show resolved Hide resolved

brettferdosi reviewed Mar 27, 2020

View reviewed changes

sys/mips/mips/pmap.c Outdated Show resolved Hide resolved

brettferdosi reviewed Mar 27, 2020

View reviewed changes

nwf force-pushed the capdirty branch from 8ecc0aa to c33db8c Compare June 25, 2020 02:19

nwf requested a review from brettferdosi June 25, 2020 02:19

nwf changed the base branch from master to dev June 25, 2020 02:21

nwf force-pushed the capdirty branch from c33db8c to 2aee0f9 Compare June 30, 2020 02:30

nwf mentioned this pull request Jun 30, 2020

Trivial tidying from capdirty #578

Merged

nwf force-pushed the capdirty branch from 2aee0f9 to 2745940 Compare June 30, 2020 20:26

nwf marked this pull request as draft June 30, 2020 20:28

bsdjhb reviewed Aug 7, 2020

View reviewed changes

sys/riscv/riscv/pmap.c Outdated Show resolved Hide resolved

sys/riscv/include/pte.h Outdated Show resolved Hide resolved

sys/riscv/riscv/pmap.c Outdated Show resolved Hide resolved

nwf force-pushed the capdirty branch from 2745940 to dfd22f1 Compare August 12, 2020 22:22

bsdjhb reviewed Feb 2, 2021

View reviewed changes

nwf force-pushed the capdirty branch from af050bb to 258d996 Compare February 9, 2021 03:00

bsdjhb reviewed Feb 10, 2021

View reviewed changes

sys/vm/swap_pager.c Show resolved Hide resolved

sys/riscv/riscv/pmap.c Outdated Show resolved Hide resolved

bsdjhb reviewed Feb 10, 2021

View reviewed changes

sys/riscv/riscv/trap.c Show resolved Hide resolved

sys/mips/mips/pmap.c Show resolved Hide resolved

arichardson reviewed Feb 11, 2021

View reviewed changes

sys/riscv/riscv/pmap.c Show resolved Hide resolved

brettferdosi reviewed Feb 12, 2021

View reviewed changes

bin/cheribsdtest/cheribsdtest_vm.c Outdated Show resolved Hide resolved

nwf force-pushed the capdirty branch from 258d996 to 450711b Compare February 12, 2021 18:23

bsdjhb reviewed Feb 16, 2021

View reviewed changes

nwf force-pushed the capdirty branch from 450711b to 903b3ca Compare February 17, 2021 18:09

nwf mentioned this pull request Feb 20, 2021

Capdirty, more vmish #937

Closed

nwf-msr and others added 14 commits March 25, 2021 22:27

Teach vm_fault_trap about VM_PROT_WRITE_CAP

2cb42bb

capdirty: introduce MI bits

8ef3a03

capdirty: propagate CAPSTORE|CAPDIRTY when pmap_copy_page_tags()-ing

0626201

capdirty: assert page busy or object locked when clearing CAPDIRTY/CA…

2339978

…PSTORE

capdirty: set PGA_CAPSTORE and PGA_CAPDIRTY for kmem

22e9ae0

capdirty: mark CAPDIRTY in page in from the swap_pager

7b0e359

The swap pager might directly set tags on physical pages, bypassing the MMU. Go ahead and mark the vm_page as CAPDIRTY (and CAPSTORE) in such cases.

capdirty: add vm_page_assert_pga_capmeta_copy

50c00de

capdirty: mips support for capdirty tracking

f341c85

capdirty: capability presence information in mincore

a0a221c

Now that we're tracking capdirty, it might be informative to show userland. Once upon a time, we thought this might be part of the revoker implementation, but now we think it's just diagnostics.

capdirty: first, minimal test in cheritest

38468fe

Co-authored-by: Brett Gutstein <[email protected]>

nwf force-pushed the capdirty branch from 903b3ca to 38468fe Compare March 25, 2021 22:34

nwf closed this Apr 23, 2021

nwf deleted the capdirty branch June 17, 2021 10:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Per-page capability-dirty tracking, take N+1 #405

Per-page capability-dirty tracking, take N+1 #405

nwf commented Mar 23, 2020

jrtc27 commented Mar 23, 2020

jrtc27 Mar 23, 2020

nwf Mar 23, 2020

nwf commented Mar 24, 2020

brettferdosi Mar 27, 2020 •

edited

Loading

brettferdosi Mar 27, 2020

brettferdosi Mar 27, 2020

brettferdosi Mar 27, 2020

nwf Jun 25, 2020

brettferdosi Mar 27, 2020

nwf Jun 25, 2020

nwf commented Jun 30, 2020

bsdjhb left a comment

This comment was marked as resolved.

This comment was marked as resolved.

bsdjhb left a comment

bsdjhb Feb 16, 2021

jrtc27 commented Feb 19, 2021

nwf commented Apr 23, 2021

Per-page capability-dirty tracking, take N+1 #405

Per-page capability-dirty tracking, take N+1 #405

Conversation

nwf commented Mar 23, 2020

jrtc27 commented Mar 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nwf commented Mar 24, 2020

brettferdosi Mar 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nwf commented Jun 30, 2020

bsdjhb left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

bsdjhb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtc27 commented Feb 19, 2021

nwf commented Apr 23, 2021

brettferdosi Mar 27, 2020 •

edited

Loading