* [RFC PATCH] cw1200: use kmalloc() allocation instead of stack
@ 2021-06-22 20:23 Jernej Skrabec
2021-06-22 20:30 ` Arnd Bergmann
2021-06-30 10:03 ` Ulf Hansson
0 siblings, 2 replies; 10+ messages in thread
From: Jernej Skrabec @ 2021-06-22 20:23 UTC (permalink / raw)
To: pizza
Cc: ulf.hansson, arnd, kvalo, davem, kuba, linux-wireless, netdev,
linux-kernel, Jernej Skrabec
It turns out that if CONFIG_VMAP_STACK is enabled and src or dst is
memory allocated on stack, SDIO operations fail due to invalid memory
address conversion:
cw1200_wlan_sdio: Probe called
sunxi-mmc 4021000.mmc: DMA addr 0x0000800051eab954+4 overflow (mask ffffffff, bus limit 0).
WARNING: CPU: 2 PID: 152 at kernel/dma/direct.h:97 dma_direct_map_sg+0x26c/0x28c
CPU: 2 PID: 152 Comm: kworker/2:2 Not tainted 5.13.0-rc1-00026-g84114ef026b9-dirty #85
Hardware name: X96 Mate (DT)
Workqueue: events_freezable mmc_rescan
pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
pc : dma_direct_map_sg+0x26c/0x28c
lr : dma_direct_map_sg+0x26c/0x28c
sp : ffff800011eab540
x29: ffff800011eab540 x28: ffff800011eab738 x27: 0000000000000000
x26: ffff000001daf010 x25: 0000000000000000 x24: 0000000000000000
x23: 0000000000000002 x22: fffffc0000000000 x21: ffff8000113b0ab0
x20: ffff80001181abb0 x19: 0000000000000001 x18: ffffffffffffffff
x17: 00000000fa97f83f x16: 00000000d2e01bf8 x15: ffff8000117ffb1d
x14: ffffffffffffffff x13: ffff8000117ffb18 x12: fffffffffffc593f
x11: ffff800011676ad0 x10: fffffffffffe0000 x9 : ffff800011eab540
x8 : 206b73616d282077 x7 : 000000000000000f x6 : 000000000000000c
x5 : 0000000000000000 x4 : 0000000000000000 x3 : 00000000ffffffff
x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff00000283b800
Call trace:
dma_direct_map_sg+0x26c/0x28c
dma_map_sg_attrs+0x2c/0x60
sunxi_mmc_request+0x70/0x420
__mmc_start_request+0x68/0x134
mmc_start_request+0x84/0xac
mmc_wait_for_req+0x70/0x100
mmc_io_rw_extended+0x1cc/0x2c0
sdio_io_rw_ext_helper+0x194/0x240
sdio_memcpy_fromio+0x20/0x2c
cw1200_sdio_memcpy_fromio+0x20/0x2c
__cw1200_reg_read+0x34/0x60
cw1200_reg_read+0x48/0x70
cw1200_load_firmware+0x38/0x5d0
cw1200_core_probe+0x794/0x970
cw1200_sdio_probe+0x124/0x22c
sdio_bus_probe+0xe8/0x1d0
really_probe+0xe4/0x504
driver_probe_device+0x64/0xcc
__device_attach_driver+0xd0/0x14c
bus_for_each_drv+0x78/0xd0
__device_attach+0xdc/0x184
device_initial_probe+0x14/0x20
bus_probe_device+0x9c/0xa4
device_add+0x350/0x83c
sdio_add_func+0x6c/0x90
mmc_attach_sdio+0x1b0/0x430
mmc_rescan+0x254/0x2e0
process_one_work+0x1d0/0x34c
worker_thread+0x13c/0x470
kthread+0x154/0x160
ret_from_fork+0x10/0x34
sunxi-mmc 4021000.mmc: dma_map_sg failed
sunxi-mmc 4021000.mmc: map DMA failed
Can't read config register.
Fix that by using kmalloc() allocated memory for read/write 16/32
funtions.
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
drivers/net/wireless/st/cw1200/hwio.c | 52 +++++++++++++++++++++------
drivers/net/wireless/st/cw1200/hwio.h | 51 ++++++++++++++++++++------
2 files changed, 83 insertions(+), 20 deletions(-)
diff --git a/drivers/net/wireless/st/cw1200/hwio.c b/drivers/net/wireless/st/cw1200/hwio.c
index 3ba462de8e91..5521cb7f2233 100644
--- a/drivers/net/wireless/st/cw1200/hwio.c
+++ b/drivers/net/wireless/st/cw1200/hwio.c
@@ -66,33 +66,65 @@ static int __cw1200_reg_write(struct cw1200_common *priv, u16 addr,
static inline int __cw1200_reg_read_32(struct cw1200_common *priv,
u16 addr, u32 *val)
{
- __le32 tmp;
- int i = __cw1200_reg_read(priv, addr, &tmp, sizeof(tmp), 0);
- *val = le32_to_cpu(tmp);
+ __le32 *tmp;
+ int i;
+
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ i = __cw1200_reg_read(priv, addr, tmp, sizeof(*tmp), 0);
+ *val = le32_to_cpu(*tmp);
+ kfree(tmp);
return i;
}
static inline int __cw1200_reg_write_32(struct cw1200_common *priv,
u16 addr, u32 val)
{
- __le32 tmp = cpu_to_le32(val);
- return __cw1200_reg_write(priv, addr, &tmp, sizeof(tmp), 0);
+ __le32 *tmp;
+ int i;
+
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ *tmp = cpu_to_le32(val);
+ i = __cw1200_reg_write(priv, addr, tmp, sizeof(*tmp), 0);
+ kfree(tmp);
+ return i;
}
static inline int __cw1200_reg_read_16(struct cw1200_common *priv,
u16 addr, u16 *val)
{
- __le16 tmp;
- int i = __cw1200_reg_read(priv, addr, &tmp, sizeof(tmp), 0);
- *val = le16_to_cpu(tmp);
+ __le16 *tmp;
+ int i;
+
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ i = __cw1200_reg_read(priv, addr, tmp, sizeof(*tmp), 0);
+ *val = le16_to_cpu(*tmp);
+ kfree(tmp);
return i;
}
static inline int __cw1200_reg_write_16(struct cw1200_common *priv,
u16 addr, u16 val)
{
- __le16 tmp = cpu_to_le16(val);
- return __cw1200_reg_write(priv, addr, &tmp, sizeof(tmp), 0);
+ __le16 *tmp;
+ int i;
+
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ *tmp = cpu_to_le16(val);
+ i = __cw1200_reg_write(priv, addr, tmp, sizeof(*tmp), 0);
+ kfree(tmp);
+ return i;
}
int cw1200_reg_read(struct cw1200_common *priv, u16 addr, void *buf,
diff --git a/drivers/net/wireless/st/cw1200/hwio.h b/drivers/net/wireless/st/cw1200/hwio.h
index d1e629a566c2..088d2a1bacc0 100644
--- a/drivers/net/wireless/st/cw1200/hwio.h
+++ b/drivers/net/wireless/st/cw1200/hwio.h
@@ -166,34 +166,65 @@ int cw1200_reg_write(struct cw1200_common *priv, u16 addr,
static inline int cw1200_reg_read_16(struct cw1200_common *priv,
u16 addr, u16 *val)
{
- __le32 tmp;
+ __le32 *tmp;
int i;
- i = cw1200_reg_read(priv, addr, &tmp, sizeof(tmp));
- *val = le32_to_cpu(tmp) & 0xfffff;
+
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ i = cw1200_reg_read(priv, addr, tmp, sizeof(*tmp));
+ *val = le32_to_cpu(*tmp) & 0xfffff;
+ kfree(tmp);
return i;
}
static inline int cw1200_reg_write_16(struct cw1200_common *priv,
u16 addr, u16 val)
{
- __le32 tmp = cpu_to_le32((u32)val);
- return cw1200_reg_write(priv, addr, &tmp, sizeof(tmp));
+ __le32 *tmp;
+ int i;
+
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ *tmp = cpu_to_le32((u32)val);
+ i = cw1200_reg_write(priv, addr, tmp, sizeof(*tmp));
+ kfree(tmp);
+ return i;
}
static inline int cw1200_reg_read_32(struct cw1200_common *priv,
u16 addr, u32 *val)
{
- __le32 tmp;
- int i = cw1200_reg_read(priv, addr, &tmp, sizeof(tmp));
- *val = le32_to_cpu(tmp);
+ __le32 *tmp;
+ int i;
+
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ i = cw1200_reg_read(priv, addr, tmp, sizeof(*tmp));
+ *val = le32_to_cpu(*tmp);
+ kfree(tmp);
return i;
}
static inline int cw1200_reg_write_32(struct cw1200_common *priv,
u16 addr, u32 val)
{
- __le32 tmp = cpu_to_le32(val);
- return cw1200_reg_write(priv, addr, &tmp, sizeof(val));
+ __le32 *tmp;
+ int i;
+
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ *tmp = cpu_to_le32(val);
+ i = cw1200_reg_write(priv, addr, tmp, sizeof(val));
+ kfree(tmp);
+ return i;
}
int cw1200_indirect_read(struct cw1200_common *priv, u32 addr, void *buf,
--
2.32.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] cw1200: use kmalloc() allocation instead of stack
2021-06-22 20:23 [RFC PATCH] cw1200: use kmalloc() allocation instead of stack Jernej Skrabec
@ 2021-06-22 20:30 ` Arnd Bergmann
2021-06-30 9:55 ` Ulf Hansson
2021-06-30 10:03 ` Ulf Hansson
1 sibling, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2021-06-22 20:30 UTC (permalink / raw)
To: Jernej Skrabec
Cc: pizza, Ulf Hansson, Kalle Valo, David Miller, Jakub Kicinski,
linux-wireless, Networking, Linux Kernel Mailing List
On Tue, Jun 22, 2021 at 10:24 PM Jernej Skrabec
<jernej.skrabec@gmail.com> wrote:
>
> It turns out that if CONFIG_VMAP_STACK is enabled and src or dst is
> memory allocated on stack, SDIO operations fail due to invalid memory
> address conversion:
Thank you for sending this!
It's worth pointing out that even without CONFIG_VMAP_STACK, using
dma_map_sg() on a stack variable is broken, though it will appear to
work most of the time but rarely cause a stack data corruption when
the cache management goes wrong.
This clearly needs to be fixed somewhere, if not with your patch, then
a similar one.
> diff --git a/drivers/net/wireless/st/cw1200/hwio.c b/drivers/net/wireless/st/cw1200/hwio.c
> index 3ba462de8e91..5521cb7f2233 100644
> --- a/drivers/net/wireless/st/cw1200/hwio.c
> +++ b/drivers/net/wireless/st/cw1200/hwio.c
> @@ -66,33 +66,65 @@ static int __cw1200_reg_write(struct cw1200_common *priv, u16 addr,
> static inline int __cw1200_reg_read_32(struct cw1200_common *priv,
> u16 addr, u32 *val)
> {
> - __le32 tmp;
> - int i = __cw1200_reg_read(priv, addr, &tmp, sizeof(tmp), 0);
> - *val = le32_to_cpu(tmp);
> + __le32 *tmp;
> + int i;
> +
> + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + i = __cw1200_reg_read(priv, addr, tmp, sizeof(*tmp), 0);
> + *val = le32_to_cpu(*tmp);
> + kfree(tmp);
> return i;
> }
There is a possible problem here when the function gets called from
atomic context, so it might need to use GFP_ATOMIC instead of
GFP_KERNEL. If it's never called from atomic context, then this patch
looks correct to me.
The alternative would be to add a bounce buffer check based on
is_vmalloc_or_module_addr() in sdio_io_rw_ext_helper(), which would
add a small bit of complexity there but solve the problem for
all drivers at once. In this case, it would probably have to use
GFP_ATOMIC regardless of whether __cw1200_reg_read_32()
is allowed to sleep, since other callers might not.
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] cw1200: use kmalloc() allocation instead of stack
2021-06-22 20:30 ` Arnd Bergmann
@ 2021-06-30 9:55 ` Ulf Hansson
2021-06-30 11:30 ` Arnd Bergmann
0 siblings, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2021-06-30 9:55 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jernej Skrabec, pizza, Kalle Valo, David Miller, Jakub Kicinski,
linux-wireless, Networking, Linux Kernel Mailing List
On Tue, 22 Jun 2021 at 22:33, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Jun 22, 2021 at 10:24 PM Jernej Skrabec
> <jernej.skrabec@gmail.com> wrote:
> >
> > It turns out that if CONFIG_VMAP_STACK is enabled and src or dst is
> > memory allocated on stack, SDIO operations fail due to invalid memory
> > address conversion:
>
> Thank you for sending this!
>
> It's worth pointing out that even without CONFIG_VMAP_STACK, using
> dma_map_sg() on a stack variable is broken, though it will appear to
> work most of the time but rarely cause a stack data corruption when
> the cache management goes wrong.
>
> This clearly needs to be fixed somewhere, if not with your patch, then
> a similar one.
>
> > diff --git a/drivers/net/wireless/st/cw1200/hwio.c b/drivers/net/wireless/st/cw1200/hwio.c
> > index 3ba462de8e91..5521cb7f2233 100644
> > --- a/drivers/net/wireless/st/cw1200/hwio.c
> > +++ b/drivers/net/wireless/st/cw1200/hwio.c
> > @@ -66,33 +66,65 @@ static int __cw1200_reg_write(struct cw1200_common *priv, u16 addr,
> > static inline int __cw1200_reg_read_32(struct cw1200_common *priv,
> > u16 addr, u32 *val)
> > {
> > - __le32 tmp;
> > - int i = __cw1200_reg_read(priv, addr, &tmp, sizeof(tmp), 0);
> > - *val = le32_to_cpu(tmp);
> > + __le32 *tmp;
> > + int i;
> > +
> > + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> > + if (!tmp)
> > + return -ENOMEM;
> > +
> > + i = __cw1200_reg_read(priv, addr, tmp, sizeof(*tmp), 0);
> > + *val = le32_to_cpu(*tmp);
> > + kfree(tmp);
> > return i;
> > }
>
> There is a possible problem here when the function gets called from
> atomic context, so it might need to use GFP_ATOMIC instead of
> GFP_KERNEL. If it's never called from atomic context, then this patch
> looks correct to me.
I would be surprised if this is called from atomic context (when IRQs
are turned off), because in most cases, to complete the read/write
request the mmc controller driver relies on IRQs being delivered.
>
> The alternative would be to add a bounce buffer check based on
> is_vmalloc_or_module_addr() in sdio_io_rw_ext_helper(), which would
> add a small bit of complexity there but solve the problem for
> all drivers at once. In this case, it would probably have to use
> GFP_ATOMIC regardless of whether __cw1200_reg_read_32()
> is allowed to sleep, since other callers might not.
I like the idea, but...
I don't think we should see this as an alternative, but rather as a
complement which would have performance issues. A warning should be
printed, if the buffer isn't properly allocated.
Additionally, I don't think GFT_ATOMIC should be needed.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] cw1200: use kmalloc() allocation instead of stack
2021-06-22 20:23 [RFC PATCH] cw1200: use kmalloc() allocation instead of stack Jernej Skrabec
2021-06-22 20:30 ` Arnd Bergmann
@ 2021-06-30 10:03 ` Ulf Hansson
2021-06-30 10:09 ` Jernej Škrabec
2021-06-30 16:08 ` David Laight
1 sibling, 2 replies; 10+ messages in thread
From: Ulf Hansson @ 2021-06-30 10:03 UTC (permalink / raw)
To: Jernej Skrabec
Cc: pizza, Arnd Bergmann, Kalle Valo, David S. Miller,
Jakub Kicinski, linux-wireless, netdev,
Linux Kernel Mailing List
On Tue, 22 Jun 2021 at 22:23, Jernej Skrabec <jernej.skrabec@gmail.com> wrote:
>
> It turns out that if CONFIG_VMAP_STACK is enabled and src or dst is
> memory allocated on stack, SDIO operations fail due to invalid memory
> address conversion:
>
> cw1200_wlan_sdio: Probe called
> sunxi-mmc 4021000.mmc: DMA addr 0x0000800051eab954+4 overflow (mask ffffffff, bus limit 0).
> WARNING: CPU: 2 PID: 152 at kernel/dma/direct.h:97 dma_direct_map_sg+0x26c/0x28c
> CPU: 2 PID: 152 Comm: kworker/2:2 Not tainted 5.13.0-rc1-00026-g84114ef026b9-dirty #85
> Hardware name: X96 Mate (DT)
> Workqueue: events_freezable mmc_rescan
> pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
> pc : dma_direct_map_sg+0x26c/0x28c
> lr : dma_direct_map_sg+0x26c/0x28c
> sp : ffff800011eab540
> x29: ffff800011eab540 x28: ffff800011eab738 x27: 0000000000000000
> x26: ffff000001daf010 x25: 0000000000000000 x24: 0000000000000000
> x23: 0000000000000002 x22: fffffc0000000000 x21: ffff8000113b0ab0
> x20: ffff80001181abb0 x19: 0000000000000001 x18: ffffffffffffffff
> x17: 00000000fa97f83f x16: 00000000d2e01bf8 x15: ffff8000117ffb1d
> x14: ffffffffffffffff x13: ffff8000117ffb18 x12: fffffffffffc593f
> x11: ffff800011676ad0 x10: fffffffffffe0000 x9 : ffff800011eab540
> x8 : 206b73616d282077 x7 : 000000000000000f x6 : 000000000000000c
> x5 : 0000000000000000 x4 : 0000000000000000 x3 : 00000000ffffffff
> x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff00000283b800
> Call trace:
> dma_direct_map_sg+0x26c/0x28c
> dma_map_sg_attrs+0x2c/0x60
> sunxi_mmc_request+0x70/0x420
> __mmc_start_request+0x68/0x134
> mmc_start_request+0x84/0xac
> mmc_wait_for_req+0x70/0x100
> mmc_io_rw_extended+0x1cc/0x2c0
> sdio_io_rw_ext_helper+0x194/0x240
> sdio_memcpy_fromio+0x20/0x2c
> cw1200_sdio_memcpy_fromio+0x20/0x2c
> __cw1200_reg_read+0x34/0x60
> cw1200_reg_read+0x48/0x70
> cw1200_load_firmware+0x38/0x5d0
> cw1200_core_probe+0x794/0x970
> cw1200_sdio_probe+0x124/0x22c
> sdio_bus_probe+0xe8/0x1d0
> really_probe+0xe4/0x504
> driver_probe_device+0x64/0xcc
> __device_attach_driver+0xd0/0x14c
> bus_for_each_drv+0x78/0xd0
> __device_attach+0xdc/0x184
> device_initial_probe+0x14/0x20
> bus_probe_device+0x9c/0xa4
> device_add+0x350/0x83c
> sdio_add_func+0x6c/0x90
> mmc_attach_sdio+0x1b0/0x430
> mmc_rescan+0x254/0x2e0
> process_one_work+0x1d0/0x34c
> worker_thread+0x13c/0x470
> kthread+0x154/0x160
> ret_from_fork+0x10/0x34
> sunxi-mmc 4021000.mmc: dma_map_sg failed
> sunxi-mmc 4021000.mmc: map DMA failed
> Can't read config register.
>
> Fix that by using kmalloc() allocated memory for read/write 16/32
> funtions.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> ---
> drivers/net/wireless/st/cw1200/hwio.c | 52 +++++++++++++++++++++------
> drivers/net/wireless/st/cw1200/hwio.h | 51 ++++++++++++++++++++------
> 2 files changed, 83 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/wireless/st/cw1200/hwio.c b/drivers/net/wireless/st/cw1200/hwio.c
> index 3ba462de8e91..5521cb7f2233 100644
> --- a/drivers/net/wireless/st/cw1200/hwio.c
> +++ b/drivers/net/wireless/st/cw1200/hwio.c
> @@ -66,33 +66,65 @@ static int __cw1200_reg_write(struct cw1200_common *priv, u16 addr,
> static inline int __cw1200_reg_read_32(struct cw1200_common *priv,
> u16 addr, u32 *val)
> {
> - __le32 tmp;
> - int i = __cw1200_reg_read(priv, addr, &tmp, sizeof(tmp), 0);
> - *val = le32_to_cpu(tmp);
> + __le32 *tmp;
> + int i;
> +
> + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + i = __cw1200_reg_read(priv, addr, tmp, sizeof(*tmp), 0);
> + *val = le32_to_cpu(*tmp);
> + kfree(tmp);
> return i;
> }
>
> static inline int __cw1200_reg_write_32(struct cw1200_common *priv,
> u16 addr, u32 val)
> {
> - __le32 tmp = cpu_to_le32(val);
> - return __cw1200_reg_write(priv, addr, &tmp, sizeof(tmp), 0);
> + __le32 *tmp;
> + int i;
> +
> + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + *tmp = cpu_to_le32(val);
> + i = __cw1200_reg_write(priv, addr, tmp, sizeof(*tmp), 0);
> + kfree(tmp);
> + return i;
> }
>
> static inline int __cw1200_reg_read_16(struct cw1200_common *priv,
> u16 addr, u16 *val)
> {
> - __le16 tmp;
> - int i = __cw1200_reg_read(priv, addr, &tmp, sizeof(tmp), 0);
> - *val = le16_to_cpu(tmp);
> + __le16 *tmp;
> + int i;
> +
> + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + i = __cw1200_reg_read(priv, addr, tmp, sizeof(*tmp), 0);
> + *val = le16_to_cpu(*tmp);
> + kfree(tmp);
> return i;
> }
>
> static inline int __cw1200_reg_write_16(struct cw1200_common *priv,
> u16 addr, u16 val)
> {
> - __le16 tmp = cpu_to_le16(val);
> - return __cw1200_reg_write(priv, addr, &tmp, sizeof(tmp), 0);
> + __le16 *tmp;
> + int i;
> +
> + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + *tmp = cpu_to_le16(val);
> + i = __cw1200_reg_write(priv, addr, tmp, sizeof(*tmp), 0);
> + kfree(tmp);
> + return i;
> }
>
> int cw1200_reg_read(struct cw1200_common *priv, u16 addr, void *buf,
> diff --git a/drivers/net/wireless/st/cw1200/hwio.h b/drivers/net/wireless/st/cw1200/hwio.h
> index d1e629a566c2..088d2a1bacc0 100644
> --- a/drivers/net/wireless/st/cw1200/hwio.h
> +++ b/drivers/net/wireless/st/cw1200/hwio.h
> @@ -166,34 +166,65 @@ int cw1200_reg_write(struct cw1200_common *priv, u16 addr,
> static inline int cw1200_reg_read_16(struct cw1200_common *priv,
> u16 addr, u16 *val)
> {
> - __le32 tmp;
> + __le32 *tmp;
> int i;
> - i = cw1200_reg_read(priv, addr, &tmp, sizeof(tmp));
> - *val = le32_to_cpu(tmp) & 0xfffff;
> +
> + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + i = cw1200_reg_read(priv, addr, tmp, sizeof(*tmp));
> + *val = le32_to_cpu(*tmp) & 0xfffff;
> + kfree(tmp);
> return i;
> }
>
> static inline int cw1200_reg_write_16(struct cw1200_common *priv,
> u16 addr, u16 val)
> {
> - __le32 tmp = cpu_to_le32((u32)val);
> - return cw1200_reg_write(priv, addr, &tmp, sizeof(tmp));
> + __le32 *tmp;
> + int i;
> +
> + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + *tmp = cpu_to_le32((u32)val);
> + i = cw1200_reg_write(priv, addr, tmp, sizeof(*tmp));
> + kfree(tmp);
> + return i;
> }
>
> static inline int cw1200_reg_read_32(struct cw1200_common *priv,
> u16 addr, u32 *val)
> {
> - __le32 tmp;
> - int i = cw1200_reg_read(priv, addr, &tmp, sizeof(tmp));
> - *val = le32_to_cpu(tmp);
> + __le32 *tmp;
> + int i;
> +
> + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + i = cw1200_reg_read(priv, addr, tmp, sizeof(*tmp));
> + *val = le32_to_cpu(*tmp);
> + kfree(tmp);
> return i;
> }
>
> static inline int cw1200_reg_write_32(struct cw1200_common *priv,
> u16 addr, u32 val)
> {
> - __le32 tmp = cpu_to_le32(val);
> - return cw1200_reg_write(priv, addr, &tmp, sizeof(val));
> + __le32 *tmp;
> + int i;
> +
> + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + *tmp = cpu_to_le32(val);
> + i = cw1200_reg_write(priv, addr, tmp, sizeof(val));
> + kfree(tmp);
> + return i;
> }
>
> int cw1200_indirect_read(struct cw1200_common *priv, u32 addr, void *buf,
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] cw1200: use kmalloc() allocation instead of stack
2021-06-30 10:03 ` Ulf Hansson
@ 2021-06-30 10:09 ` Jernej Škrabec
2021-06-30 12:00 ` Ulf Hansson
2021-06-30 16:08 ` David Laight
1 sibling, 1 reply; 10+ messages in thread
From: Jernej Škrabec @ 2021-06-30 10:09 UTC (permalink / raw)
To: Ulf Hansson
Cc: pizza, Arnd Bergmann, Kalle Valo, David S. Miller,
Jakub Kicinski, linux-wireless, netdev,
Linux Kernel Mailing List
Hi Ulf!
Dne sreda, 30. junij 2021 ob 12:03:13 CEST je Ulf Hansson napisal(a):
> On Tue, 22 Jun 2021 at 22:23, Jernej Skrabec <jernej.skrabec@gmail.com>
wrote:
> > It turns out that if CONFIG_VMAP_STACK is enabled and src or dst is
> > memory allocated on stack, SDIO operations fail due to invalid memory
> > address conversion:
> >
> > cw1200_wlan_sdio: Probe called
> > sunxi-mmc 4021000.mmc: DMA addr 0x0000800051eab954+4 overflow (mask
> > ffffffff, bus limit 0). WARNING: CPU: 2 PID: 152 at
> > kernel/dma/direct.h:97 dma_direct_map_sg+0x26c/0x28c CPU: 2 PID: 152
> > Comm: kworker/2:2 Not tainted 5.13.0-rc1-00026-g84114ef026b9-dirty #85
> > Hardware name: X96 Mate (DT)
> > Workqueue: events_freezable mmc_rescan
> > pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
> > pc : dma_direct_map_sg+0x26c/0x28c
> > lr : dma_direct_map_sg+0x26c/0x28c
> > sp : ffff800011eab540
> > x29: ffff800011eab540 x28: ffff800011eab738 x27: 0000000000000000
> > x26: ffff000001daf010 x25: 0000000000000000 x24: 0000000000000000
> > x23: 0000000000000002 x22: fffffc0000000000 x21: ffff8000113b0ab0
> > x20: ffff80001181abb0 x19: 0000000000000001 x18: ffffffffffffffff
> > x17: 00000000fa97f83f x16: 00000000d2e01bf8 x15: ffff8000117ffb1d
> > x14: ffffffffffffffff x13: ffff8000117ffb18 x12: fffffffffffc593f
> > x11: ffff800011676ad0 x10: fffffffffffe0000 x9 : ffff800011eab540
> > x8 : 206b73616d282077 x7 : 000000000000000f x6 : 000000000000000c
> > x5 : 0000000000000000 x4 : 0000000000000000 x3 : 00000000ffffffff
> > x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff00000283b800
> >
> > Call trace:
> > dma_direct_map_sg+0x26c/0x28c
> > dma_map_sg_attrs+0x2c/0x60
> > sunxi_mmc_request+0x70/0x420
> > __mmc_start_request+0x68/0x134
> > mmc_start_request+0x84/0xac
> > mmc_wait_for_req+0x70/0x100
> > mmc_io_rw_extended+0x1cc/0x2c0
> > sdio_io_rw_ext_helper+0x194/0x240
> > sdio_memcpy_fromio+0x20/0x2c
> > cw1200_sdio_memcpy_fromio+0x20/0x2c
> > __cw1200_reg_read+0x34/0x60
> > cw1200_reg_read+0x48/0x70
> > cw1200_load_firmware+0x38/0x5d0
> > cw1200_core_probe+0x794/0x970
> > cw1200_sdio_probe+0x124/0x22c
> > sdio_bus_probe+0xe8/0x1d0
> > really_probe+0xe4/0x504
> > driver_probe_device+0x64/0xcc
> > __device_attach_driver+0xd0/0x14c
> > bus_for_each_drv+0x78/0xd0
> > __device_attach+0xdc/0x184
> > device_initial_probe+0x14/0x20
> > bus_probe_device+0x9c/0xa4
> > device_add+0x350/0x83c
> > sdio_add_func+0x6c/0x90
> > mmc_attach_sdio+0x1b0/0x430
> > mmc_rescan+0x254/0x2e0
> > process_one_work+0x1d0/0x34c
> > worker_thread+0x13c/0x470
> > kthread+0x154/0x160
> > ret_from_fork+0x10/0x34
> >
> > sunxi-mmc 4021000.mmc: dma_map_sg failed
> > sunxi-mmc 4021000.mmc: map DMA failed
> > Can't read config register.
> >
> > Fix that by using kmalloc() allocated memory for read/write 16/32
> > funtions.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Thanks! But I found few more places which need this kind of fix:
https://github.com/jernejsk/linux-1/commit/
1cba9a7764c7d5bbdeb4ddeaa91ff20a0339f6ff
I guess I can keep R-b tag?
Best regards,
Jernej
>
> Kind regards
> Uffe
>
> > ---
> >
> > drivers/net/wireless/st/cw1200/hwio.c | 52 +++++++++++++++++++++------
> > drivers/net/wireless/st/cw1200/hwio.h | 51 ++++++++++++++++++++------
> > 2 files changed, 83 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/net/wireless/st/cw1200/hwio.c
> > b/drivers/net/wireless/st/cw1200/hwio.c index 3ba462de8e91..5521cb7f2233
> > 100644
> > --- a/drivers/net/wireless/st/cw1200/hwio.c
> > +++ b/drivers/net/wireless/st/cw1200/hwio.c
> > @@ -66,33 +66,65 @@ static int __cw1200_reg_write(struct cw1200_common
> > *priv, u16 addr,>
> > static inline int __cw1200_reg_read_32(struct cw1200_common *priv,
> >
> > u16 addr, u32 *val)
> >
> > {
> >
> > - __le32 tmp;
> > - int i = __cw1200_reg_read(priv, addr, &tmp, sizeof(tmp), 0);
> > - *val = le32_to_cpu(tmp);
> > + __le32 *tmp;
> > + int i;
> > +
> > + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> > + if (!tmp)
> > + return -ENOMEM;
> > +
> > + i = __cw1200_reg_read(priv, addr, tmp, sizeof(*tmp), 0);
> > + *val = le32_to_cpu(*tmp);
> > + kfree(tmp);
> >
> > return i;
> >
> > }
> >
> > static inline int __cw1200_reg_write_32(struct cw1200_common *priv,
> >
> > u16 addr, u32 val)
> >
> > {
> >
> > - __le32 tmp = cpu_to_le32(val);
> > - return __cw1200_reg_write(priv, addr, &tmp, sizeof(tmp), 0);
> > + __le32 *tmp;
> > + int i;
> > +
> > + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> > + if (!tmp)
> > + return -ENOMEM;
> > +
> > + *tmp = cpu_to_le32(val);
> > + i = __cw1200_reg_write(priv, addr, tmp, sizeof(*tmp), 0);
> > + kfree(tmp);
> > + return i;
> >
> > }
> >
> > static inline int __cw1200_reg_read_16(struct cw1200_common *priv,
> >
> > u16 addr, u16 *val)
> >
> > {
> >
> > - __le16 tmp;
> > - int i = __cw1200_reg_read(priv, addr, &tmp, sizeof(tmp), 0);
> > - *val = le16_to_cpu(tmp);
> > + __le16 *tmp;
> > + int i;
> > +
> > + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> > + if (!tmp)
> > + return -ENOMEM;
> > +
> > + i = __cw1200_reg_read(priv, addr, tmp, sizeof(*tmp), 0);
> > + *val = le16_to_cpu(*tmp);
> > + kfree(tmp);
> >
> > return i;
> >
> > }
> >
> > static inline int __cw1200_reg_write_16(struct cw1200_common *priv,
> >
> > u16 addr, u16 val)
> >
> > {
> >
> > - __le16 tmp = cpu_to_le16(val);
> > - return __cw1200_reg_write(priv, addr, &tmp, sizeof(tmp), 0);
> > + __le16 *tmp;
> > + int i;
> > +
> > + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> > + if (!tmp)
> > + return -ENOMEM;
> > +
> > + *tmp = cpu_to_le16(val);
> > + i = __cw1200_reg_write(priv, addr, tmp, sizeof(*tmp), 0);
> > + kfree(tmp);
> > + return i;
> >
> > }
> >
> > int cw1200_reg_read(struct cw1200_common *priv, u16 addr, void *buf,
> >
> > diff --git a/drivers/net/wireless/st/cw1200/hwio.h
> > b/drivers/net/wireless/st/cw1200/hwio.h index d1e629a566c2..088d2a1bacc0
> > 100644
> > --- a/drivers/net/wireless/st/cw1200/hwio.h
> > +++ b/drivers/net/wireless/st/cw1200/hwio.h
> > @@ -166,34 +166,65 @@ int cw1200_reg_write(struct cw1200_common *priv, u16
> > addr,>
> > static inline int cw1200_reg_read_16(struct cw1200_common *priv,
> >
> > u16 addr, u16 *val)
> >
> > {
> >
> > - __le32 tmp;
> > + __le32 *tmp;
> >
> > int i;
> >
> > - i = cw1200_reg_read(priv, addr, &tmp, sizeof(tmp));
> > - *val = le32_to_cpu(tmp) & 0xfffff;
> > +
> > + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> > + if (!tmp)
> > + return -ENOMEM;
> > +
> > + i = cw1200_reg_read(priv, addr, tmp, sizeof(*tmp));
> > + *val = le32_to_cpu(*tmp) & 0xfffff;
> > + kfree(tmp);
> >
> > return i;
> >
> > }
> >
> > static inline int cw1200_reg_write_16(struct cw1200_common *priv,
> >
> > u16 addr, u16 val)
> >
> > {
> >
> > - __le32 tmp = cpu_to_le32((u32)val);
> > - return cw1200_reg_write(priv, addr, &tmp, sizeof(tmp));
> > + __le32 *tmp;
> > + int i;
> > +
> > + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> > + if (!tmp)
> > + return -ENOMEM;
> > +
> > + *tmp = cpu_to_le32((u32)val);
> > + i = cw1200_reg_write(priv, addr, tmp, sizeof(*tmp));
> > + kfree(tmp);
> > + return i;
> >
> > }
> >
> > static inline int cw1200_reg_read_32(struct cw1200_common *priv,
> >
> > u16 addr, u32 *val)
> >
> > {
> >
> > - __le32 tmp;
> > - int i = cw1200_reg_read(priv, addr, &tmp, sizeof(tmp));
> > - *val = le32_to_cpu(tmp);
> > + __le32 *tmp;
> > + int i;
> > +
> > + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> > + if (!tmp)
> > + return -ENOMEM;
> > +
> > + i = cw1200_reg_read(priv, addr, tmp, sizeof(*tmp));
> > + *val = le32_to_cpu(*tmp);
> > + kfree(tmp);
> >
> > return i;
> >
> > }
> >
> > static inline int cw1200_reg_write_32(struct cw1200_common *priv,
> >
> > u16 addr, u32 val)
> >
> > {
> >
> > - __le32 tmp = cpu_to_le32(val);
> > - return cw1200_reg_write(priv, addr, &tmp, sizeof(val));
> > + __le32 *tmp;
> > + int i;
> > +
> > + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> > + if (!tmp)
> > + return -ENOMEM;
> > +
> > + *tmp = cpu_to_le32(val);
> > + i = cw1200_reg_write(priv, addr, tmp, sizeof(val));
> > + kfree(tmp);
> > + return i;
> >
> > }
> >
> > int cw1200_indirect_read(struct cw1200_common *priv, u32 addr, void *buf,
> >
> > --
> > 2.32.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] cw1200: use kmalloc() allocation instead of stack
2021-06-30 9:55 ` Ulf Hansson
@ 2021-06-30 11:30 ` Arnd Bergmann
2021-06-30 12:03 ` Ulf Hansson
0 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2021-06-30 11:30 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jernej Skrabec, pizza, Kalle Valo, David Miller, Jakub Kicinski,
linux-wireless, Networking, Linux Kernel Mailing List
On Wed, Jun 30, 2021 at 11:56 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 22 Jun 2021 at 22:33, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Tue, Jun 22, 2021 at 10:24 PM Jernej Skrabec
> > <jernej.skrabec@gmail.com> wrote:
> > >
> > > It turns out that if CONFIG_VMAP_STACK is enabled and src or dst is
> > > memory allocated on stack, SDIO operations fail due to invalid memory
> > > address conversion:
> >
> > Thank you for sending this!
> >
> > It's worth pointing out that even without CONFIG_VMAP_STACK, using
> > dma_map_sg() on a stack variable is broken, though it will appear to
> > work most of the time but rarely cause a stack data corruption when
> > the cache management goes wrong.
> >
> > This clearly needs to be fixed somewhere, if not with your patch, then
> > a similar one.
> >
> > > diff --git a/drivers/net/wireless/st/cw1200/hwio.c b/drivers/net/wireless/st/cw1200/hwio.c
> > > index 3ba462de8e91..5521cb7f2233 100644
> > > --- a/drivers/net/wireless/st/cw1200/hwio.c
> > > +++ b/drivers/net/wireless/st/cw1200/hwio.c
> > > @@ -66,33 +66,65 @@ static int __cw1200_reg_write(struct cw1200_common *priv, u16 addr,
> > > static inline int __cw1200_reg_read_32(struct cw1200_common *priv,
> > > u16 addr, u32 *val)
> > > {
> > > - __le32 tmp;
> > > - int i = __cw1200_reg_read(priv, addr, &tmp, sizeof(tmp), 0);
> > > - *val = le32_to_cpu(tmp);
> > > + __le32 *tmp;
> > > + int i;
> > > +
> > > + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> > > + if (!tmp)
> > > + return -ENOMEM;
> > > +
> > > + i = __cw1200_reg_read(priv, addr, tmp, sizeof(*tmp), 0);
> > > + *val = le32_to_cpu(*tmp);
> > > + kfree(tmp);
> > > return i;
> > > }
> >
> > There is a possible problem here when the function gets called from
> > atomic context, so it might need to use GFP_ATOMIC instead of
> > GFP_KERNEL. If it's never called from atomic context, then this patch
> > looks correct to me.
>
> I would be surprised if this is called from atomic context (when IRQs
> are turned off), because in most cases, to complete the read/write
> request the mmc controller driver relies on IRQs being delivered.
I thought I had seen a spinlock in the forked driver, but I don't see
it now, so I probably misremembered that bit.
> > The alternative would be to add a bounce buffer check based on
> > is_vmalloc_or_module_addr() in sdio_io_rw_ext_helper(), which would
> > add a small bit of complexity there but solve the problem for
> > all drivers at once. In this case, it would probably have to use
> > GFP_ATOMIC regardless of whether __cw1200_reg_read_32()
> > is allowed to sleep, since other callers might not.
>
> I like the idea, but...
>
> I don't think we should see this as an alternative, but rather as a
> complement which would have performance issues. A warning should be
> printed, if the buffer isn't properly allocated.
Fair enough. I found the function call I was looking for: object_is_on_stack(),
the patch below should print a warning once when a driver passes
a bad buffer, but I did not test that.
There are some possible variations on that: an on-stack buffer by
itself can work as long as the DMA is cache-coherent and stacks
are not vmapped. For the is_vmalloc_or_module_addr() case,
we may decide to just return an error, rather than running into
a kernel oops.
> Additionally, I don't think GFT_ATOMIC should be needed.
Ok, I now see the mmc_wait_for_req() in mmc_io_rw_extended()
that probably means it can not be called in atomic context at all,
and that GFP_KERNEL is safe, and that any driver calling it with
a spinlock held is already broken.
Arnd
8<---
diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
index 4c229dd2b6e5..845f9ca3b200 100644
--- a/drivers/mmc/core/sdio_ops.c
+++ b/drivers/mmc/core/sdio_ops.c
@@ -124,6 +124,7 @@ int mmc_io_rw_extended(struct mmc_card *card, int
write, unsigned fn,
int err;
WARN_ON(blksz == 0);
+ WARN_ON_ONCE(is_vmalloc_or_module_addr(buf) || object_is_on_stack(buf));
/* sanity check */
if (addr & ~0x1FFFF)
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] cw1200: use kmalloc() allocation instead of stack
2021-06-30 10:09 ` Jernej Škrabec
@ 2021-06-30 12:00 ` Ulf Hansson
0 siblings, 0 replies; 10+ messages in thread
From: Ulf Hansson @ 2021-06-30 12:00 UTC (permalink / raw)
To: Jernej Škrabec
Cc: pizza, Arnd Bergmann, Kalle Valo, David S. Miller,
Jakub Kicinski, linux-wireless, netdev,
Linux Kernel Mailing List
On Wed, 30 Jun 2021 at 12:09, Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
>
> Hi Ulf!
>
> Dne sreda, 30. junij 2021 ob 12:03:13 CEST je Ulf Hansson napisal(a):
> > On Tue, 22 Jun 2021 at 22:23, Jernej Skrabec <jernej.skrabec@gmail.com>
> wrote:
> > > It turns out that if CONFIG_VMAP_STACK is enabled and src or dst is
> > > memory allocated on stack, SDIO operations fail due to invalid memory
> > > address conversion:
> > >
> > > cw1200_wlan_sdio: Probe called
> > > sunxi-mmc 4021000.mmc: DMA addr 0x0000800051eab954+4 overflow (mask
> > > ffffffff, bus limit 0). WARNING: CPU: 2 PID: 152 at
> > > kernel/dma/direct.h:97 dma_direct_map_sg+0x26c/0x28c CPU: 2 PID: 152
> > > Comm: kworker/2:2 Not tainted 5.13.0-rc1-00026-g84114ef026b9-dirty #85
> > > Hardware name: X96 Mate (DT)
> > > Workqueue: events_freezable mmc_rescan
> > > pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
> > > pc : dma_direct_map_sg+0x26c/0x28c
> > > lr : dma_direct_map_sg+0x26c/0x28c
> > > sp : ffff800011eab540
> > > x29: ffff800011eab540 x28: ffff800011eab738 x27: 0000000000000000
> > > x26: ffff000001daf010 x25: 0000000000000000 x24: 0000000000000000
> > > x23: 0000000000000002 x22: fffffc0000000000 x21: ffff8000113b0ab0
> > > x20: ffff80001181abb0 x19: 0000000000000001 x18: ffffffffffffffff
> > > x17: 00000000fa97f83f x16: 00000000d2e01bf8 x15: ffff8000117ffb1d
> > > x14: ffffffffffffffff x13: ffff8000117ffb18 x12: fffffffffffc593f
> > > x11: ffff800011676ad0 x10: fffffffffffe0000 x9 : ffff800011eab540
> > > x8 : 206b73616d282077 x7 : 000000000000000f x6 : 000000000000000c
> > > x5 : 0000000000000000 x4 : 0000000000000000 x3 : 00000000ffffffff
> > > x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff00000283b800
> > >
> > > Call trace:
> > > dma_direct_map_sg+0x26c/0x28c
> > > dma_map_sg_attrs+0x2c/0x60
> > > sunxi_mmc_request+0x70/0x420
> > > __mmc_start_request+0x68/0x134
> > > mmc_start_request+0x84/0xac
> > > mmc_wait_for_req+0x70/0x100
> > > mmc_io_rw_extended+0x1cc/0x2c0
> > > sdio_io_rw_ext_helper+0x194/0x240
> > > sdio_memcpy_fromio+0x20/0x2c
> > > cw1200_sdio_memcpy_fromio+0x20/0x2c
> > > __cw1200_reg_read+0x34/0x60
> > > cw1200_reg_read+0x48/0x70
> > > cw1200_load_firmware+0x38/0x5d0
> > > cw1200_core_probe+0x794/0x970
> > > cw1200_sdio_probe+0x124/0x22c
> > > sdio_bus_probe+0xe8/0x1d0
> > > really_probe+0xe4/0x504
> > > driver_probe_device+0x64/0xcc
> > > __device_attach_driver+0xd0/0x14c
> > > bus_for_each_drv+0x78/0xd0
> > > __device_attach+0xdc/0x184
> > > device_initial_probe+0x14/0x20
> > > bus_probe_device+0x9c/0xa4
> > > device_add+0x350/0x83c
> > > sdio_add_func+0x6c/0x90
> > > mmc_attach_sdio+0x1b0/0x430
> > > mmc_rescan+0x254/0x2e0
> > > process_one_work+0x1d0/0x34c
> > > worker_thread+0x13c/0x470
> > > kthread+0x154/0x160
> > > ret_from_fork+0x10/0x34
> > >
> > > sunxi-mmc 4021000.mmc: dma_map_sg failed
> > > sunxi-mmc 4021000.mmc: map DMA failed
> > > Can't read config register.
> > >
> > > Fix that by using kmalloc() allocated memory for read/write 16/32
> > > funtions.
> > >
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> >
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Thanks! But I found few more places which need this kind of fix:
> https://github.com/jernejsk/linux-1/commit/
> 1cba9a7764c7d5bbdeb4ddeaa91ff20a0339f6ff
I couldn't find it.
>
> I guess I can keep R-b tag?
Well, just send a new version and I will respond to it again, no
worries. Or send an additional one on top.
[...]
Kind regards
Uffe
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] cw1200: use kmalloc() allocation instead of stack
2021-06-30 11:30 ` Arnd Bergmann
@ 2021-06-30 12:03 ` Ulf Hansson
2021-06-30 12:21 ` Arnd Bergmann
0 siblings, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2021-06-30 12:03 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jernej Skrabec, pizza, Kalle Valo, David Miller, Jakub Kicinski,
linux-wireless, Networking, Linux Kernel Mailing List
On Wed, 30 Jun 2021 at 13:30, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Jun 30, 2021 at 11:56 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Tue, 22 Jun 2021 at 22:33, Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > On Tue, Jun 22, 2021 at 10:24 PM Jernej Skrabec
> > > <jernej.skrabec@gmail.com> wrote:
> > > >
> > > > It turns out that if CONFIG_VMAP_STACK is enabled and src or dst is
> > > > memory allocated on stack, SDIO operations fail due to invalid memory
> > > > address conversion:
> > >
> > > Thank you for sending this!
> > >
> > > It's worth pointing out that even without CONFIG_VMAP_STACK, using
> > > dma_map_sg() on a stack variable is broken, though it will appear to
> > > work most of the time but rarely cause a stack data corruption when
> > > the cache management goes wrong.
> > >
> > > This clearly needs to be fixed somewhere, if not with your patch, then
> > > a similar one.
> > >
> > > > diff --git a/drivers/net/wireless/st/cw1200/hwio.c b/drivers/net/wireless/st/cw1200/hwio.c
> > > > index 3ba462de8e91..5521cb7f2233 100644
> > > > --- a/drivers/net/wireless/st/cw1200/hwio.c
> > > > +++ b/drivers/net/wireless/st/cw1200/hwio.c
> > > > @@ -66,33 +66,65 @@ static int __cw1200_reg_write(struct cw1200_common *priv, u16 addr,
> > > > static inline int __cw1200_reg_read_32(struct cw1200_common *priv,
> > > > u16 addr, u32 *val)
> > > > {
> > > > - __le32 tmp;
> > > > - int i = __cw1200_reg_read(priv, addr, &tmp, sizeof(tmp), 0);
> > > > - *val = le32_to_cpu(tmp);
> > > > + __le32 *tmp;
> > > > + int i;
> > > > +
> > > > + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> > > > + if (!tmp)
> > > > + return -ENOMEM;
> > > > +
> > > > + i = __cw1200_reg_read(priv, addr, tmp, sizeof(*tmp), 0);
> > > > + *val = le32_to_cpu(*tmp);
> > > > + kfree(tmp);
> > > > return i;
> > > > }
> > >
> > > There is a possible problem here when the function gets called from
> > > atomic context, so it might need to use GFP_ATOMIC instead of
> > > GFP_KERNEL. If it's never called from atomic context, then this patch
> > > looks correct to me.
> >
> > I would be surprised if this is called from atomic context (when IRQs
> > are turned off), because in most cases, to complete the read/write
> > request the mmc controller driver relies on IRQs being delivered.
>
> I thought I had seen a spinlock in the forked driver, but I don't see
> it now, so I probably misremembered that bit.
>
> > > The alternative would be to add a bounce buffer check based on
> > > is_vmalloc_or_module_addr() in sdio_io_rw_ext_helper(), which would
> > > add a small bit of complexity there but solve the problem for
> > > all drivers at once. In this case, it would probably have to use
> > > GFP_ATOMIC regardless of whether __cw1200_reg_read_32()
> > > is allowed to sleep, since other callers might not.
> >
> > I like the idea, but...
> >
> > I don't think we should see this as an alternative, but rather as a
> > complement which would have performance issues. A warning should be
> > printed, if the buffer isn't properly allocated.
>
> Fair enough. I found the function call I was looking for: object_is_on_stack(),
> the patch below should print a warning once when a driver passes
> a bad buffer, but I did not test that.
>
> There are some possible variations on that: an on-stack buffer by
> itself can work as long as the DMA is cache-coherent and stacks
> are not vmapped. For the is_vmalloc_or_module_addr() case,
> we may decide to just return an error, rather than running into
> a kernel oops.
>
> > Additionally, I don't think GFT_ATOMIC should be needed.
>
> Ok, I now see the mmc_wait_for_req() in mmc_io_rw_extended()
> that probably means it can not be called in atomic context at all,
> and that GFP_KERNEL is safe, and that any driver calling it with
> a spinlock held is already broken.
>
> Arnd
>
> 8<---
> diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
> index 4c229dd2b6e5..845f9ca3b200 100644
> --- a/drivers/mmc/core/sdio_ops.c
> +++ b/drivers/mmc/core/sdio_ops.c
> @@ -124,6 +124,7 @@ int mmc_io_rw_extended(struct mmc_card *card, int
> write, unsigned fn,
> int err;
>
> WARN_ON(blksz == 0);
> + WARN_ON_ONCE(is_vmalloc_or_module_addr(buf) || object_is_on_stack(buf));
Looks reasonable to me, at least we should start giving a warning.
Would you like to send a formal patch that we can test?
Kind regards
Uffe
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] cw1200: use kmalloc() allocation instead of stack
2021-06-30 12:03 ` Ulf Hansson
@ 2021-06-30 12:21 ` Arnd Bergmann
0 siblings, 0 replies; 10+ messages in thread
From: Arnd Bergmann @ 2021-06-30 12:21 UTC (permalink / raw)
To: Ulf Hansson
Cc: Jernej Skrabec, pizza, Kalle Valo, David Miller, Jakub Kicinski,
linux-wireless, Networking, Linux Kernel Mailing List
On Wed, Jun 30, 2021 at 2:03 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
> > index 4c229dd2b6e5..845f9ca3b200 100644
> > --- a/drivers/mmc/core/sdio_ops.c
> > +++ b/drivers/mmc/core/sdio_ops.c
> > @@ -124,6 +124,7 @@ int mmc_io_rw_extended(struct mmc_card *card, int
> > write, unsigned fn,
> > int err;
> >
> > WARN_ON(blksz == 0);
> > + WARN_ON_ONCE(is_vmalloc_or_module_addr(buf) || object_is_on_stack(buf));
>
> Looks reasonable to me, at least we should start giving a warning.
> Would you like to send a formal patch that we can test?
Done.
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [RFC PATCH] cw1200: use kmalloc() allocation instead of stack
2021-06-30 10:03 ` Ulf Hansson
2021-06-30 10:09 ` Jernej Škrabec
@ 2021-06-30 16:08 ` David Laight
1 sibling, 0 replies; 10+ messages in thread
From: David Laight @ 2021-06-30 16:08 UTC (permalink / raw)
To: 'Ulf Hansson', Jernej Skrabec
Cc: pizza, Arnd Bergmann, Kalle Valo, David S. Miller,
Jakub Kicinski, linux-wireless, netdev,
Linux Kernel Mailing List
From: Ulf Hansson
> Sent: 30 June 2021 11:03
...
> > It turns out that if CONFIG_VMAP_STACK is enabled and src or dst is
> > memory allocated on stack, SDIO operations fail due to invalid memory
> > address conversion:
...
> > Fix that by using kmalloc() allocated memory for read/write 16/32
> > funtions.
Could a field be added to 'struct cw1200_common'
that the functions could use as a bounce buffer?
ISTM that is DMA are being done there must be some
serialisation in there somewhere that will stop
concurrent accesses.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-06-30 16:08 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-22 20:23 [RFC PATCH] cw1200: use kmalloc() allocation instead of stack Jernej Skrabec
2021-06-22 20:30 ` Arnd Bergmann
2021-06-30 9:55 ` Ulf Hansson
2021-06-30 11:30 ` Arnd Bergmann
2021-06-30 12:03 ` Ulf Hansson
2021-06-30 12:21 ` Arnd Bergmann
2021-06-30 10:03 ` Ulf Hansson
2021-06-30 10:09 ` Jernej Škrabec
2021-06-30 12:00 ` Ulf Hansson
2021-06-30 16:08 ` David Laight
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).