Re: [PATCH 5/6] dmaengine: idxd: Fix error code handling in idxd_cdev_open()

[Vinicius Costa Gomes <vinicius.gomes@intel.com>]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] dmaengine: idxd: idxd_cdev_open() masks -ENOMEM from xa_insert()
> - [High] dmaengine: stranded PASID xarray entry on shared FD close breaks fault handling
> - [Medium] dmaengine: idxd: missing idxd_wq_disable_pasid() rollback on ida_alloc() failure
> --
>
> commit 63ea0ebbf579eb846dc4486575508a710df270fc
> Author: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>
> dmaengine: idxd: Fix error code handling in idxd_cdev_open()
>
> idxd_cdev_open() mishandles rc on two error paths: on ida_alloc() failure,
> it jumps to failed_ida without setting rc, returning 0. And when xa_insert()
> returns -EBUSY, rc is left negative and can leak out. This commit fixes
> both paths.
>
>> diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
>> index 82b07cf942ef8..abbab9bdd06c3 100644
>> --- a/drivers/dma/idxd/cdev.c
>> +++ b/drivers/dma/idxd/cdev.c
>> @@ -264,8 +264,11 @@ static int idxd_cdev_open(struct inode *inode, struct file *filp)
>>  		mutex_lock(&wq->uc_lock);
>>  		rc = xa_insert(&wq->upasid_xa, pasid, ctx, GFP_KERNEL);
>>  		mutex_unlock(&wq->uc_lock);
>> -		if (rc < 0)
>> -			dev_warn(dev, "PASID entry already exist in xarray.\n");
>> +		if (rc < 0) {
>> +			/* An existing entry is harmless, warn and continue */
>> +			dev_warn(dev, "PASID entry already exists in xarray.\n");
>> +			rc = 0;
>> +		}
>
> [Severity: High]
> Does this code unintentionally mask all negative return codes, including
> -ENOMEM?
>
> xa_insert() can return -ENOMEM if memory allocation fails. By unconditionally
> clearing all negative return codes (rc = 0), this allows the file open to
> succeed without registering the user context in the xarray.
>
> Later, when a subsequent hardware page fault occurs for this PASID,
> idxd_copy_cr() will call xa_load(), fail to find the context, and silently
> drop the completion record.
>

Will fix. Should have thought of -ENOMEM.

> [Severity: High]
> If a process opens multiple file descriptors to a shared IDXD workqueue,
> does treating -EBUSY as harmless leave the second FD stranded when the first
> FD is closed?
>
> When the first open succeeds, the context is inserted into the upasid_xa
> xarray. The second open fails the xa_insert with -EBUSY, which this code
> explicitly treats as harmless and continues, leaving the first context in
> the xarray.
>
> However, when the process closes the first file descriptor,
> idxd_xa_pasid_remove() successfully matches and removes the first context
> from the xarray:
>
> drivers/dma/idxd/cdev.c:idxd_xa_pasid_remove() {
> 	ptr = xa_cmpxchg(&wq->upasid_xa, ctx->pasid, ctx, NULL, GFP_KERNEL);
> }
>
> The second file descriptor remains open and active, but its PASID is no
> longer mapped in the xarray. If a hardware fault occurs for this PASID,
> idxd_copy_cr() will fail to find the context in the xarray, warn
> 'No user context', and silently drop the completion record.
>
>>  
>>  		if (wq_dedicated(wq)) {
>>  			rc = idxd_wq_set_pasid(wq, pasid);
>> @@ -279,6 +282,7 @@ static int idxd_cdev_open(struct inode *inode, struct file *filp)
>>  	idxd_cdev = wq->idxd_cdev;
>>  	ctx->id = ida_alloc(&file_ida, GFP_KERNEL);
>>  	if (ctx->id < 0) {
>> +		rc = ctx->id;
>>  		dev_warn(dev, "ida alloc failure\n");
>>  		goto failed_ida;
>>  	}

Another pre-existing issue. Will try to fix on v2, from a quick look,
seems that this one is one of the messier ones, that might need a
separate series. Let's see.

>
> [Severity: Medium]
> Does the failed_ida error path leak the PASID hardware configuration?
>
> During open(), if the WQ is dedicated, idxd_wq_set_pasid() programs the
> hardware WQ with the calling process's PASID. If the subsequent ida_alloc()
> fails, the error path (failed_ida) unbinds the PASID from the IOMMU and
> removes it from the xarray:
>
> drivers/dma/idxd/cdev.c:idxd_cdev_open() {
>     ...
> failed_ida:
> failed_set_pasid:
> 	if (device_user_pasid_enabled(idxd))
> 		idxd_xa_pasid_remove(ctx);
> failed_get_pasid:
> 	if (device_user_pasid_enabled(idxd) && !IS_ERR_OR_NULL(sva))
> 		iommu_sva_unbind_device(sva);
>     ...
> }
>
> However, it fails to call idxd_wq_disable_pasid() to clear it from the
> hardware WQ. This leaves the hardware WQ in the ENABLED state programmed
> with a stale, unbound PASID.
>

Another pre-existing issue, but should be more or less straightfoward to
fix. Will fix on v2.


Cheers,
-- 
Vinicius