Details

Reviewers

jhb
alc
kib
andrew

Commits

rG203e5a1eeec1: linker: Reset DMAP protections in link_elf_unload_file()

Summary

On x86, when a preloaded kernel module is unloaded, we free the backing
(physically contiguous) pages. The ET_REL linker will have adjusted
protections on segments of the preloaded file, which updates the direct
map, so the original protections must be restored when unloading the
module.

Previously this was handled in kmem_bootstrap_free(), but I see no
reason not to handle this within the kernel linker. Moreover, we were
not resetting permissions in the kernel map on arm64.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 69549
Build 66432: arc lint + arc unit

Event Timeline

markj created this revision.Wed, Dec 31, 4:37 PM

Herald added subscribers: olce, imp. · View Herald TranscriptWed, Dec 31, 4:37 PM

markj requested review of this revision.Wed, Dec 31, 4:37 PM

Harbormaster completed remote builds in B69549: Diff 168805.Wed, Dec 31, 4:37 PM

kib added inline comments.Wed, Dec 31, 9:22 PM

sys/kern/link_elf.c
828	Why do you need prot and mask? If you would directly order the function to either calculate the access bits based on PF_ flags or to set it to RW regardless of the PF_ phdr flags, it would be IMO cleaner and even slightly more compact.

Use a bool flag to ignore segment protections instead of prot+mask.

Harbormaster completed remote builds in B69555: Diff 168822.Thu, Jan 1, 3:12 PM

kib accepted this revision.Thu, Jan 1, 3:44 PM

This revision is now accepted and ready to land.Thu, Jan 1, 3:44 PM

alc added inline comments.Thu, Jan 1, 6:07 PM

sys/kern/link_elf.c
1435–1437	I think most people, particularly those who don't have at least a passing knowledge of the pmap and direct map, will be mightily confused by passing VM_PROT_RW here. I think they will expect to see VM_PROT_NONE here. Thoughts?

markj marked an inline comment as done.Thu, Jan 1, 11:41 PM

markj added inline comments.

sys/kern/link_elf.c
1435–1437	We could define a per-arch DMAP_DEFAULT_PROT which gets used here instead. At least amd64, arm64, riscv map the DMAP as RW; on amd64 this changed in commit 33c72b24dee54cd9f5ccab28478fd5a8b5876944. I'm not sure about powerpc, but we can side-step the issue for now since pmap_change_prot() isn't implemented there. I can't see a reason for DMAP_DEFAULT_PROT to be anything other than VM_PROT_RW though, so in the long run we'd like to have a single definition, not an MD one.

kib added inline comments.Thu, Jan 1, 11:51 PM

sys/kern/link_elf.c
1435–1437	Setting the permissions to VM_PROT_NONE does not restore the write enable bit on DMAP (at least on x86). Kernel assumes that any free memory is writeable through DMAP, so does not change it protection on allocation and possible access. Also, I do not think much use of a definition for DMAP_DEFAULT_PROT for the reason that was stated: it is identical for all arches. So IMO a comment would be enough why we change the protection despite the KVA mapping will be destroyed.

markj added a subscriber: jhibbits.Fri, Jan 2, 2:10 AM

markj added inline comments.

sys/kern/link_elf.c
1435–1437	Setting the permissions to VM_PROT_NONE does not restore the write enable bit on DMAP (at least on x86). Kernel assumes that any free memory is writeable through DMAP, so does not change it protection on allocation and possible access. I don't follow. To be clear, I am suggesting having `#define DMAP_DEFAULT_PROT VM_PROT_RW`, and then here we would write `preload_protect_reset(ef, DMAP_DEFAULT_PROT)`. Also, I do not think much use of a definition for DMAP_DEFAULT_PROT for the reason that was stated: it is identical for all arches. So IMO a comment would be enough why we change the protection despite the KVA mapping will be destroyed. @jhibbits mentioned on IRC that it is RWX on powerpc when available, and it cannot trivially be changed to RW. It is not immediately relevant here since preload_protect() is a no-op on everything other than amd64 and arm64. I think I agree that a comment is sufficient though.

kib added inline comments.Fri, Jan 2, 2:14 AM

sys/kern/link_elf.c
1435–1437	The VM_PROT_NONE part of my reply was in response to Alan' suggestion to use VM_PROT_NONE there. At least I do not understand how would it work.