On 27.10.20 09:38, Mike Rapoport wrote:
On Mon, Oct 26, 2020 at 06:05:30PM +0000, Edgecombe, Rick P wrote:
On Mon, 2020-10-26 at 11:05 +0200, Mike Rapoport wrote:
On Mon, Oct 26, 2020 at 01:13:52AM +0000, Edgecombe, Rick P wrote:
On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:
Indeed, for architectures that define
CONFIG_ARCH_HAS_SET_DIRECT_MAP
it is
possible that __kernel_map_pages() would fail, but since this
function is
void, the failure will go unnoticed.
Could you elaborate on how this could happen? Do you mean during
runtime today or if something new was introduced?
A failure in__kernel_map_pages() may happen today. For instance, on
x86
if the kernel is built with DEBUG_PAGEALLOC.
__kernel_map_pages(page, 1, 0);
will need to split, say, 2M page and during the split an allocation
of
page table could fail.
On x86 at least, DEBUG_PAGEALLOC expects to never have to break a page
on the direct map and even disables locking in cpa because it assumes
this. If this is happening somehow anyway then we should probably fix
that. Even if it's a debug feature, it will not be as useful if it is
causing its own crashes.
I'm still wondering if there is something I'm missing here. It seems
like you are saying there is a bug in some arch's, so let's add a WARN
in cross-arch code to log it as it crashes. A warn and making things
clearer seem like good ideas, but if there is a bug we should fix it.
The code around the callers still functionally assume re-mapping can't
fail.
Oh, I've meant x86 kernel *without* DEBUG_PAGEALLOC, and indeed the call
that unmaps pages back in safe_copy_page will just reset a 4K page to
NP because whatever made it NP at the first place already did the split.
Still, on arm64 with DEBUG_PAGEALLOC=n there is a possibility of a race
between map/unmap dance in __vunmap() and safe_copy_page() that may
cause access to unmapped memory:
__vunmap()
vm_remove_mappings()
set_direct_map_invalid()
safe_copy_page()
__kernel_map_pages()
return
do_copy_page() -> fault
This is a theoretical bug, but it is still not nice :)
Currently, the only user of __kernel_map_pages() outside
DEBUG_PAGEALLOC
is hibernation, but I think it would be safer to entirely prevent
usage
of __kernel_map_pages() when DEBUG_PAGEALLOC=n.
I totally agree it's error prone FWIW. On x86, my mental model of how
it is supposed to work is: If a page is 4k and NP it cannot fail to be
remapped. set_direct_map_invalid_noflush() should result in 4k NP
pages, and DEBUG_PAGEALLOC should result in all 4k pages on the direct
map. Are you seeing this violated or do I have wrong assumptions?
You are right, there is a set of assumptions about the remapping of the
direct map pages that make it all work, at least on x86.
But this is very subtle and it's not easy to wrap one's head around
this.
That's why putting __kernel_map_pages() out of "common" use and
keep it only for DEBUG_PAGEALLOC would make things clearer.
Beyond whatever you are seeing, for the latter case of new things
getting introduced to an interface with hidden dependencies... Another
edge case could be a new caller to set_memory_np() could result in
large NP pages. None of the callers today should cause this AFAICT, but
it's not great to rely on the callers to know these details.
A caller of set_memory_*() or set_direct_map_*() should expect a failure
and be ready for that. So adding a WARN to safe_copy_page() is the first
step in that direction :)
I am probably missing something important, but why are we
saving/restoring the content of pages that were explicitly removed from
the identity mapping such that nobody will access them?
Pages that are not allocated should contain garbage or be zero
(init_on_free). That should be easy to handle without ever reading the
page content.
The other user seems to be vm_remove_mappings(), where we only
*temporarily* remove the mapping - while hibernating, that code
shouldn't be active anymore I guess - or we could protect it from happening.
As I expressed in another mail, secretmem pages should rather not be
saved when hibernating - hibernation should be rather be disabled.
What am I missing?
--
Thanks,
David / dhildenb
