Re: [PATCH] scsi: aacraid: fix dma_free_coherent() size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: aacraid: DMA unmap size mismatch due to shrunken can_queue
--

commit 1e9d5fb7ed09ff94558efbd312a1809b83cd68c2
Author: Thomas Fourier <fourier.thomas@gmail.com>

scsi: aacraid: fix dma_free_coherent() size

This commit updates the DMA unmap size calculation in aac_fib_map_free()
to use dev->max_cmd_size instead of dev->max_fib_size. This is intended to
match the initial allocation size in fib_map_alloc() and prevent leaving
parts of the memory mapped.

> diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
> index 0dba4406494a5..16cc4593a5c97 100644
> --- a/drivers/scsi/aacraid/commsup.c
> +++ b/drivers/scsi/aacraid/commsup.c
> @@ -81,7 +81,7 @@ void aac_fib_map_free(struct aac_dev *dev)
>  		return;
>  
>  	num_fibs = dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB;
> -	fib_size = dev->max_fib_size + sizeof(struct aac_fib_xporthdr);
> +	fib_size = dev->max_cmd_size + sizeof(struct aac_fib_xporthdr);
>  	alloc_size = fib_size * num_fibs + ALIGN32 - 1;

[Severity: High]
Does this recalculation still result in a DMA unmap size mismatch?

While this update corrects the fib_size multiplier, the num_fibs multiplier
is based on dev->scsi_host_ptr->can_queue, which can be modified after the
initial allocation.

For example, fib_map_alloc() allocates memory based on the initial value of
can_queue:

drivers/scsi/aacraid/commsup.c:fib_map_alloc() {
    ...
	dev->hw_fib_va = dma_alloc_coherent(&dev->pdev->dev,
		(dev->max_cmd_size + sizeof(struct aac_fib_xporthdr))
		* (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB) + (ALIGN32 - 1),
		&dev->hw_fib_pa, GFP_KERNEL);
    ...
}

However, for Rocket adapters, can_queue is shrunk after the allocation
successfully completes:

drivers/scsi/aacraid/rkt.c:aac_rkt_select_comm() {
    ...
			dev->scsi_host_ptr->can_queue = AAC_NUM_IO_FIB_RKT;
    ...
}

When aac_fib_map_free() is called, it recalculates the unmap size using the
newly shrunken can_queue, which causes a smaller size to be passed to
dma_free_coherent(). 

Will passing a mismatched smaller size to dma_free_coherent() violate the
DMA API contract and lead to leaked memory or corrupt allocator state?

>  
>  	dma_free_coherent(&dev->pdev->dev, alloc_size, dev->hw_fib_va,
>  			  dev->hw_fib_pa);
>  
>  	dev->hw_fib_va = NULL;
>  	dev->hw_fib_pa = 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016145420.50870-2-fourier.thomas@gmail.com?part=1