It was completely wrong before adding largepage support, but now it is closer to being correct than not. I propose to remove the second sentence from your diff, and change 'an entire region' to 'an entire large page'.

kib added reviewers: markj, alc.May 24 2021, 9:10 PM

In D30442#683682, @kib wrote:

It was completely wrong before adding largepage support, but now it is closer to being correct than not. I propose to remove the second sentence from your diff, and change 'an entire region' to 'an entire large page'.

Maybe something like: "Page sizes many vary within a virtual address space; the granularity of page protection changes may be as large as an entire large page."

That being said, I dislike these semantics. It seems to introduce the potential for protection errors and IMO we should require that addr..addr+len should cover the whole page for large pages so there is no potential confusion of intent.

Won't we demote large pages if the protections of sub-pages differ? I don't think we (yet) have a flag to make mappings use a minimum page size (e.g. to force the use of 2MB pages and not permit demoting to 4K pages) for which the effect would then be as Brooks' new sentence describes.

In D30442#683737, @jhb wrote:

Won't we demote large pages if the protections of sub-pages differ? I don't think we (yet) have a flag to make mappings use a minimum page size (e.g. to force the use of 2MB pages and not permit demoting to 4K pages) for which the effect would then be as Brooks' new sentence describes.

superpages != largepages. Largepages are relatively new thing, for them we guarantee that

backing object is populated with contiguous pages suitable for superpage mapping
userspace maping is always done with superpages PTEs
we do not allow to clip them not at superpage boundary

LGTM

If mprotect(2) can force the breakup of pages suitable for superpage mappings, then it opens a vector for a denial of performance by mapping and breaking up all the superpages held by the operating system.

My take would be to fail mprotect(2) requests that would force such breakups. The application could then retry for the larger page.

In D30442#686022, @mckusick wrote:

If mprotect(2) can force the breakup of pages suitable for superpage mappings, then it opens a vector for a denial of performance by mapping and breaking up all the superpages held by the operating system.

My take would be to fail mprotect(2) requests that would force such breakups. The application could then retry for the larger page.

mprotect(2) changes protection of the specific mapping. The underlying physical pages cannot be 'broken' by mprotect, in the sense that they continue to be the properly aligned physically contiguous run of suitable size. Pmap module routinely breaks (demotes) superpage mapings for different reason, and trying to disable traditionally enabled user actions is wrong. Esp. because superpages are transparent and user does not have neither control nor visibility when the mapping is promoted.

alc accepted this revision.May 30 2021, 6:40 AM

alc added inline comments.

lib/libc/sys/mprotect.2
49–61	While we're here, I find this nearby sentence to be incomprehensible. Could you please try to fix it too?

This revision is now accepted and ready to land.May 30 2021, 6:40 AM

In D30442#683738, @kib wrote:

In D30442#683737, @jhb wrote:

Won't we demote large pages if the protections of sub-pages differ? I don't think we (yet) have a flag to make mappings use a minimum page size (e.g. to force the use of 2MB pages and not permit demoting to 4K pages) for which the effect would then be as Brooks' new sentence describes.

superpages != largepages. Largepages are relatively new thing, for them we guarantee that

backing object is populated with contiguous pages suitable for superpage mapping

userspace maping is always done with superpages PTEs

we do not allow to clip them not at superpage boundary

Kostik, the implications of (3) may not be obvious to everyone, specifically, that an mprotect() that does not cover an entire explicit largepage will fail.

mprotect.2: Update text for largepages
mprotect.2: Improve the description of prot

This revision now requires review to proceed.Jun 8 2021, 7:38 PM

Harbormaster completed remote builds in B39809: Diff 90602.Jun 8 2021, 7:38 PM

brooks retitled this revision from mprotect.2: Remove legacy BSD text to mprotect.2: Update text for largepages.Jun 8 2021, 7:39 PM

brooks edited the summary of this revision. (Show Details)

I've to update the description to match what I believe is currently implemented. I've not mentioned largepages by name because I can find no documentation of them to reference. I've also attempted to improve the description of prot (that will be a separate commit).

kib added inline comments.Jun 8 2021, 8:13 PM

lib/libc/sys/mprotect.2
57	s/allocation/page/ perhaps

s/allocation/page/

Harbormaster completed remote builds in B39814: Diff 90607.Jun 8 2021, 9:27 PM

kib accepted this revision.Jun 8 2021, 9:54 PM

This revision is now accepted and ready to land.Jun 8 2021, 9:54 PM

alc added inline comments.Jun 8 2021, 11:07 PM

lib/libc/sys/mprotect.2
51–57	I'm afraid that this is going to cause confusion. While this is true for explicit large pages, it is not true for transparent superpages. As I believe John already mentioned, for transparent superpages, the kernel will automatically demote the page size and then perform the requested protection changes.

brooks added inline comments.Jun 9 2021, 8:52 PM

lib/libc/sys/mprotect.2
51–57	I guess the question is, should we go back to my original proposal of deleting the original (outright wrong) text and leaving documentation of large pages for later or try to explain the difference between superpages and large pages here?