Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/ukallocbuddy: Add debug freelist sanity checks #932

Closed
wants to merge 2 commits into from

Conversation

andreittr
Copy link
Contributor

@andreittr andreittr commented Jun 5, 2023

Description of changes

This adds a Kconfig option to enable sanity checking of buddy allocator free lists at runtime. Check points are placed at the beginning and end of functions that operate on the free lists. When the option is disabled the checks default to zero-overhead no-ops.
In addition, this adds an assert that returned allocated pages are correctly aligned to their buddy order.

Furthermore, this PR preemptively hardens the page allocator against pathologically large numbers of requested pages which might cause out-of-bounds access to the freelist head array.

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): N/A
  • Platform(s): N/A
  • Application(s): N/A

Additional configuration

Can be tested with the code snippet from #931 (without applying that patch), should complain after the free() call.

@andreittr andreittr requested a review from a team as a code owner June 5, 2023 09:21
@unikraft-bot unikraft-bot added area/lib Internal Unikraft Microlibrary lang/c Issues or PRs to do with C/C++ lib/ukallocbbuddy labels Jun 5, 2023
@razvand razvand requested review from Starnox and flpostolache and removed request for a team and craciunoiuc June 15, 2023 16:11
@razvand razvand assigned razvand and unassigned nderjung Jun 15, 2023
@razvand razvand added this to the v0.14.0 (Prometheus) milestone Jun 15, 2023
Copy link
Contributor

@Starnox Starnox left a comment

Choose a reason for hiding this comment

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

It seems to catch the errors I tried throwing at it. Performance is not impacted when the option is not selected. For me it looks like a nice addition.

Reviewed-by: Eduard-Florin Mihailescu mihailescu.eduard@gmail.com

@andreittr
Copy link
Contributor Author

Update: additional commit hardening the out-of-memory check in the buddy page allocator.

@unikraft-bot
Copy link
Member

Checkpatch failed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request but it encountered errors:

SHA commit checkpatch
d856370 lib/ukallocbuddy: Add debug freelist sanity checks
ce14ad0 lib/ukallocbuddy: Harden buddy allocator OOM check

Truncated logs starting from first error d856370:

WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#60: FILE: lib/ukallocbbuddy/bbuddy.c:90:
+ * for consistency. Useful when suspecting memory corruption. */

ERROR:CODE_INDENT: code indent should use tabs where possible
#76: FILE: lib/ukallocbbuddy/bbuddy.c:106:
+^I^I^I          "got %u, expected %zu\n", \$

ERROR:CODE_INDENT: code indent should use tabs where possible
#77: FILE: lib/ukallocbbuddy/bbuddy.c:107:
+^I^I^I          c, i, head, off, c->level, i); \$

ERROR:CODE_INDENT: code indent should use tabs where possible
#81: FILE: lib/ukallocbbuddy/bbuddy.c:111:
+^I^I^I          c, i, head, off, c->pprev); \$

ERROR:CODE_INDENT: code indent should use tabs where possible
#84: FILE: lib/ukallocbbuddy/bbuddy.c:114:
+^I^I^I          "got %p, expected %p\n", \$

ERROR:CODE_INDENT: code indent should use tabs where possible
#85: FILE: lib/ukallocbbuddy/bbuddy.c:115:
+^I^I^I          c, i, head, off, *c->pprev, c); \$

ERROR:CODE_INDENT: code indent should use tabs where possible
#89: FILE: lib/ukallocbbuddy/bbuddy.c:119:
+^I^I^I          c, i, head, off, c->next); \$

ERROR:OPEN_BRACE: that open brace { should be on the previous line
#91: FILE: lib/ukallocbbuddy/bbuddy.c:121:
+		} else if (!FREELIST_ALIGNED(c->next, i) && \
+		           c->next->next != NULL) \
+		{ \

ERROR:CODE_INDENT: code indent should use tabs where possible
#92: FILE: lib/ukallocbbuddy/bbuddy.c:122:
+^I^I           c->next->next != NULL) \$

ERROR:CODE_INDENT: code indent should use tabs where possible
#95: FILE: lib/ukallocbbuddy/bbuddy.c:125:
+^I^I^I          "%p not aligned to %zx boundary\n", \$

ERROR:CODE_INDENT: code indent should use tabs where possible
#96: FILE: lib/ukallocbbuddy/bbuddy.c:126:
+^I^I^I          c, i, head, off, c->next, \$

ERROR:CODE_INDENT: code indent should use tabs where possible
#97: FILE: lib/ukallocbbuddy/bbuddy.c:127:
+^I^I^I          (size_t)1 << (__PAGE_SHIFT + i)); \$

total: 11 errors, 1 warnings, 138 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

NOTE: Whitespace errors detected.
      You may wish to use scripts/cleanpatch or scripts/cleanfile

/tmp/build/a53d4c5b/patches/0001-lib-ukallocbuddy-Add-debug-freelist-sanity-checks.patch has style problems, please review.

NOTE: Ignored message types: ASSIGN_IN_IF FILE_PATH_CHANGES NEW_TYPEDEFS OBSOLETE

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

View complete logs | Learn more about Unikraft's coding style and contribution guidelines.

Copy link
Contributor

@Starnox Starnox left a comment

Choose a reason for hiding this comment

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

LGTM!

Reviewed-by: Eduard-Florin Mihailescu mihailescu.eduard@gmail.com

@razvand razvand requested a review from StefanJum August 9, 2023 17:40
This adds a Kconfig option to enable sanity checking of buddy allocator
free lists at runtime. Check points are placed at the beginning and end
of functions that operate on the free lists. When the option is disabled
the checks default to zero-overhead no-ops.
In addition, this adds an assert that returned allocated pages are
correctly aligned to their buddy order.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
When the buddy allocator chooses a page order to allocate it can (for
pathologically large values of num_pages) start from a value larger than
FREELIST_SIZE, leading to out-of-bounds access of the freelist array.
This change hardens the out-of-memory check to prevent this.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
@andreittr
Copy link
Contributor Author

Rebased on staging & fixed some style issues (remaining checkpatch failures expected; will propose fixes to checkpatch).

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Thank you @andreittr, all good.
Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
When the buddy allocator chooses a page order to allocate it can (for
pathologically large values of num_pages) start from a value larger than
FREELIST_SIZE, leading to out-of-bounds access of the freelist array.
This change hardens the out-of-memory check to prevent this.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Eduard-Florin Mihailescu <mihailescu.eduard@gmail.com>
Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #932
@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 10, 2023
@andreittr andreittr deleted the ttr/buddysanity branch August 10, 2023 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lib Internal Unikraft Microlibrary ci/merged Merged by CI lang/c Issues or PRs to do with C/C++ lib/ukallocbbuddy
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

6 participants