On 11/25/2020 10:02 AM, Jason Gunthorpe wrote:
On Mon, Nov 23, 2020 at 11:50:24AM -0500, Dennis Dalessandro wrote:
@@ -133,8 +121,16 @@ void hfi1_mmu_rb_unregister(struct mmu_rb_handler *handler) unsigned long flags; struct list_head del_list;
- /*
* do_exit() calls exit_mm() before exit_files() which would call close* and end up in here. If there is no mm, then its a kernel thread and* we need to let it continue the removal.*/- if (current->mm && (handler->mn.mm != current->mm))
return;- /* Unregister first so we don't get any more notifications. */
- mmu_notifier_unregister(&handler->mn, handler->mm);
- mmu_notifier_unregister(&handler->mn, handler->mn.mm);
This logic cannot be right.. The only caller does:
if (pq->handler) hfi1_mmu_rb_unregister(pq->handler);[..] kfree(pq);
So this is leaking the mmu_notifier registration if the user manages to trigger hfi1_user_sdma_free_queues() from another process.
Since hfi1_user_sdma_free_queues() is called from close() it doesn't look OK.
When the object that creates the notifier is destroyed the notifier should be deleted unconditionally.
Only accesses to a VA should be qualified to ensure that a notifier is registered on current->mm before touching the VA.
Ah yes. I think this just all goes away then. The context init and pq allocation is what triggers the registration, whenever we tear it down it should do the de-registration. v5 coming up after running tests.
-Denny