Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

several caprevoke optimizations #2195

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

several caprevoke optimizations #2195

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c5427db
caprevoke: Make the skip_fault optimization smarter
markjdb Jul 24, 2024
0577a32
caprevoke: Skip VM objects that have OBJ_HASCAP clear
markjdb Aug 5, 2024
f0dce4c
caprevoke: Bail if the target process exited during an async scan
markjdb Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 38 additions & 12 deletions sys/vm/vm_cheri_revoke.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@

static void vm_cheri_revoke_pass_pre(vm_map_t);
static void vm_cheri_revoke_pass_post(vm_map_t);
static int vm_cheri_revoke_pass_locked(const struct vm_cheri_revoke_cookie *);
static int vm_cheri_revoke_pass_locked(struct vmspace *,
const struct vm_cheri_revoke_cookie *);

static SYSCTL_NODE(_vm_stats, OID_AUTO, cheri_revoke,
CTLFLAG_RD | CTLFLAG_MPSAFE, 0,
Expand All @@ -84,15 +85,20 @@
"Number of VM map re-lookups");

static COUNTER_U64_DEFINE_EARLY(cheri_skip_prot_no_readcap);
SYSCTL_COUNTER_U64(_vm_stats_cheri_revoke, OID_AUTO, skip_prot_no_readcap, CTLFLAG_RD,

Check warning on line 88 in sys/vm/vm_cheri_revoke.c

View workflow job for this annotation

GitHub Actions / Style Checker 

line over 80 characters
&cheri_skip_prot_no_readcap,
"Virtual pages skipped in map entries without readcap permission");

static COUNTER_U64_DEFINE_EARLY(cheri_skip_obj_no_hascap);
SYSCTL_COUNTER_U64(_vm_stats_cheri_revoke, OID_AUTO, skip_obj_no_hascap, CTLFLAG_RD,

Check warning on line 93 in sys/vm/vm_cheri_revoke.c

View workflow job for this annotation

GitHub Actions / Style Checker 

line over 80 characters
&cheri_skip_obj_no_hascap,
"Virtual pages skipped in VM objects with no capabilities");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason to prefer this over the caprevoke stats infrastructure? (Though admittedly I'd not be sad to see the latter get ripped out.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly that I wanted to have a global view of the counter value, and sysctl is more convenient for that. The existing stats infrastructure collects per-process stats and I believe is limited to printing them when the process exits. The right direction might be a hybrid scheme wherein we maintain per-process (really, per-vmspace) and global counters, and use the former to update the latter after each scan.

static COUNTER_U64_DEFINE_EARLY(cheri_last_ref_early_finish);
SYSCTL_COUNTER_U64(_vm_stats_cheri_revoke, OID_AUTO, last_ref_early_finish, CTLFLAG_RD,

Check warning on line 98 in sys/vm/vm_cheri_revoke.c

View workflow job for this annotation

GitHub Actions / Style Checker 

line over 80 characters
&cheri_last_ref_early_finish,
"Scans finished early because the target exited");

/***************************** KERNEL THREADS ***************************/

static MALLOC_DEFINE(M_REVOKE, "cheri_revoke", "cheri_revoke temporary data");
Expand Down Expand Up @@ -182,6 +188,7 @@
mtx_unlock(&async_revoke_mtx);

vmspace_switch_aio(arc->vm);
vmspace_free(arc->vm);

/*
* Advance the async state machine.
Expand All @@ -192,15 +199,14 @@
/*
* Do the actual revocation pass.
*/
error = vm_cheri_revoke_pass_locked(&arc->cookie);
error = vm_cheri_revoke_pass_locked(arc->vm, &arc->cookie);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks a bit weird to use this after it was seemingly free'd above. It might be clearer to the reader with a comment and a local vm temporary like so:

     vmspace_switch_aio(&arc->vm);
     vm = arc->vm;

     /*
      * Drop arc's reference on the vmspace.  The scanner kproc holds a reference via
      * its p_vmspace pointer while the scanning the address space.  Dropping the
      * reference here permits the scan to return early if target process exits early
      * leaving the scanner kproc's reference as the only reference.
      */
     vmspace_free(arc->vm);


/*
* A revocation pass is done. Advance the state machine again
* so that the application can see the result.
*/
vm_cheri_revoke_pass_async_post(map, error);

vmspace_free(arc->vm);
free(arc, M_REVOKE);
}

Expand Down Expand Up @@ -907,10 +913,14 @@
* The map must be read-locked on entry and will be read-locked on exit, but
* the lock may be dropped internally. The map must, therefore, also be
* held across invocation.
*
* The containing vmspace may be referenced by the "vm" parameter to signal that
* the revocation sweep is asynchronous and should terminate if the caller ends
* up holding the final vmspace reference.
*/
static int
vm_cheri_revoke_map_entry(const struct vm_cheri_revoke_cookie *crc,
vm_map_entry_t entry, vm_offset_t *addr)
struct vmspace *vm, vm_map_entry_t entry, vm_offset_t *addr)
{
vm_offset_t ooffset;
vm_object_t obj;
Expand Down Expand Up @@ -961,6 +971,13 @@
vm_offset_t oaddr = *addr;
#endif

/* Has the target process already exited? */
if (vm != NULL && refcount_load(&vm->vm_refcnt) == 1) {
VM_OBJECT_WUNLOCK(obj);
counter_u64_add(cheri_last_ref_early_finish, 1);
return (KERN_NOT_RECEIVER);
}

/* Find ourselves in this object */
ooffset = *addr - entry->start + entry->offset;

Expand Down Expand Up @@ -1033,7 +1050,8 @@
* returning.
*/
static int
vm_cheri_revoke_pass_locked(const struct vm_cheri_revoke_cookie *crc)
vm_cheri_revoke_pass_locked(struct vmspace *vm,
const struct vm_cheri_revoke_cookie *crc)
{
int res = KERN_SUCCESS;
const vm_map_t map = crc->map;
Expand All @@ -1056,13 +1074,21 @@
* XXX Somewhere around here we should be resetting
* MPROT_QUARANTINE'd map entries to be usable again, yes?
*/
res = vm_cheri_revoke_map_entry(crc, entry, &addr);
res = vm_cheri_revoke_map_entry(crc, vm, entry, &addr);

/*
* We might be bailing out because a page fault failed for
* catastrophic reasons (or polite ones like ptrace()).
*/
if (res != KERN_SUCCESS) {
switch (res) {
case KERN_SUCCESS:
break;
case KERN_NOT_RECEIVER:
/* The vmspace is orphaned, there is nothing to do. */
res = KERN_SUCCESS;
goto out;
default:
/*
* We might be bailing out because a page fault failed
* for catastrophic reasons (or polite ones like
* ptrace()).
*/
printf("CHERI revoke bail va=%lx res=%d\n", addr, res);
goto out;
}
Expand Down Expand Up @@ -1106,10 +1132,10 @@
int res;

vm_cheri_revoke_pass_pre(crc->map);
res = vm_cheri_revoke_pass_locked(crc);
res = vm_cheri_revoke_pass_locked(NULL, crc);
vm_cheri_revoke_pass_post(crc->map);

return (res);

Check warning on line 1138 in sys/vm/vm_cheri_revoke.c

View workflow job for this annotation

GitHub Actions / Style Checker 

Missing Signed-off-by: line
}

void
Expand Down
Loading