linux-kernel - Re: [PATCH v4 for-next 1/2] RDMA/core: Provide rdma_user_mmap_disassociate() to disassociate mmap pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d76dd514-aceb-b7cb-705a-298fc905fae3@hisilicon.com>
Date: Mon, 9 Sep 2024 16:41:00 +0800
From: Junxian Huang <huangjunxian6@...ilicon.com>
To: Jason Gunthorpe <jgg@...pe.ca>
CC: <leon@...nel.org>, <linux-rdma@...r.kernel.org>, <linuxarm@...wei.com>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 for-next 1/2] RDMA/core: Provide
 rdma_user_mmap_disassociate() to disassociate mmap pages



On 2024/9/7 20:12, Jason Gunthorpe wrote:
> On Thu, Sep 05, 2024 at 09:11:54PM +0800, Junxian Huang wrote:
> 
>> @@ -698,11 +700,20 @@ static int ib_uverbs_mmap(struct file *filp, struct vm_area_struct *vma)
>>  	ucontext = ib_uverbs_get_ucontext_file(file);
>>  	if (IS_ERR(ucontext)) {
>>  		ret = PTR_ERR(ucontext);
>> -		goto out;
>> +		goto out_srcu;
>>  	}
>> +
>> +	mutex_lock(&file->disassociation_lock);
>> +	if (file->disassociated) {
>> +		ret = -EPERM;
>> +		goto out_mutex;
>> +	}
> 
> What sets disassociated back to false once the driver reset is
> completed?
> 
> I think you should probably drop this and instead add a lock and test
> inside the driver within its mmap op. While reset is ongoing fail all
> new mmaps.
> 

disassociated won't be set back to false. This is to stop new mmaps on
this ucontext even after reset is completed, because during hns reset,
all resources will be destroyed, and the ucontexts will become unavailable.

But of course, other drivers may handle this case differently from hns, so
I will remove disassociated here and put it in hns driver.

>>  	/*
>>  	 * Disassociation already completed, the VMA should already be zapped.
>>  	 */
>> -	if (!ufile->ucontext)
>> +	if (!ufile->ucontext || ufile->disassociated)
>>  		goto out_unlock;
> 
> Is this needed? It protects agains fork, but since the driver is still
> present I wonder if it is OK
> 

Will remove it too.

>> @@ -822,6 +837,8 @@ void uverbs_user_mmap_disassociate(struct ib_uverbs_file *ufile)
>>  	struct rdma_umap_priv *priv, *next_priv;
>>  
>>  	lockdep_assert_held(&ufile->hw_destroy_rwsem);
>> +	mutex_lock(&ufile->disassociation_lock);
>> +	ufile->disassociated = true;
> 
> I think this doesn't need the hw_destroy_rwsem anymore since you are
> using this new disassociation_lock instead. It doesn't make alot of
> sense to hold the hw_destroy_rwsem for read here, it was ment to be
> held for write.
> 

Then it seems we should remove the lockdep_assert_held() here?

Junxian

> Jason