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

[<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)
+{
+    struct sdio_func *func = container_of(wilc->dev, struct sdio_func, 
dev);
+    int size, ret;
+    struct wilc_sdio *sdio_priv = wilc->bus_data;
+    u8 *buf;
+
+    sdio_claim_host(func);
+
+    func->num = cmd->function;
+    func->cur_blksize = cmd->block_size;
+    size = cmd->count;
+    buf = sdio_priv->dma_buffer; /* use allocated memory */
+
+    if (cmd->read_write) {  /* write */
+        memcpy(buf, cmd->buffer, size);
+        ret = sdio_memcpy_toio(func, cmd->address, buf, size);
+    } else {        /* read */
+        ret = sdio_memcpy_fromio(func, buf, cmd->address, size);
+        memcpy(cmd->buffer, buf, size);
+    }
+
+    sdio_release_host(func);
+
+    if (ret)
+        dev_err(&func->dev, "%s..failed, err(%d)\n", __func__,  ret);
+
+    return ret;
+}
+
  static int wilc_sdio_probe(struct sdio_func *func,
                 const struct sdio_device_id *id)
  {
@@ -128,10 +159,16 @@ 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)
-        goto free;
+        goto free_dma_buffer;

      if (IS_ENABLED(CONFIG_WILC1000_HW_OOB_INTR)) {
          struct device_node *np = func->card->dev.of_node;
@@ -160,6 +197,8 @@ static int wilc_sdio_probe(struct sdio_func *func,
  dispose_irq:
      irq_dispose_mapping(wilc->dev_irq_num);
      wilc_netdev_cleanup(wilc);
+free_dma_buffer:
+    kfree(sdio_priv->dma_buffer);
  free:
      kfree(sdio_priv);
      return ret;
@@ -172,6 +211,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);
  }

@@ -378,7 +418,7 @@ static int wilc_sdio_write_reg(struct wilc *wilc, 
u32 addr, u32 data)
          cmd.count = 4;
          cmd.buffer = (u8 *)&data;
          cmd.block_size = sdio_priv->block_size;
-        ret = wilc_sdio_cmd53(wilc, &cmd);
+        ret = wilc_sdio_cmd53_extend(wilc, &cmd);
          if (ret)
              dev_err(&func->dev,
                  "Failed cmd53, write reg (%08x)...\n", addr);
@@ -496,7 +536,7 @@ static int wilc_sdio_read_reg(struct wilc *wilc, u32 
addr, u32 *data)
          cmd.buffer = (u8 *)data;

          cmd.block_size = sdio_priv->block_size;
-        ret = wilc_sdio_cmd53(wilc, &cmd);
+        ret = wilc_sdio_cmd53_extend(wilc, &cmd);
          if (ret) {
              dev_err(&func->dev,
                  "Failed cmd53, read reg (%08x)...\n", addr);
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c 
b/drivers/net/wireless/microchip/wilc1000/wlan.c
index 947d9a0a494e..0576d865c603 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -714,7 +714,7 @@ int wilc_wlan_handle_txq(struct wilc *wilc, u32 
*txq_count)
      int ret = 0;
      int counter;
      int timeout;
-    u32 vmm_table[WILC_VMM_TBL_SIZE];
+    u32 *vmm_table = wilc->vmm_table;
      u8 ac_pkt_num_to_chip[NQUEUES] = {0, 0, 0, 0};
      const struct wilc_hif_func *func;
      int srcu_idx;
-- 
2.34.1