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

[Michael Walle]

Am 2022-07-29 17:39, schrieb Ajay.Kathat@xxxxxxxxxxxxx:
> 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@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.
>
>
> 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