Re: [PATCH for-rc v4] IB/hfi1: Ensure correct mm is used at all times

[Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>]

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