Page MenuHomeFreeBSD

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

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib created this revision.Mar 20 2018, 1:17 PM
jtl accepted this revision.Mar 20 2018, 4:16 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.

kib added inline comments.Mar 20 2018, 4:33 PM
sys/amd64/amd64/machdep.c
1557 ↗(On Diff #40495)

To be even more clean:
this chunk will not be committed, while the fpu.c chunk will be.

jtl added a comment.Mar 20 2018, 4:44 PM

Sounds good. Thanks!

This revision was automatically updated to reflect the committed changes.