Re: [PATCH v2 05/23] dmaengine: sdxi: Configure context tables

[Nathan Lynch <nathan.lynch@amd.com>]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Premature programming of the L2 table base address in the hardware register, leading to a dangling DMA pointer on error paths.
> - [Medium] Unnecessary and potentially harmful `__packed` attribute on naturally aligned DMA structures.
> --
>
> commit 0a4d9e9c9ca24875b4f7b4512a3bf669c25c0595
> Author: Nathan Lynch <nathan.lynch@amd.com>
>
> dmaengine: sdxi: Configure context tables
>
> This commit allocates and configures the two-level context tracking tables (L1
> and L2) for the SDXI driver. It allocates enough space for context IDs 0-127
> and caps the maximum contexts accordingly, using devres-managed allocations to
> simplify cleanup.
>
>> diff --git a/drivers/dma/sdxi/device.c b/drivers/dma/sdxi/device.c
>> --- a/drivers/dma/sdxi/device.c
>> +++ b/drivers/dma/sdxi/device.c
>
> [ ... ]
>
>> @@ -172,6 +182,32 @@ static int sdxi_fn_activate(struct sdxi_dev *sdxi)
>>  			   FIELD_GET(SDXI_MMIO_CAP1_OPB_000_CAP, cap1));
>>  	sdxi_write64(sdxi, SDXI_MMIO_CTL2, ctl2);
>>  
>> +	/* SDXI 1.0 4.1.8.2 Context Level 2 Table Setup */
>> +	sdxi->L2_table = dmam_alloc_coherent(sdxi->dev,
>> +					     sizeof(*sdxi->L2_table),
>> +					     &sdxi->L2_dma, GFP_KERNEL);
>> +	if (!sdxi->L2_table)
>> +		return -ENOMEM;
>> +
>> +	cxt_l2 = FIELD_PREP(SDXI_MMIO_CXT_L2_PTR, sdxi->L2_dma >> ilog2(SZ_4K));
>> +	sdxi_write64(sdxi, SDXI_MMIO_CXT_L2, cxt_l2);
>> +
>> +	/* SDXI 1.0 4.1.8.3 Context Level 1 Table Setup */
>> +	sdxi->L1_table = dmam_alloc_coherent(sdxi->dev,
>> +					     sizeof(*sdxi->L1_table),
>> +					     &sdxi->L1_dma, GFP_KERNEL);
>> +	if (!sdxi->L1_table)
>> +		return -ENOMEM;
>
> If dmam_alloc_coherent() fails for L1_table, we return -ENOMEM. The devres
> framework will then free L2_table, but does this leave a dangling physical
> pointer in the hardware SDXI_MMIO_CXT_L2 register?
>
> Could this lead to a DMA use-after-free regression if the hardware incorrectly
> resumes or is later manipulated? Should we defer writing to SDXI_MMIO_CXT_L2
> until both tables are fully allocated and linked?

Yes, the error path potentially leaves MMIO_CXT_L2 programmed, but the
device is not going to access the table address until it actually gets
driven to active state. Maybe I'm missing something but I don't see any
issue here.