Skip to content

feat(libkernel): accept MADV_PROTECT advisory hint in madvise - #4582

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
jagrat04:feat/libkernel-madvise-madv-protect-pr
Oct 13, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
jagrat04:feat/libkernel-madvise-madv-protect-pr

Conversation

@jagrat04

Copy link
Copy Markdown
Contributor

What

Accepts MADV_PROTECT (10) as a valid advisory hint in madvise in libkernel. In FreeBSD, MADV_PROTECT protects the process from OOM termination. Like the other madvise hints (0-9), user-mode emulation accepts it as a no-op hint without terminating the process via NotImplemented_nid_no_patch. Updates unit tests in GuestMemory.cpp and documents Technical Debt.

Tested

ools/check_conventions.py --base origin/main

Guest unit tests in core/libs/tests/GuestMemory.cpp

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Unblocks: none
  • AI-assisted: yes

@github-actions

github-actions Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Progress report (43e3a70 → 59bd0c1)

📈 System libraries: 87.89% (+0.02%, +1 implemented functions)

✅ 1 implemented
Library Name
libkernel madvise_nid_postfix

@github-actions github-actions Bot added libkernel docs needs rebase Conflicts with main, rebase needed labels Oct 12, 2026
@jagrat04
jagrat04 force-pushed the feat/libkernel-madvise-madv-protect-pr branch from de693db to 5e6c257 Compare October 12, 2026 18:57
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 12, 2026

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Before this can go in:

  • madvise with MADV_PROTECT is not an address-range hint in FreeBSD: kern_madvise (sys/vm/vm_mmap.c) handles it before the address check, ignores addr/len, and calls kern_procctl(..., PROC_SPROTECT, PPROT_SET) on the calling process. Here PosixMemory.cpp:229 still validates the range for advice 10, so madvise(0, 0xFFFFFFFFFFFFF000, 10) returns EINVAL where FreeBSD does not look at it. Please make advice 10 skip the range check, or keep throwing.
  • PROC_SPROTECT needs privilege (PRIV_VM_MADV_PROTECT) and fails otherwise, so returning 0 unconditionally is a guess, not the exact result. If you keep it as a stub, say which result it follows and why in the TechnicalDebt line (docs/dev/debt/libkernel.md:146), and cite vm_mmap.c.
  • core/libs/tests/GuestMemory.cpp:1969 only adds advice 10 with a valid range. Add a row with an invalid range for advice 10, so the test pins the behaviour chosen above.
  • The description names no title or issue that calls madvise(MADV_PROTECT). Without one, the previous explicit throw is the exact-or-throw choice; please add the title/PPSA (or log) that hits it.

Feel free to push the changes and ping me.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 12, 2026
@jagrat04
jagrat04 force-pushed the feat/libkernel-madvise-madv-protect-pr branch from 5e6c257 to 59bd0c1 Compare October 12, 2026 22:52
@jagrat04

Copy link
Copy Markdown
Contributor Author

Updated as requested:

  • Rebased cleanly on origin/main.
  • madvise skips the address/length validation for GuestAdviceProtect (10), matching FreeBSD 11 kern_madvise (sys/vm/vm_mmap.c) handling before address checks.
  • Documented in docs/dev/debt/libkernel.md citing sys/vm/vm_mmap.c and noting the assumption of caller privilege (PRIV_VM_MADV_PROTECT).
  • Added tests in GuestMemory.cpp checking invalid ranges with advice 10.

@nicolasfalesy

Copy link
Copy Markdown
Contributor

Linux test (Arch, GCC 16.2.1, glibc, Release), this PR merged onto main 7e181fc together with #4725, #4719, #4585 and #4582 (no conflicts): guest_memory passes. With this PR's source change reverted (test kept), it fails on Linux, so the check guards the change on Linux too.

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked against FreeBSD kern_madvise: advice 10 now returns before the range validation, the debt entry documents the privilege assumption, and the new guest_memory rows fail with the source change reverted. Build and ctest green locally (also confirmed on Linux by @nicolasfalesy); merging.

@adriadam10
adriadam10 merged commit a373241 into boykopovar:main Oct 13, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants