RE: [PATCH] wilc1000: fix DMA on stack objects

[Michael Walle]

Am 29. Juli 2022 11:51:12 MESZ schrieb David Laight <David.Laight@xxxxxxxxxx>:
>From: Michael Walle
>> Sent: 28 July 2022 16:21
>> 
>> From: Michael Walle <mwalle@xxxxxxxxxx>
>> 
>> Sometimes wilc_sdio_cmd53() is called with addresses pointing to an
>> object on the stack. E.g. wilc_sdio_write_reg() will call it with an
>> address pointing to one of its arguments. Detect whether the buffer
>> address is not DMA-able in which case a bounce buffer is used. The bounce
>> buffer itself is protected from parallel accesses by sdio_claim_host().
>> 
>> Fixes: 5625f965d764 ("wilc1000: move wilc driver out of staging")
>> Signed-off-by: Michael Walle <mwalle@xxxxxxxxxx>
>> ---
>> The bug itself probably goes back way more, but I don't know if it makes
>> any sense to use an older commit for the Fixes tag. If so, please suggest
>> one.
>> 
>> The bug leads to an actual error on an imx8mn SoC with 1GiB of RAM. But the
>> error will also be catched by CONFIG_DEBUG_VIRTUAL:
>> [    9.817512] virt_to_phys used for non-linear address: (____ptrval____) (0xffff80000a94bc9c)
>> 
>>  .../net/wireless/microchip/wilc1000/sdio.c    | 28 ++++++++++++++++---
>>  1 file changed, 24 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c
>> b/drivers/net/wireless/microchip/wilc1000/sdio.c
>> index 7962c11cfe84..e988bede880c 100644
>> --- a/drivers/net/wireless/microchip/wilc1000/sdio.c
>> +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
>> @@ -27,6 +27,7 @@ struct wilc_sdio {
>>  	bool irq_gpio;
>>  	u32 block_size;
>>  	int has_thrpt_enh3;
>> +	u8 *dma_buffer;
>>  };
>> 
>>  struct sdio_cmd52 {
>> @@ -89,6 +90,9 @@ static int wilc_sdio_cmd52(struct wilc *wilc, struct sdio_cmd52 *cmd)
>>  static int wilc_sdio_cmd53(struct wilc *wilc, struct sdio_cmd53 *cmd)
>>  {
>>  	struct sdio_func *func = container_of(wilc->dev, struct sdio_func, dev);
>> +	struct wilc_sdio *sdio_priv = wilc->bus_data;
>> +	bool need_bounce_buf = false;
>> +	u8 *buf = cmd->buffer;
>>  	int size, ret;
>> 
>>  	sdio_claim_host(func);
>> @@ -100,12 +104,20 @@ static int wilc_sdio_cmd53(struct wilc *wilc, struct sdio_cmd53 *cmd)
>>  	else
>>  		size = cmd->count;
>> 
>> +	if ((!virt_addr_valid(buf) || object_is_on_stack(buf)) &&
>
>How cheap are the above tests?
>It might just be worth always doing the 'bounce'?

I'm not sure how cheap they are, but I don't think it costs more than copying the bulk data around. That's up to the maintainer to decide. 

>
>> +	    !WARN_ON_ONCE(size > WILC_SDIO_BLOCK_SIZE)) {
>
>That WARN() ought to be an error return?

It will just behave as before. I noticed it *will* work in some cases, although the object is on the stack. I mean the driver seems to work fine at least on some SoCs. So I didn't want to change the behavior if the bounce buffer is too small. Of course we could also return an error here. All the calls with stack adresses I've seen for now were the register reads and writes and the txq handling (the vmm_tables IIRC). 

>Or just assume that large buffers will dma-capable?

See above. It should be large enough. But I didn't audit everything. 

-michael 

>
>	David
>
>> +		need_bounce_buf = true;
>> +		buf = sdio_priv->dma_buffer;
>> +	}
>> +
>>  	if (cmd->read_write) {  /* write */
>> -		ret = sdio_memcpy_toio(func, cmd->address,
>> -				       (void *)cmd->buffer, size);
>> +		if (need_bounce_buf)
>> +			memcpy(buf, cmd->buffer, size);
>> +		ret = sdio_memcpy_toio(func, cmd->address, buf, size);
>>  	} else {        /* read */
>> -		ret = sdio_memcpy_fromio(func, (void *)cmd->buffer,
>> -					 cmd->address,  size);
>> +		ret = sdio_memcpy_fromio(func, buf, cmd->address, size);
>> +		if (need_bounce_buf)
>> +			memcpy(cmd->buffer, buf, size);
>>  	}
>> 
>>  	sdio_release_host(func);
>> @@ -127,6 +139,12 @@ static int wilc_sdio_probe(struct sdio_func *func,
>>  	if (!sdio_priv)
>>  		return -ENOMEM;
>> 
>> +	sdio_priv->dma_buffer = kzalloc(WILC_SDIO_BLOCK_SIZE, GFP_KERNEL);
>> +	if (!sdio_priv->dma_buffer) {
>> +		ret = -ENOMEM;
>> +		goto free;
>> +	}
>> +
>>  	ret = wilc_cfg80211_init(&wilc, &func->dev, WILC_HIF_SDIO,
>>  				 &wilc_hif_sdio);
>>  	if (ret)
>> @@ -160,6 +178,7 @@ static int wilc_sdio_probe(struct sdio_func *func,
>>  	irq_dispose_mapping(wilc->dev_irq_num);
>>  	wilc_netdev_cleanup(wilc);
>>  free:
>> +	kfree(sdio_priv->dma_buffer);
>>  	kfree(sdio_priv);
>>  	return ret;
>>  }
>> @@ -171,6 +190,7 @@ static void wilc_sdio_remove(struct sdio_func *func)
>> 
>>  	clk_disable_unprepare(wilc->rtc_clk);
>>  	wilc_netdev_cleanup(wilc);
>> +	kfree(sdio_priv->dma_buffer);
>>  	kfree(sdio_priv);
>>  }
>> 
>> --
>> 2.30.2
>
>-
>Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
>Registration No: 1397386 (Wales)
>