Page MenuHomeFreeBSD

Introduce pmap_change_prot().
ClosedPublic

Authored by markj on Sep 22 2019, 7:51 PM.

Details

Summary

Generalize pmap_change_attr_locked() so that it can be used to update
mappings permissions as well. This is intended for use in the kernel
linker, both to apply protections to preloaded kernel modules (which are
not covered in kernel_map), and to ensure that the direct map is updated
when some protection changes are applied to the kernel map.

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

markj created this revision.Sep 22 2019, 7:51 PM
markj added reviewers: alc, kib.Sep 22 2019, 7:53 PM
kib added inline comments.Sep 22 2019, 8:23 PM
sys/amd64/amd64/pmap.c
8043 ↗(On Diff #62436)

Why do we need to change protection on the direct map ?

markj added a comment.Sep 22 2019, 8:29 PM

I meant to explain my intent in the description. The function is only really helpful on platforms where kernel mappings do not permit all possible access types by default, i.e., amd64. On other platforms it can simply be a no-op. If there are no objections to the approach of adding a pmap_change_prot() for use in MI code, I will write stubs for the other platforms.

sys/amd64/amd64/pmap.c
8043 ↗(On Diff #62436)

Mainly to ensure that a read-only mapping is not writeable via the direct map. We do this already for the amd64 kernel image itself, in create_pagetables().

kib added inline comments.Sep 23 2019, 2:39 PM
sys/amd64/amd64/pmap.c
7790 ↗(On Diff #62436)

After you changed types to u_long, it makes no sense to have pmap_pde_props(). Might be it can exists as a macro, but I do not see much benefits.

8102 ↗(On Diff #62436)

Don't we need/want to preset PG_M ?

markj added inline comments.Sep 23 2019, 3:28 PM
sys/amd64/amd64/pmap.c
7790 ↗(On Diff #62436)

Hmm, I do not see why it made sense before this diff. The only difference is the use of pd_entry_t vs. pt_entry_t. I will merge them.

8043 ↗(On Diff #62436)

Let me try to justify this approach further: initially I implemented a standalone pmap_change_prot() which only operated on the kernel map, i.e., direct map aliases were ignored. Then I realized that it was very similar to pmap_change_attr(), so I merged them. I am not sure that it is really important to modify the direct map for protection changes in the kernel map (this can be bypassed by clearing WP, after all), but conceptually it is weird to ignore the direct map alias if the intent is to restrict protections on kernel mappings, and it may help catch bugs. I also believe it will not cause excessive demotions: preloaded kernel modules are contiguous in physical memory, and modules loaded by link_elf.c will generally be backed by pages allocated from a reservation, provided that they are loaded during system startup.

8102 ↗(On Diff #62436)

I'm not sure. I think that would be incorrect for the pageable submaps. In other cases we always preset PG_M, I believe, so I can't see a case where setting PG_M here would change anything.

markj updated this revision to Diff 62475.Sep 23 2019, 4:19 PM
markj marked an inline comment as done.

Merge pmap_pte_props() and pmap_pde_props().

kib accepted this revision.Sep 23 2019, 5:13 PM
This revision is now accepted and ready to land.Sep 23 2019, 5:13 PM
This revision was automatically updated to reflect the committed changes.