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

[<Ajay.Kathat@microchip.com>]

On 04/08/22 12:52, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> the content is safe
>
> 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.


Thanks. Indeed, the duplicate function can be avoided. I will update the 
patch and send modified patch for the review.
Btw, I was trying to reproduce the warning message by enabling 
CONFIG_DEBUG_VIRTUAL config but no luck. It seems enabling the config is 
not enough to test on my host or may be I am missing something. I would 
need the help to test and confirm if the modified patch do solve the 
issue with imx8mn.

Regards,
Ajay