[PATCH] dmaengine: idxd: Avoid unnecessary destruction of file_ida

[PATCH] dmaengine: idxd: Avoid unnecessary destruction of file_ida

[Fenghua Yu]

file_ida is allocated during cdev open and is freed accordingly
during cdev release. This sequence is guaranteed by driver file
operations. Therefore, there is no need to destroy an already empty
file_ida when the WQ cdev is removed.

Worse, ida_free() in cdev release may happen after destruction of 
file_ida per WQ cdev. This can lead to accessing an id in file_ida
after it has been destroyed, resulting in a kernel panic.

Remove ida_destroy(&file_ida) to address these issues.

Fixes: e6fd6d7e5f0f ("dmaengine: idxd: add a device to represent the file opened")
Signed-off-by: Lijun Pan <lijun.pan@intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 drivers/dma/idxd/cdev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
index baa51927675c..3311c920f47a 100644
--- a/drivers/dma/idxd/cdev.c
+++ b/drivers/dma/idxd/cdev.c
@@ -1272,7 +1272,6 @@ void idxd_wq_del_cdev(struct idxd_wq *wq)
 	struct idxd_cdev *idxd_cdev;
 
 	idxd_cdev = wq->idxd_cdev;
-	ida_destroy(&file_ida);
 	wq->idxd_cdev = NULL;
 	cdev_device_del(&idxd_cdev->cdev, cdev_dev(idxd_cdev));
 	put_device(cdev_dev(idxd_cdev));
-- 
2.37.1

Re: [PATCH] dmaengine: idxd: Avoid unnecessary destruction of file_ida

[Dave Jiang]

On 1/29/24 18:39, Fenghua Yu wrote:
> file_ida is allocated during cdev open and is freed accordingly
> during cdev release. This sequence is guaranteed by driver file
> operations. Therefore, there is no need to destroy an already empty
> file_ida when the WQ cdev is removed.
> 
> Worse, ida_free() in cdev release may happen after destruction of 
> file_ida per WQ cdev. This can lead to accessing an id in file_ida
> after it has been destroyed, resulting in a kernel panic.
> 
> Remove ida_destroy(&file_ida) to address these issues.
> 
> Fixes: e6fd6d7e5f0f ("dmaengine: idxd: add a device to represent the file opened")
> Signed-off-by: Lijun Pan <lijun.pan@intel.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/dma/idxd/cdev.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
> index baa51927675c..3311c920f47a 100644
> --- a/drivers/dma/idxd/cdev.c
> +++ b/drivers/dma/idxd/cdev.c
> @@ -1272,7 +1272,6 @@ void idxd_wq_del_cdev(struct idxd_wq *wq)
>  	struct idxd_cdev *idxd_cdev;
>  
>  	idxd_cdev = wq->idxd_cdev;
> -	ida_destroy(&file_ida);
>  	wq->idxd_cdev = NULL;
>  	cdev_device_del(&idxd_cdev->cdev, cdev_dev(idxd_cdev));
>  	put_device(cdev_dev(idxd_cdev));

Re: [PATCH] dmaengine: idxd: Avoid unnecessary destruction of file_ida

[Fenghua Yu]

Hi, Vinod,

On 1/30/24 07:34, Dave Jiang wrote:
> 
> 
> On 1/29/24 18:39, Fenghua Yu wrote:
>> file_ida is allocated during cdev open and is freed accordingly
>> during cdev release. This sequence is guaranteed by driver file
>> operations. Therefore, there is no need to destroy an already empty
>> file_ida when the WQ cdev is removed.
>>
>> Worse, ida_free() in cdev release may happen after destruction of
>> file_ida per WQ cdev. This can lead to accessing an id in file_ida
>> after it has been destroyed, resulting in a kernel panic.
>>
>> Remove ida_destroy(&file_ida) to address these issues.
>>
>> Fixes: e6fd6d7e5f0f ("dmaengine: idxd: add a device to represent the file opened")
>> Signed-off-by: Lijun Pan <lijun.pan@intel.com>
>> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> 
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/dma/idxd/cdev.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
>> index baa51927675c..3311c920f47a 100644
>> --- a/drivers/dma/idxd/cdev.c
>> +++ b/drivers/dma/idxd/cdev.c
>> @@ -1272,7 +1272,6 @@ void idxd_wq_del_cdev(struct idxd_wq *wq)
>>   	struct idxd_cdev *idxd_cdev;
>>   
>>   	idxd_cdev = wq->idxd_cdev;
>> -	ida_destroy(&file_ida);
>>   	wq->idxd_cdev = NULL;
>>   	cdev_device_del(&idxd_cdev->cdev, cdev_dev(idxd_cdev));
>>   	put_device(cdev_dev(idxd_cdev));

I noticed this patch was not merged to upstream yet. The patch is still 
cleanly applied to upstream.

Could you please help merge this patch?

Thanks.

-Fenghua

Re: [PATCH] dmaengine: idxd: Avoid unnecessary destruction of file_ida

[Vinod Koul]

On Mon, 29 Jan 2024 17:39:54 -0800, Fenghua Yu wrote:
> file_ida is allocated during cdev open and is freed accordingly
> during cdev release. This sequence is guaranteed by driver file
> operations. Therefore, there is no need to destroy an already empty
> file_ida when the WQ cdev is removed.
> 
> Worse, ida_free() in cdev release may happen after destruction of
> file_ida per WQ cdev. This can lead to accessing an id in file_ida
> after it has been destroyed, resulting in a kernel panic.
> 
> [...]

Applied, thanks!

[1/1] dmaengine: idxd: Avoid unnecessary destruction of file_ida
      commit: 76e43fa6a456787bad31b8d0daeabda27351a480

Best regards,
-- 
~Vinod