Differential D26211

amd64 pmap_pkru_same: prev_ppr was always NULL
Needs ReviewPublic
Actions

Authored by vangyzen on Aug 27 2020, 3:04 PM.

Details

Reviewers

kib

Summary

Fix the logic so it works as it appears.

Reported by: Coverity

Test Plan

I have no idea how to test this. Any advice would be appreciated.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 33626
Build 30869: arc lint + arc unit

Event Timeline

vangyzen created this revision.Aug 27 2020, 3:04 PM

Herald added subscribers: dab, badger. · View Herald TranscriptAug 27 2020, 3:04 PM

vangyzen requested review of this revision.Aug 27 2020, 3:04 PM

Harbormaster completed remote builds in B33193: Diff 76282.Aug 27 2020, 3:04 PM

This code fragment checks that PKRU index is consistent for the whole range. It is used only by superpage promotion code to ensure that promoting 2M range to superpage is possible by all PKRU keys of the constituent 4K pages being same.

To test you would need a machine with PKRU support, i.e. Skylake server. Then set up anon 2M mapping aligned at 2M. Test for promotion is to touch all pages in the range, checking for promotion after that is done with mincore(2) MINCORE_SUPER.

If you set the whole range with some key using x86_pkru_protect_range(), then mincore should repor MINCORE_SUPER. If at least one page was marked with different key, then after touching the range, MINCORE_SUPER must be not set.

This revision is now accepted and ready to land.Aug 27 2020, 4:26 PM

add pkru_test for the fix in pmap_pkru_same()

This revision now requires review to proceed.Sep 18 2020, 4:36 PM

Harbormaster completed remote builds in B33626: Diff 77186.Sep 18 2020, 4:36 PM

@kib Thank you for the detailed description. You practically wrote the test for me.

Two of the test cases do not pass, so I'm not sure the other two are valid. Anyway, I don't have any more time to spend on this, and it seems valuable in its current form.

I do not see why did not you committed the pmap.c change already. Please do, regardless of tests.

tests/sys/vm/pkru_test.c
67	Change reads to writes and see if your failing tests start working.

In D26211#589162, @kib wrote:

I do not see why did not you committed the pmap.c change already. Please do, regardless of tests.

I like to commit a test with its corresponding bug fix. The test might take some time, though, so I'll go ahead.

tests/sys/vm/pkru_test.c
67	I did that originally, but I'm using PKRU to prevent writes, so the test crashes. By my understanding of the spec, write-only isn't possible with PKRU. If I change to writes, the default test still fails in the same way. I'm testing on OneFS, which is behind head. I'll see if I can test on head.

vangyzen marked an inline comment as done.Sep 18 2020, 8:51 PM

vangyzen mentioned this in rS365890: amd64 pmap_pkru_same: prev_ppr was always NULL.Sep 18 2020, 8:53 PM

kib added inline comments.Sep 18 2020, 9:03 PM

tests/sys/vm/pkru_test.c
67	For the test, you do not need different permissions, only different keys. See https://gist.github.com/45ff1d526aef78e8697daeca9560f0f1 for an example of promotion working.