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

[Michael Walle <michael@walle.cc>]

Am 2022-07-29 17:39, schrieb Ajay.Kathat@microchip.com:
> On 29/07/22 20:28, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 29. Juli 2022 11:51:12 MESZ schrieb David Laight 
>> <David.Laight@ACULAB.COM>:
>>> From: Michael Walle
>>>> Sent: 28 July 2022 16:21
>>>> 
>>>> From: Michael Walle <mwalle@kernel.org>
>>>> 
>>>> 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@kernel.org>
>>>> ---
>>>> 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.
> 
> 
> I think, the above checks for each CMD53 might add up to the processing
> time of this function. These checks can be avoided, if we add new
> function similar to 'wilc_sdio_cmd53' which can be called when the 
> local
> variables are used. Though we have to perform the memcpy operation 
> which
> is anyway required to handle this scenario for small size data.
> 
> Mostly, either the static global data or dynamically allocated buffer 
> is
> used with cmd53 except wilc_sdio_write_reg, wilc_sdio_read_reg
> wilc_wlan_handle_txq functions.
> 
> I have created a patch using the above approach which can fix this 
> issue
> and will have no or minimal impact on existing functionality. The same
> is copied below:
> 
> 
> ---
>   .../net/wireless/microchip/wilc1000/netdev.h  |  1 +
>   .../net/wireless/microchip/wilc1000/sdio.c    | 46 
> +++++++++++++++++--
>   .../net/wireless/microchip/wilc1000/wlan.c    |  2 +-
>   3 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h
> b/drivers/net/wireless/microchip/wilc1000/netdev.h
> index 43c085c74b7a..2137ef294953 100644
> --- a/drivers/net/wireless/microchip/wilc1000/netdev.h
> +++ b/drivers/net/wireless/microchip/wilc1000/netdev.h
> @@ -245,6 +245,7 @@ struct wilc {
>       u8 *rx_buffer;
>       u32 rx_buffer_offset;
>       u8 *tx_buffer;
> +    u32 vmm_table[WILC_VMM_TBL_SIZE];
> 
>       struct txq_handle txq[NQUEUES];
>       int txq_entries;
> diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c
> b/drivers/net/wireless/microchip/wilc1000/sdio.c
> index 600cc57e9da2..19d4350ecc22 100644
> --- a/drivers/net/wireless/microchip/wilc1000/sdio.c
> +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
> @@ -28,6 +28,7 @@ struct wilc_sdio {
>       u32 block_size;
>       bool isinit;
>       int has_thrpt_enh3;
> +    u8 *dma_buffer;
>   };
> 
>   struct sdio_cmd52 {
> @@ -117,6 +118,36 @@ static int wilc_sdio_cmd53(struct wilc *wilc,
> struct sdio_cmd53 *cmd)
>       return ret;
>   }
> 
> +static int wilc_sdio_cmd53_extend(struct wilc *wilc, struct sdio_cmd53
> *cmd)

If you handle all the stack cases anyway, the caller can just use
a bounce buffer and you don't need to duplicate the function.

-michael