| Message ID | 20210630122057.2795882-1-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: warn for invalid SDIO data buffers | expand |
Hi Arnd,
I love your patch! Yet something to improve:
[auto build test ERROR on soc/for-next]
[also build test ERROR on linus/master v5.13 next-20210630]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Arnd-Bergmann/mmc-warn-for-invalid-SDIO-data-buffers/20210630-202237
base: https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
config: i386-randconfig-c001-20210630 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/256b826ee105fe46723b99c3f128ea01aa3e7adf
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Arnd-Bergmann/mmc-warn-for-invalid-SDIO-data-buffers/20210630-202237
git checkout 256b826ee105fe46723b99c3f128ea01aa3e7adf
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "is_vmalloc_or_module_addr" [drivers/mmc/core/mmc_core.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Thu, Jul 1, 2021 at 1:02 AM kernel test robot <lkp@intel.com> wrote: > > Hi Arnd, > > I love your patch! Yet something to improve: > > [auto build test ERROR on soc/for-next] > [also build test ERROR on linus/master v5.13 next-20210630] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Arnd-Bergmann/mmc-warn-for-invalid-SDIO-data-buffers/20210630-202237 > base: https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next > config: i386-randconfig-c001-20210630 (attached as .config) > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 > reproduce (this is a W=1 build): > # https://github.com/0day-ci/linux/commit/256b826ee105fe46723b99c3f128ea01aa3e7adf > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Arnd-Bergmann/mmc-warn-for-invalid-SDIO-data-buffers/20210630-202237 > git checkout 256b826ee105fe46723b99c3f128ea01aa3e7adf > # save the attached .config to linux build tree > make W=1 ARCH=i386 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All errors (new ones prefixed by >>, old ones prefixed by <<): > > >> ERROR: modpost: "is_vmalloc_or_module_addr" [drivers/mmc/core/mmc_core.ko] undefined! Ah, I see that is_vmalloc_addr() is exported, but is_vmalloc_or_module_addr() is not. I assume it's ok to add a corresponding EXPORT_SYMBOL_GPL() for it as well. I'll wait for other comments before resending with that change. Arnd
Hi Arnd On 2021/6/30 20:20, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Jernej Skrabec reported a problem with the cw1200 driver failing on > arm64 systems with CONFIG_VMAP_STACK=y. > > The driver in this case passes a pointer to a stack variable (in vmalloc > space) into the sdio layer, which gets translated into an invalid DMA > address. > > Even without CONFIG_VMAP_STACK, the driver is still unreliable, as > cache invalidations on the DMA buffer may cause random data corruption > in adjacent stack slots. > > This could be worked around in the SDIO core, but in the discussion we > decided that passing a stack variable into SDIO should always be considered > a bug, as it is for USB drivers. > > Change the sdio core to produce a one-time warning for any on-stack > (both with and without CONFIG_VMAP_STACK) as well as any vmalloc > or module-local address that would have the same translation problem. This was the previous comment about the same topic. Should we check for mmc_io_rw_direct? https://www.spinics.net/lists/linux-mmc/msg41794.html > > Cc: Jernej Skrabec <jernej.skrabec@gmail.com> > Link: https://lore.kernel.org/lkml/20210622202345.795578-1-jernej.skrabec@gmail.com/ > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/mmc/core/sdio_ops.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c > index 4c229dd2b6e5..14e983faf223 100644 > --- a/drivers/mmc/core/sdio_ops.c > +++ b/drivers/mmc/core/sdio_ops.c > @@ -6,6 +6,7 @@ > */ > > #include <linux/scatterlist.h> > +#include <linux/sched/task_stack.h> > > #include <linux/mmc/host.h> > #include <linux/mmc/card.h> > @@ -124,6 +125,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 Fri, Jul 2, 2021 at 3:02 AM Shawn Lin <shawn.lin@rock-chips.com> wrote: > On 2021/6/30 20:20, Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > Jernej Skrabec reported a problem with the cw1200 driver failing on > > arm64 systems with CONFIG_VMAP_STACK=y. > > > > The driver in this case passes a pointer to a stack variable (in vmalloc > > space) into the sdio layer, which gets translated into an invalid DMA > > address. > > > > Even without CONFIG_VMAP_STACK, the driver is still unreliable, as > > cache invalidations on the DMA buffer may cause random data corruption > > in adjacent stack slots. > > > > This could be worked around in the SDIO core, but in the discussion we > > decided that passing a stack variable into SDIO should always be considered > > a bug, as it is for USB drivers. > > > > Change the sdio core to produce a one-time warning for any on-stack > > (both with and without CONFIG_VMAP_STACK) as well as any vmalloc > > or module-local address that would have the same translation problem. > > This was the previous comment about the same topic. > Should we check for mmc_io_rw_direct? > > https://www.spinics.net/lists/linux-mmc/msg41794.html Hi Shawn, thank you for remembering that previous discussion, that is a good question. Looking at the code though, I don't actually see any part of mmc_io_rw_direct() doing DMA on a caller-provided buffer. The only thing I see in the code is a 'u8 *out' argument, but that is just a pointer to a single byte that is set by this function. Do you see any other issue with that function, or does that mean we don't have to change it? Arnd
On 2021/7/2 15:03, Arnd Bergmann wrote: > On Fri, Jul 2, 2021 at 3:02 AM Shawn Lin <shawn.lin@rock-chips.com> wrote: >> On 2021/6/30 20:20, Arnd Bergmann wrote: >>> From: Arnd Bergmann <arnd@arndb.de> >>> >>> Jernej Skrabec reported a problem with the cw1200 driver failing on >>> arm64 systems with CONFIG_VMAP_STACK=y. >>> >>> The driver in this case passes a pointer to a stack variable (in vmalloc >>> space) into the sdio layer, which gets translated into an invalid DMA >>> address. >>> >>> Even without CONFIG_VMAP_STACK, the driver is still unreliable, as >>> cache invalidations on the DMA buffer may cause random data corruption >>> in adjacent stack slots. >>> >>> This could be worked around in the SDIO core, but in the discussion we >>> decided that passing a stack variable into SDIO should always be considered >>> a bug, as it is for USB drivers. >>> >>> Change the sdio core to produce a one-time warning for any on-stack >>> (both with and without CONFIG_VMAP_STACK) as well as any vmalloc >>> or module-local address that would have the same translation problem. >> >> This was the previous comment about the same topic. >> Should we check for mmc_io_rw_direct? >> >> https://www.spinics.net/lists/linux-mmc/msg41794.html > > Hi Shawn, > > thank you for remembering that previous discussion, that is a > good question. Looking at the code though, I don't actually > see any part of mmc_io_rw_direct() doing DMA on a caller-provided > buffer. The only thing I see in the code is a 'u8 *out' argument, but > that is just a pointer to a single byte that is set by this function. > I didn't quite get the point we need check out for mmc_io_rw_direct() either. As I thought what Ulf was suggesting was some controllers might use data fifo to get cmd response and get DMA involved? But I don't see a explicit one. > Do you see any other issue with that function, or does that mean > we don't have to change it? > > Arnd > > >
On Fri, 2 Jul 2021 at 09:03, Arnd Bergmann <arnd@kernel.org> wrote: > > On Fri, Jul 2, 2021 at 3:02 AM Shawn Lin <shawn.lin@rock-chips.com> wrote: > > On 2021/6/30 20:20, Arnd Bergmann wrote: > > > From: Arnd Bergmann <arnd@arndb.de> > > > > > > Jernej Skrabec reported a problem with the cw1200 driver failing on > > > arm64 systems with CONFIG_VMAP_STACK=y. > > > > > > The driver in this case passes a pointer to a stack variable (in vmalloc > > > space) into the sdio layer, which gets translated into an invalid DMA > > > address. > > > > > > Even without CONFIG_VMAP_STACK, the driver is still unreliable, as > > > cache invalidations on the DMA buffer may cause random data corruption > > > in adjacent stack slots. > > > > > > This could be worked around in the SDIO core, but in the discussion we > > > decided that passing a stack variable into SDIO should always be considered > > > a bug, as it is for USB drivers. > > > > > > Change the sdio core to produce a one-time warning for any on-stack > > > (both with and without CONFIG_VMAP_STACK) as well as any vmalloc > > > or module-local address that would have the same translation problem. > > > > This was the previous comment about the same topic. > > Should we check for mmc_io_rw_direct? > > > > https://www.spinics.net/lists/linux-mmc/msg41794.html > > Hi Shawn, > > thank you for remembering that previous discussion, that is a > good question. Looking at the code though, I don't actually > see any part of mmc_io_rw_direct() doing DMA on a caller-provided > buffer. The only thing I see in the code is a 'u8 *out' argument, but > that is just a pointer to a single byte that is set by this function. > > Do you see any other issue with that function, or does that mean > we don't have to change it? I was wrong when I earlier said that we needed to care about mmc_io_rw_direct(). mmc_io_rw_direct() transfer "data" over the CMD line. MMC host drivers can't do DMA on that. I think the $subject patch looks reasonable to me. Kind regards Uffe
diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c index 4c229dd2b6e5..14e983faf223 100644 --- a/drivers/mmc/core/sdio_ops.c +++ b/drivers/mmc/core/sdio_ops.c @@ -6,6 +6,7 @@ */ #include <linux/scatterlist.h> +#include <linux/sched/task_stack.h> #include <linux/mmc/host.h> #include <linux/mmc/card.h> @@ -124,6 +125,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)