[Intel-gfx] Possible use_mm() mis-uses

[Zhi Wang]

Hi Linus:

Thanks for letting us know that. We would fix this ASAP. The kvmgt.c 
module is a part of GVT-g code. It's our fault that we didn't find this 
mis-uses, not i915 or KVM guys. Wish they would feel better after seeing 
this message.

Thanks,
Zhi.

On 08/23/18 00:44, Linus Torvalds wrote:
> Guys and gals,
>   this is a *very* random list of people on the recipients list, but we
> had a subtle TLB shootdown issue in the VM, and that brought up some
> issues when people then went through the code more carefully.
> 
> I think we have a handle on the TLB shootdown bug itself. But when
> people were discussing all the possible situations, one thing that
> came up was "use_mm()" that takes a mm, and makes it temporarily the
> mm for a kernel thread (until "unuse_mm()", duh).
> 
> And it turns out that some of those uses are definitely wrong, some of
> them are right, and some of them are suspect or at least so overly
> complicated that it's hard for the VM people to know if they are ok.
> 
> Basically, the rule for "use_mm()" is that the mm in question *has* to
> have a valid page table associated with it over the whole use_mm() ->
> unuse_mm() sequence. That may sound obvious, and I guess it actually
> is so obvious that there isn't even a comment about it, but the actual
> users are showing that it's sadly apparently not so obvious after all.
> 
> There is one user that uses the "obviously correct" model: the vhost
> driver does a "mmget()" and "mmput()" pair around its use of it,
> thanks to vhost_dev_set_owner() doing a
> 
>          dev->mm = get_task_mm(current);
> 
> to look up the mm, and then the teardown case does a
> 
>          if (dev->mm)
>                  mmput(dev->mm);
>          dev->mm = NULL;
> 
> This is the *right* sequence. A gold star to the vhost people.
> 
> Sadly, the vhost people are the only ones who seem to get things
> unquestionably right. And some of those gold star people are also
> apparently involved in the cases that didn't get things right.
> 
> An example of something that *isn't* right, is the i915 kvm interface,
> which does
> 
>          use_mm(kvm->mm);
> 
> on an mm that was initialized in virt/kvm/kvm_main.c using
> 
>          mmgrab(current->mm);
>          kvm->mm = current->mm;
> 
> which is *not* right. Using "mmgrab()" does indeed guarantee the
> lifetime of the 'struct mm_struct' itself, but it does *not* guarantee
> the lifetime of the page tables. You need to use "mmget()" and
> "mmput()", which get the reference to the actual process address
> space!
> 
> Now, it is *possible* that the kvm use is correct too, because kvm
> does register a mmu_notifier chain, and in theory you can avoid the
> proper refcounting by just making sure the mmu "release" notifier
> kills any existing uses, but I don't really see kvm doing that. Kvm
> does register a release notifier, but that just flushes the shadow
> page tables, it doesn't kill any use_mm() use by some i915 use case.
> 
> So while the vhost use looks right, the kvm/i915 use looks definitely wrong.
> 
> The other users of "use_mm()" and "unuse_mm()" are less
> black-and-white right vs wrong..
> 
> One of the complex ones is the amdgpu driver. It does a
> "use_mm(mmptr)" deep deep in the guts of a macro that ends up being
> used in fa few places, and it's very hard to tell if it's right.
> 
> It looks almost certainly buggy (there is no mmget/mmput to get the
> refcount), but there _is_ a "release" mmu_notifier function and that
> one - unlike the kvm case - looks like it might actually be trying to
> flush the existing pending users of that mm.
> 
> But on the whole, I'm suspicious of the amdgpu use. It smells. Jann
> Horn pointed out that even if it migth be ok due to the mmu_notifier,
> the comments are garbage:
> 
>>   Where "process" in the uniquely-named "struct queue" is a "struct
>>   kfd_process"; that struct's definition has this comment in it:
>>
>>         /*
>>          * Opaque pointer to mm_struct. We don't hold a reference to
>>          * it so it should never be dereferenced from here. This is
>>          * only used for looking up processes by their mm.
>>          */
>>         void *mm;
>>
>>   So I think either that comment is wrong, or their code is wrong?
> 
> so I'm chalking the amdgpu use up in the "broken" column.
> 
> It's also actually quite hard to synchronze with some other kernel
> worker thread correctly, so just on general principles, if you use
> "use_mm()" it really really should be on something that you've
> properly gotten a mm refcount on with mmget(). Really. Even if you try
> to synchronize things.
> 
> The two final cases are two uses in the USB gadget driver. Again, they
> don't have the proper mmget/mmput, but they may br ok simply because
> the uses are done for AIO, and the VM teardown is preceded by an AIO
> teardown, so the proper serialization may come in from that.
> 
> Anyway, sorry for the long email, and the big list of participants and
> odd mailing lists, but I'd really like people to look at their
> "use_mm()" cases, and ask themselves if they have done enough to
> guarantee that the full mm exists. Again, "mmgrab()" is *not* enough
> on its own. You need either "mmget()" or some lifetime guarantee.
> 
> And if you do have those lifetime guarantees, it would be really nice
> to get a good explanatory comment about said lifetime guarantees above
> the "use_mm()" call. Ok?
> 
> Note that the lifetime rules are very important, because obviously
> use_mm() itself is never called in the context of the process that has
> the mm. By definition, we're in a kernel thread and it uses somebody
> elses mm. So it's important to show that that "somebody else" really
> is serialized with the kernel thread somehow, and keeps the mm alive.
> The easiest way by far to have that guarantee is to have that
> "mmget/mmput" model.
> 
> Please? Even if you're convinced your code is right, just a comment
> about _why_ it's right, ok?
> 
> And no, use_mm() cannot just do the mmget internally, only to have
> unuse_mm() do the mmput().  That was our first reaction when looking
> at this, but if the caller has screwed up the lifetime rules, it's not
> actually clear that the mm is guaranteed to be around even when use_mm
> is entered. So the onus of proper lifetime rules really is on the
> caller, not on use_mm()/unuse_mm().
> 
>               Linus
>