facilitate processes to be excluded from PTI

[tychon]

Summary

For a specific set of processes (e.g. those that are able to open /dev/kmem) it may not be necessary to perform PTI. By excluding processes from PTI it's possible to avoid the performance impact of PTI.

This review expands the checks for UCR3 == PMAP_NO_CR3 to enable processes to be excluded from PTI. This review however does not define nor implement the policy for which processes are actually excluded from PTI.

[kib]

Please upoad the patch with full context (use svn diff -x -U999999 if diffing with svn).

[tychon]

As requested, I've updated the patch with full context.

[kib]

sys/amd64/amd64/cpu_switch.S

218 ↗

(On Diff #41549)

Why is this check is needed ? I did not repeated the note for all occurrences, but IMO UCR3 check is enough.

223 ↗

(On Diff #41549)

Perhaps calculate the result in %r8 and remove the instructions at lines 224 and 225 ?

sys/amd64/amd64/machdep.c

1798 ↗

(On Diff #41549)

I think this should be commented about. It relies on some assumptions that might be not true in the final version.

sys/amd64/amd64/pmap.c

7443 ↗

(On Diff #41549)

This is quite strange code. I expect it is needed for !PCID case as well. Also note that pmap_activate() may be called when interrupts are not disabled. I suspect that you have to move rsp0 recalculation back to cpu_swtch.s.

[tychon]

I've updated the diff to incorporate the feedback.

sys/amd64/amd64/cpu_switch.S

218 ↗

(On Diff #41549)

After initializing pc_ucr3 to PMAP_NO_CR3 in pmap_pinit0() I was indeed able to delete that check.

223 ↗

(On Diff #41549)

Alas, %r8 contains the PCB so I can't clobber it. I'm not entirely happy with the number of branches in that code, but can't figure out an improved way. At least I got rid of that one branch you mentioned above.

sys/amd64/amd64/machdep.c

1798 ↗

(On Diff #41549)

Since I've reworked the the way the stack pointer in the TSS is handled in cpu_switch() and pmap_activate_sw() this initialization actually isn't needed at all -- it gets updated in all cases before transition from CPL0.

However, it made sense to put a reasonable value in there in case the code changes in the future. If you prefer I can take it out entirely but what's there currently will be incorrect after this patch.

sys/amd64/amd64/pmap.c

7443 ↗

(On Diff #41549)

Good catch! The adjustment of the TSS stack pointer needs to happen in all cases PCID or !PCID. I've moved the code outside of that block.

It needs to remain in here though rather than cpu_swtch() as we can transition from CPL0, in the case of exec(), without going through cpu_swtch(). This may leave rsp0 broken if the child process has PTI enabled but the parent doesn't.

[kib]

sys/amd64/amd64/pmap.c

7460 ↗

(On Diff #41576)

Move local vars declaration into the block at the function start, per style(9).

7443 ↗

(On Diff #41549)

This code can be executing with interrupts enabled. The inconsistent pc_ucr3 vs. tss.rsp0 can be fatal, I believe.

I suspect a solution is to always disable interrupts around all calls to pmap_activate.

[tychon]

I've updated the diff to incorporate the feedback.

[tychon]

sys/amd64/amd64/pmap.c

7443 ↗

(On Diff #41549)

Perhaps I'm mistaken, but I thought that tss.rsp0 only came into play when we're transitioned from the kernel to user land. Namely even if we get an interrupt in the kernel when pc_ucr3 and tss.rsp0 are inconsistent that's okay. They need to be consistent when we exit CPL0. That may happen either via a return from fast_syscall() after leaving this function or if we happen to get interrupted and context switched via cpu_switch() at a later time -- where another attempt at getting pc_ucr3 and tss.rsp0 consistent is made.

[kib]

sys/amd64/amd64/pmap.c

7443 ↗

(On Diff #41549)

Ok.

But still, pmap_activate_sw() is about managing pmap, and not the other state. It is unnatural to have the state management not related to MMU, there. One option is to move rs0 recalculation into doreti, but then it would occur sometimes unnecessary.

Another thing that can be usefully improved, I believe, is to store top of the stack in PCPU pti_stack, instead of the bottom. Then one addition can be saved.

[tychon]

sys/amd64/amd64/pmap.c

7443 ↗

(On Diff #41549)

Indeed storing the top of the PTI stack in PCPU would save an add. I'll do that and update the diff.

Now what to do about the TSS manipulation in pmap_activate_sw() is a little more confounding. It could be done in doreti() but there it will add overhead unnecessarily in most cases.

pmap_activate_sw() does seems a bit unnatural as it is not directly an MMU register nor a pmap data structure. Arguably though it is a reasonable place because if the pmap wasn't changing then the TSS rsp wouldn't need to be adjusted so from a code flow perspective it makes sense.

I'd favor performance over semantics. Let me see if I can find a more suitable location for this snippet. Unless you have further suggestions!

[tychon]

Add top of PTI stack to PCPU to avoid it's calculation in cpu_switch().

[kib]

Please look at trap.c line 465. I think that if (pti) bit of the condition should be replaced.

sys/amd64/amd64/exception.S

305 ↗

(On Diff #41633)

I think it would be more useful to save the user %cr3 into pc_saved_ucr3 always, regardless of the PTI state for the current process. If a mistake was done and user process executed with the wrong page tables, then the condition in trap_pfault() needs to have the actual cr3 value.

352 ↗

(On Diff #41633)

If you move this check right after <name>_pti_doreti and do the direct jump to X<name>, you can save two useless swapgs'es.

505 ↗

(On Diff #41633)

Is this check still needed ?

sys/amd64/amd64/pmap.c

7443 ↗

(On Diff #41549)

Ok, let is stay as is.

[tychon]

I've also fixed up the frame->tf_rip == (long)doreti_iret code in trap().

sys/amd64/amd64/exception.S

352 ↗

(On Diff #41633)

The trap occurs with %gs set to the user value. Don't we need to switch to the kernel value in order to examine pc_ucr3?