Re: [GIT pull] core/urgent for v5.18-rc6

[Linus Torvalds]

On Sun, May 8, 2022 at 11:09 AM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> 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