Make KPI for handling of rw/ro kernel text.
ClosedPublic
Actions

Authored by kib on Mar 20 2018, 1:17 PM.

Details

Reviewers

Commits

rS331253: Disable write protection around patching of XSAVE instruction in the
rS331252: Provide KPI for handling of rw/ro kernel text.

Summary

This is pure syntax patch to create an interface to enable and later restore write access to the kernel text. It is in line with e.g. vm_fault_disable_pagefaults() by allowing the nesting.

The chunk in amd64/machdep.c:hammer_time() is kept for now to illustrate why I decided to create the KPI, but the issue is currently being handled in different manner. Still I believe that organization of CR0.WP is useful.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kib created this revision.Mar 20 2018, 1:17 PM

Herald added subscribers: emaste, imp. · View Herald TranscriptMar 20 2018, 1:17 PM

Harbormaster completed remote builds in B15660: Diff 40495.Mar 20 2018, 1:17 PM

FWIW, I am aware of someone else doing fixups who may need this KPI. I agree it is a good idea.

sys/amd64/amd64/machdep.c
1557 ↗	(On Diff #40495)	Now that you have added disable_wp/enable_wp calls in fpuinit_bsp1(), is it your plan to commit this change? (You said you left it to illustrate why you created the KPI. I was unclear if you left it only for this review or actually intend to commit it.)
sys/amd64/include/md_var.h
57 ↗	(On Diff #40495)	Is it worth making this an inline function so you can do the bool check without a function call? On the other hand, this probably should never be called in a critical path, so maybe simplicity is better.

This revision is now accepted and ready to land.Mar 20 2018, 4:16 PM

kib added inline comments.Mar 20 2018, 4:27 PM

sys/amd64/amd64/machdep.c
1557 ↗	(On Diff #40495)	Sorry, I was not clear, I did not mentioned the other issue, which is the sysinit sorting in init_main(). We have the linker sets elements declared a consts and then sysinits pointers array is put into rodata. This works as far as CR0.WP is disabled. The chunk from machdep.c:hammer_time() fixes that, but it is better to fix by putting sysinit into data, and I do not indend to commit the line you comment is attached to.
sys/amd64/include/md_var.h
57 ↗	(On Diff #40495)	Yes, my thought were exactly the same. This should not be done on the critical path, and you need a lot of precautions when you use it. For instance, the context switching must be disabled. So it does not make sense to expose the implementation in the header.