neonvm-kernel: Patch null ptr deref in x86 pagetable setup #1392
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ref https://neondb.slack.com/archives/C0807C9SSJ2/p1748363834798349 ref https://neondb.slack.com/archives/C03TN5G758R/p1748944236243709 ref #1391 Broader discussion in the slack threads, but at a high level: 1. We started seeing null pointer dereferences after failed allocations in phys_pud_init() and friends 2. Those functions don't handle allocation failure (because normally they're called during boot, where the allocation cannot fail) 3. The allocation uses the GFP_ATOMIC flag, which *can* fail 4. This has been the case for a while, so whatever reason we're starting to see allocation failures, this code probably does need to be able to handle them. This change consists of two patches: * 00-phys_pgtable_nullptr.patch -- the actual change * 01-debug-phys_pgtable_nullptr.patch -- extra neon-specific logging so that we can scan for any VMs that would have hit the bug. The main idea of the change is to propagate allocation failures by returning zero from the phys_{p4d,pud,pmd}_init() functions, turning that into an -ENOMEM return when we get the chance. Those phys_{p4d,pud,pmd}_init() functions currently return the last physical address mapped; I've assumed that zero is not a valid return, and updated arch_add_memory() to return -ENOMEM if init_memory_mapping() returns zero. This matches the behavior of add_pages(), which is also called by arch_add_memory(). This patch was tested by injecting failure in phys_pud_init for a single call to alloc_low_page() after boot, confirming that: * an allocation failure will no longer cause a null pointer dereference * virtio-mem hotplug succeeds after the initial failure, if made to retry Check the slack threads for more detail on that testing.
sharnoff
changed the title
stradig
reviewed
Jun 3, 2025
Omrigan
reviewed
Jun 3, 2025
petuhovskiy
approved these changes
Jun 3, 2025
There was a problem hiding this comment.
The patches look good to me! However, I'm not familiar with this code in linux kernel at all, so I cannot say if it's correct – it's possible that error handling here should be more complicated, but I don't really know.
I checked that the patches shouldn't make anything worse – the new code will be executed only on allocation failures, and the current behaviour is NPE, so it cannot get any worse. So it should be totally ok to deploy it to staging/prod, if we want to.
Co-authored-by: Arthur Petukhovsky <[email protected]>
Co-authored-by: Arthur Petukhovsky <[email protected]>
Omrigan
added a commit
that referenced
this pull request
Jun 4, 2025
After a recent announcement [1] by Go team, saying they won't proceed with adding error handling syntax to Go, it is evident we have to implement a solution downstream. We already build Linux kernel with custom patches (see #1386, #1392), it is logical that as a next step we should have our own fork of Go, called Neon Go. The first feature is a syntax to handle errors. It supports the following syntax: ... := ... handle err { ... } Which is equivalent to: ..., err := ... if err != nil { ... } Advantages: 1. Easy to read. 2. Easy to write (no need to look for the ! key). Timeline to implement: 5 June - merge to main 6 June - deploy to staging 7 June - deploy to production [1]: https://go.dev/blog/error-syntax NOTE: this is, of course, a joke. I don't have any strong opinion about error handling in Go.
Omrigan
approved these changes
Jun 18, 2025
neonvm-kernel/patches/0001-x86-mm-Update-mapped-addresses-in-phys_-pmd-pud-_ini.patch
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ref https://neondb.slack.com/archives/C0807C9SSJ2/p1748363834798349
ref https://neondb.slack.com/archives/C03TN5G758R/p1748944236243709
ref #1391
Broader discussion in the slack threads, but at a high level:
phys_pud_init()and friendsGFP_ATOMICflag, which can failThis change consists of several patches, matching this thread on LKML.
The main idea of the change is to propagate allocation failures by changing the
phys_*_init()functions to return-ENOMEM(which in turn requires changing their return type).This patch was tested by injecting failure in
phys_pud_init()for a single call toalloc_low_page()after boot, confirming that:Check the slack threads for more detail on that testing.