Message-Id: <1650590628.043zdepwk1.astroid@bobo.none>
Date:   Fri, 22 Apr 2022 11:51:35 +1000
From:   Nicholas Piggin <npiggin@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "ast@...nel.org" <ast@...nel.org>, "bp@...en8.de" <bp@...en8.de>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "dborkman@...hat.com" <dborkman@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "hch@...radead.org" <hch@...radead.org>,
        "hpa@...or.com" <hpa@...or.com>,
        "imbrenda@...ux.ibm.com" <imbrenda@...ux.ibm.com>,
        Kernel Team <Kernel-team@...com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "mbenes@...e.cz" <mbenes@...e.cz>,
        "mcgrof@...nel.org" <mcgrof@...nel.org>,
        "pmladek@...e.com" <pmladek@...e.com>,
        "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>,
        Mike Rapoport <rppt@...nel.org>,
        "song@...nel.org" <song@...nel.org>,
        Song Liu <songliubraving@...com>
Subject: Re: [PATCH v4 bpf 0/4] vmalloc: bpf: introduce VM_ALLOW_HUGE_VMAP

Excerpts from Linus Torvalds's message of April 22, 2022 10:49 am:
> On Thu, Apr 21, 2022 at 4:30 PM Nicholas Piggin <npiggin@...il.com> 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?

Right, it is for that (and possibly other similar ro->nx stuff for
module loading and instruction patching etc).

That's why we don't do huge vmalloc for executable mappings (yet).

I don't understand why you're trying to claim that limitation is a
bug in power or warrants disabling the whole thing for data mappings
as well.

> 
>> > 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.

So that's not a bug though. To be clear, VM_FLUSH_RESET_PERMS is
clearly never to be used in driver code, it's a low-level arch
specific detail. So the flag itself does not have to be usable
by any old random code.

> 
>> 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/14444103-d51b-0fb3-ee63-c3f182f0b546@molgen.mpg.de/

Right, I agree see the patch I posted.

> 
> 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.

Surely it's not unreasonable that there will be *some* bugs when
coverage is expanded.

I'm not saying the code I write is perfect and bug-free when I say
it's fine on power, I'm saying that in concept it is fine.

> 
>> 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?

Well the x86 bugs in its direct mapping stuff seem to be. If they
go away when not using huge mappings for modules then fine, I'm
just not really across all the issues but it sounded like there
were more than just this module case.

> 
> 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
> *'?

Probably quite a few, but the intersection of those with ones that
also allocate huge vmalloc *and* modify struct page fields to trip
that bug was probably empty.

> Seriously. Why are you so convinced that "oh, any vmalloc() can be
> converted to large pages"?

Because it can be transparent. The bug was (stupidly) using compound
pages when it should have just used split higher order pages.

But the whole idea of it is that it's supposed to be transparent to
drivers. I don't get why you're freaking out about vmalloc_to_page(),
a huge mapping is pretty much just like a small mapping except the
pages are contiguous and the page tables are a bit different.

> 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?

Yes of course it will, with said 2-line bugfix.

> 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.

No, can you admit that vmalloc_to_page is not some some crazy problem?
The drivers are doing fairly horrible things sure, but *nothing* can
look at the PTEs without going through accessors (except arch code
obviously, which has to adapt before enabling). vmalloc_to_page() just
gives you a struct page! Why would a driver care that it now happens
to be contiguous with the next one or not??

>> 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/20220415164413.2727220-3-song@kernel.org/
> 
> for [PATCH 2/4] in the series for this particular issue.


I was being facetious. The problem is you can't do ^ because x86 is
buggy.

> 
> 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.

Everybody writes bugs, nobody has 100% test coverage. I missed
something, big whoop. That doesn't invalidate the entire concept
of the feature.

> 
> 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?

No, opt-in for correctness is stupid and unecessary because callers 
shouldn't have to know about it. That's the whole point of the huge
vmalloc is that it's supposed to be transparent to non-arch code.

That drm screen buffer thing involved in the bug -- why wouldn't that
want to use huge pages?

> 
> Do you now understand that this is not some "x86" thing?
> 

If the only bug is that compound page thing and x86 is otherwise fine
then sure, and patch 1 was unnecessary to begin with.

Thanks,
Nick

Powered by blists - more mailing lists