Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 28 Jun 2023 20:50:58 +0100
From:   Lorenzo Stoakes <lstoakes@gmail.com>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        mm-commits@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [GIT PULL] MM updates for 6.5-rc1
Message-ID: <986f48a6-5ec2-4f69-b1dc-72fe5b7ada77@lucifer.local>
References: <20230626085035.e66992e96b4c6d37dad54bd9@linux-foundation.org>
 <CAHk-=whppqqPdjVOx0mONDFx+JjewZPacionbWCUUFdrfOon-g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=whppqqPdjVOx0mONDFx+JjewZPacionbWCUUFdrfOon-g@mail.gmail.com>
Precedence: bulk

On Wed, Jun 28, 2023 at 12:19:42PM -0700, Linus Torvalds wrote:
> On Mon, 26 Jun 2023 at 08:50, Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > Linus, please merge the MM updates for the 6.5-rc cycle.
>
> Hmm. I have merged this, as pr-tracker-bot already noticed, but I
> found a bug after merging it.
>
> mm/memory.c: __access_remote_vm() is entirely broken for the
> HAVE_IOREMAP_PROT case (ie all normal architectures), because it does
> (skipping the non-HAVE_IOREMAP_PROT case):
>
>         struct vm_area_struct *vma = NULL;
>         struct page *page = get_user_page_vma_remote(mm, addr,
>                                                      gup_flags, &vma);
>
>         if (IS_ERR_OR_NULL(page)) {
>                 [ ... ]
>                 if (!vma)
>                         break;
>                 if (vma->vm_ops && vma->vm_ops->access)
>                         res = vma->vm_ops->access(vma, addr, buf, ...
>
> but get_user_page_vma_remote() doesn't even set vma if it fails!
>
> So that "if (!vma)" case will always trigger, and the whole ->access()
> thing is never done.

Ugh yeah...

This came about because the helper function handles the vma_lookup() case
but obviously we now only bother with the vma_lookup() if the gup_remote()
succeeds.

It's made a little trickier by the fact we warn on !vma if the gup
succeeded, so probably special casing this one makes sense for now.

There's been discussion about eliminating this weirdo thing gup returning 0
but being treated as a not-failure, I will probably try to patch this soon
for the one usecase where it matters (uprobes) and maybe also look at this
whole odd 'special mappings are ok in this one place' thing here.

>
> So that __access_remote_vm() conversion in commit ca5e863233e8
> ("mm/gup: remove vmas parameter from get_user_pages_remote()") is
> entirely broken.
>
> Now, I don't disagree with removing the vmas parameter. I just
> disagree with the get_user_page_vma_remote() helper use here.
>
> I think the minimal fix is to just put the vma_lookup() back in the error case:
>
>     --- a/mm/memory.c
>     +++ b/mm/memory.c
>     @@ -5592,6 +5592,7 @@ int __access_remote_vm(struct mm_struct *mm,
>                  * Check if this is a VM_IO | VM_PFNMAP VMA, which
>                  * we can access using slightly different code.
>                  */
>     +           vma = vma_lookup(mm, addr);
>                 if (!vma)
>                         break;
>                 if (vma->vm_ops && vma->vm_ops->access)
>
> and I'll commit that fix for now. Anybody who disagrees, please holler.
>
>                  Linus

Yeah I think this is the best fix in this case, we're not doing any
additional work since this wouldn't have run in the helper.