Re: [bug report] dma-mapping: add benchmark support for streaming DMA APIs

[Dan Carpenter <dan.carpenter@oracle.com>]

On Wed, Dec 09, 2020 at 10:01:49AM +0000, Song Bao Hua (Barry Song) wrote:
> 
> 
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Wednesday, December 9, 2020 8:00 PM
> > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> > Cc: iommu@lists.linux-foundation.org
> > Subject: [bug report] dma-mapping: add benchmark support for streaming DMA APIs
> > 
> > Hello Barry Song,
> > 
> > The patch 65789daa8087: "dma-mapping: add benchmark support for
> > streaming DMA APIs" from Nov 16, 2020, leads to the following static
> > checker warning:
> > 
> > 	kernel/dma/map_benchmark.c:241 map_benchmark_ioctl()
> > 	error: undefined (user controlled) shift '1 << (map->bparam.dma_bits)'
> > 
> > kernel/dma/map_benchmark.c
> >    191  static long map_benchmark_ioctl(struct file *file, unsigned int cmd,
> >    192                  unsigned long arg)
> >    193  {
> >    194          struct map_benchmark_data *map = file->private_data;
> >    195          void __user *argp = (void __user *)arg;
> >    196          u64 old_dma_mask;
> >    197
> >    198          int ret;
> >    199
> >    200          if (copy_from_user(&map->bparam, argp, sizeof(map->bparam)))
> >                                    ^^^^^^^^^^^^^
> > Comes from the user
> > 
> >    201                  return -EFAULT;
> >    202
> >    203          switch (cmd) {
> >    204          case DMA_MAP_BENCHMARK:
> >    205                  if (map->bparam.threads == 0 ||
> >    206                      map->bparam.threads > DMA_MAP_MAX_THREADS) {
> >    207                          pr_err("invalid thread number\n");
> >    208                          return -EINVAL;
> >    209                  }
> >    210
> >    211                  if (map->bparam.seconds == 0 ||
> >    212                      map->bparam.seconds > DMA_MAP_MAX_SECONDS) {
> >    213                          pr_err("invalid duration seconds\n");
> >    214                          return -EINVAL;
> >    215                  }
> >    216
> >    217                  if (map->bparam.node != NUMA_NO_NODE &&
> >    218                      !node_possible(map->bparam.node)) {
> >    219                          pr_err("invalid numa node\n");
> >    220                          return -EINVAL;
> >    221                  }
> >    222
> >    223                  switch (map->bparam.dma_dir) {
> >    224                  case DMA_MAP_BIDIRECTIONAL:
> >    225                          map->dir = DMA_BIDIRECTIONAL;
> >    226                          break;
> >    227                  case DMA_MAP_FROM_DEVICE:
> >    228                          map->dir = DMA_FROM_DEVICE;
> >    229                          break;
> >    230                  case DMA_MAP_TO_DEVICE:
> >    231                          map->dir = DMA_TO_DEVICE;
> >    232                          break;
> >    233                  default:
> >    234                          pr_err("invalid DMA direction\n");
> >    235                          return -EINVAL;
> >    236                  }
> >    237
> >    238                  old_dma_mask = dma_get_mask(map->dev);
> >    239
> >    240                  ret = dma_set_mask(map->dev,
> >    241                                     DMA_BIT_MASK(map->bparam.dma_bits));
> >                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > If this is more than 31 then the behavior is undefined (but in real life
> > it will shift wrap).
> 
> Guess it should be less than 64?
> For 64, it would be ~0ULL, otherwise, it will be 1ULL<<n-1

Yeah.  You're right > 64 is undefined, not 31 as I said.

> 
> In test app,
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=7679325702 
> 
> I have some code like:
> +	/* suppose the mininum DMA zone is 1MB in the world */
> +	if (bits < 20 || bits > 64) {
> +		fprintf(stderr, "invalid dma mask bit, must be in 20-64\n");
> +		exit(1);
> +	}
> 
> Maybe I should do the same thing in kernel as well.

Sounds good!

regards,
dan carpenter

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu