Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <165201148069.536527.1960632033331546251.tglx@xen13>
 <CAHk-=wjMmSZzMJ3Xnskdg4+GGz=5p5p+GSYyFBTh0f-DgvdBWg@mail.gmail.com> <CAHk-=whj6m0B9ys4XyE4AAPPoXE7XpBtxL7mD3Rk6EYX1=Ugng@mail.gmail.com>
In-Reply-To: <CAHk-=whj6m0B9ys4XyE4AAPPoXE7XpBtxL7mD3Rk6EYX1=Ugng@mail.gmail.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sun, 8 May 2022 11:36:23 -0700
Message-ID: <CAHk-=wiEC-Jdx4iTP5NQ0a8stsU+SL0DjbTwhQ+mvPGNBXyMag@mail.gmail.com>
Subject: Re: [GIT pull] core/urgent for v5.18-rc6
To:     Thomas Gleixner <tglx@linutronix.de>,
        Zhangfei Gao <zhangfei.gao@foxmail.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Jean-Philippe Brucker <jean-philippe@linaro.org>,
        Jacob Pan <jacob.jun.pan@linux.intel.com>,
        Dave Hansen <dave.hansen@intel.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "the arch/x86 maintainers" <x86@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, May 8, 2022 at 11:09 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Looks like it is
>
>  ->(*sva_bind)()
>     -> intel_svm_bind_mm()
>       -> mmu_notifier_register(&svm->notifier, mm)
>
> and yes, the mmu notifiers annoyingly end up doing an mmgrab [..]

Side note: quite independently of this mmgrab issue, I think the code
in question is *very* suspect and horrendously fragile.

In particular, the code ends up being called through things like this:

     handle = iommu_sva_bind_device(uacce->parent, current->mm, NULL);

and then that Intel svm.c code does this:

                svm->pasid = mm->pasid;
                svm->mm = mm;
                svm->flags = flags;

and saves off that mm pointer in the 'svm' structure.

AND IT NEVER TAKES ANY REFERENCE TO IT AT ALL!

It then does

        mm = svm->mm;

later at some unspecified time, and the 'mm' might long since have died.

In other words, the code works almost by accident - the only user of
the 'mm' pointer seems to be that mmu_notifier_register() thing, so it
basically treats the 'struct mm_struct' as something as a random
cookie.

And yes, the mmu notifiers do then take that mmgrab reference to the
mm, so it all works.

But it sure looks horrendously ugly. Saving off a 'struct mm_struct'
pointer with having basically an accidental reference to it is WRONG.

In fact, it will save off that pointer whether it then actually does
the mmu_notifier thing on it, because the code actually does

                [...]
                svm->mm = mm;
                svm->flags = flags;
                INIT_LIST_HEAD_RCU(&svm->devs);

                if (!(flags & SVM_FLAG_SUPERVISOR_MODE)) {
                        svm->notifier.ops = &intel_mmuops;
                        ret = mmu_notifier_register(&svm->notifier, mm);
                [...]

so that mmu_notifier_register() call is conditional. On the freeing
path, it then uses that "svm->notifier.ops" pointer as a "did we
register this thing or not" flag, so again - it all technically
*works*, but this is all horrendously ugly and wrong on so many
levels, keeping pointers around with very dubious reference counting
indeed.

               Linus