Page MenuHomeFreeBSD

mprotect.2: Update text for largepages
ClosedPublic

Authored by brooks on May 24 2021, 8:47 PM.

Details

Summary

The mprotect syscall acts on page granularity and that page size may
vary (e.g., largepage mappings).

Remove some text that dates to the BSD 4.4 import and is misleading.

Sponsored by: DARPA

Diff Detail

Repository
R11 FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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'.

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

  1. backing object is populated with contiguous pages suitable for superpage mapping
  2. userspace maping is always done with superpages PTEs
  3. we do not allow to clip them not at superpage boundary
gbe added a subscriber: gbe.

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.

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 added inline comments.
lib/libc/sys/mprotect.2
50–51 ↗(On Diff #89777)

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

  1. backing object is populated with contiguous pages suitable for superpage mapping
  2. userspace maping is always done with superpages PTEs
  3. 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
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).

lib/libc/sys/mprotect.2
57 ↗(On Diff #90602)

s/allocation/page/ perhaps

This revision is now accepted and ready to land.Jun 8 2021, 9:54 PM
lib/libc/sys/mprotect.2
51–57 ↗(On Diff #90607)

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.

lib/libc/sys/mprotect.2
51–57 ↗(On Diff #90607)

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?

For the purposes of this discussion, I think you should just go with your original proposal as it is clearly a useful step forward.

Arg, I somehow included this review number in a ports commit message. I'll open a new one.