On Thu, Apr 21, 2022 at 1:57 AM Nicholas Piggin <[email protected]> wrote:
>
> Those were (AFAIKS) all in arch code though.
No Nick, they really weren't.
The bpf issue with VM_FLUSH_RESET_PERMS means that all your arguments
are invalid, because this affected non-architecture code.
So the bpf case had two independent issues: one was just bpf doing a
really bad job at making sure the executable mapping was sanely
initialized.
But the other was an actual bug in that hugepage case for vmalloc.
And that bug was an issue on power too.
So your "this is purely an x86 issue" argument is simply wrong.
Because I'm very much looking at that power code that says "oh,
__module_alloc() needs more work".
Notice?
Can these be fixed? Yes. But they can't be fixed by saying "oh, let's
disable it on x86".
Although it's probably true that at that point, some of the issues
would no longer be nearly as noticeable.
Linus
Excerpts from Linus Torvalds's message of April 22, 2022 1:44 am:
> On Thu, Apr 21, 2022 at 1:57 AM Nicholas Piggin <[email protected]> wrote:
>>
>> Those were (AFAIKS) all in arch code though.
>
> No Nick, they really weren't.
>
> The bpf issue with VM_FLUSH_RESET_PERMS means that all your arguments
> are invalid, because this affected non-architecture code.
VM_FLUSH_RESET_PERMS was because bpf uses the arch module allocation
code which was not capable of dealing with huge pages in the arch
specific direct map manipulation stuff was unable to deal with it.
An x86 bug.
> So the bpf case had two independent issues: one was just bpf doing a
> really bad job at making sure the executable mapping was sanely
> initialized.
>
> But the other was an actual bug in that hugepage case for vmalloc.
>
> And that bug was an issue on power too.
I missed it, which bug was that?
>
> So your "this is purely an x86 issue" argument is simply wrong.
> Because I'm very much looking at that power code that says "oh,
> __module_alloc() needs more work".
>
> Notice?
No I don't notice. More work to support huge allocations for
executable mappings, sure. But the arch's implementation explicitly
does not support that yet. That doesn't make huge vmalloc broken!
Ridiculous. It works fine.
>
> Can these be fixed? Yes. But they can't be fixed by saying "oh, let's
> disable it on x86".
You did just effectively disable it on x86 though.
And why can't it be reverted on x86 until it's fixed on x86??
> Although it's probably true that at that point, some of the issues
> would no longer be nearly as noticeable.
There really aren't all these "issues" you're imagining. They
aren't noticable now, on power or s390, because they have
non-buggy HAVE_ARCH_HUGE_VMALLOC implementations.
If you're really going to insist on this will you apply this to fix
(some of) the performance regressions it introduced?
Thanks,
Nick
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 6e5b4488a0c5..b555f17e84d5 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -8919,7 +8919,10 @@ void *__init alloc_large_system_hash(const char *tablename,
table = memblock_alloc_raw(size,
SMP_CACHE_BYTES);
} else if (get_order(size) >= MAX_ORDER || hashdist) {
- table = __vmalloc(size, gfp_flags);
+ if (IS_ENABLED(CONFIG_PPC) || IS_ENABLED(CONFIG_S390))
+ table = vmalloc_huge(size, gfp_flags);
+ else
+ table = __vmalloc(size, gfp_flags);
virt = true;
if (table)
huge = is_vm_area_hugepages(table);
On Thu, Apr 21, 2022 at 4:30 PM Nicholas Piggin <[email protected]> wrote:
>
> VM_FLUSH_RESET_PERMS was because bpf uses the arch module allocation
> code which was not capable of dealing with huge pages in the arch
> specific direct map manipulation stuff was unable to deal with it.
> An x86 bug.
.. and a power one? The only thing really special in __module_alloc()
on power is that same VM_FLUSH_RESET_PERMS.
Why had you otherwise disabled it there on powerpc too?
> > And that bug was an issue on power too.
>
> I missed it, which bug was that?
See above. At least it's very strongly implied by how the powerpc
__module_alloc() function also used
VM_FLUSH_RESET_PERMS | VM_NO_HUGE_VMAP,
because there isn't much else going on.
> No I don't notice. More work to support huge allocations for
> executable mappings, sure. But the arch's implementation explicitly
> does not support that yet. That doesn't make huge vmalloc broken!
> Ridiculous. It works fine.
There are several other reports of problems that weren't related to
permissions (at least not obviously so).
You were pointed at one of them in this thread:
https://lore.kernel.org/all/[email protected]/
and yes, it also happened on x86-64, but my point this whole time has
been that x86-64 gets A *LOT* MORE TEST COVERAGE.
See the difference? The fact that it has workedc for you on powerpc
doesn't mean that it's fine on powerpc. It only means that powerpc
gets about one thousandth of the test coverage that x86-64 gets.
> You did just effectively disable it on x86 though.
I disabled it *EVERYWHERE*.
What is so hard to understand about that?
Why are you so convinced this is about x86?
It's not.
> There really aren't all these "issues" you're imagining. They
> aren't noticable now, on power or s390, because they have
> non-buggy HAVE_ARCH_HUGE_VMALLOC implementations.
So I really think you've not tested it.
How many of those powerpc or s390 machines do you think test drivers
that do vmalloc_to_page() and then do something with that 'struct page
*'?
Seriously. Why are you so convinced that "oh, any vmalloc() can be
converted to large pages"?
I really think the only reason you think that is because it ran on
machines that basically have almost no drivers in use, and are all
very homogenous, and just didn't happen to hit the bugs.
IOW, I think you are probably entirely right that x86 has its own set
of excitement (the bpf thread has this thing about how x86 does RO
differently from other architectures), and there are likely x86
specific bugs *TOO*.
But let's just pick a random driver that uses vmalloc (literally
random - I just grepped for it and started looking at it):
drivers/infiniband/hw/qib/qib_file_ops.c
and it unquestionably does absolutely disgusting things, and if
looking at the code makes you go "nobody should do that", then I won't
disagree all that much.
But as an example of what it does, it basically does things like this:
rcd->subctxt_uregbase = vmalloc_user(...);
and then you can mmap it in user space in mmap_kvaddr():
addr = rcd->subctxt_uregbase;
size = PAGE_SIZE * subctxt_cnt;
...
vma->vm_pgoff = (unsigned long) addr >> PAGE_SHIFT;
vma->vm_ops = &qib_file_vm_ops;
and then the page fault routine is:
static const struct vm_operations_struct qib_file_vm_ops = {
.fault = qib_file_vma_fault,
};
and that function qib_file_vma_fault() does things like this:
page = vmalloc_to_page((void *)(vmf->pgoff << PAGE_SHIFT));
if (!page)
return VM_FAULT_SIGBUS;
get_page(page);
vmf->page = page;
return 0;
and let me just claim
(a) I bet you have never EVER tested this kind of insane code on powerpc
(b) do you think this will work great if vmalloc() allocates large pages?
Can you now see what I'm saying?
Can you now admit that the whole "nmake vmalloc() do large pages
without explicit opt-in" was a HORRIBLE MISTAKE.
> If you're really going to insist on this will you apply this to fix
> (some of) the performance regressions it introduced?
No.
That patch is disgusting and there is no excuse for applying something
crap like that.
What I applied was the first in a series of patches that do it sanely.
That whole "a sane way forward" thing.
See
https://lore.kernel.org/all/[email protected]/
for [PATCH 2/4] in the series for this particular issue.
But I'm not applying anything else than the "disable this mess" before
we have more discussion and consensus.
And dammit, you had better just admit that this wasn't some x86-only thing.
Powerpc and s390 were BROKEN GARBAGE AND JUST DIDN'T HAVE THE TEST COVERAGE.
Seriously.
And the thing is, your opt-out approach was just POINTLESS. The actual
cases that are performance-critical are likely in the single digits.
It's probably just that big-hash case and maybe a *couple* of other
cases (ie the bpf jit really wants to use it).
So opt-in is clearly the correct thing to do.
Do you now understand why I applied that "users must opt-in" patch?
Do you now understand that this is not some "x86" thing?
Linus