| Message ID | 20210622202345.795578-1-jernej.skrabec@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] cw1200: use kmalloc() allocation instead of stack | expand |
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
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 >
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
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)
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
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
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)
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,
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(-)