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

["Song Bao Hua (Barry Song)" <song.bao.hua@hisilicon.com>]

> -----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

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.

> 
>    242                  if (ret) {
>    243                          pr_err("failed to set dma_mask on device %s\n",
>    244                                  dev_name(map->dev));
>    245                          return -EINVAL;
>    246                  }
>    247
>    248                  ret = do_map_benchmark(map);
>    249
>    250                  /*
>    251                   * restore the original dma_mask as many devices' dma_mask
> are
>    252                   * set by architectures, acpi, busses. When we bind them
> back
>    253                   * to their original drivers, those drivers shouldn't see
>    254                   * dma_mask changed by benchmark
>    255                   */
>    256                  dma_set_mask(map->dev, old_dma_mask);
>    257                  break;
>    258          default:
>    259                  return -EINVAL;
>    260          }
>    261
>    262          if (copy_to_user(argp, &map->bparam, sizeof(map->bparam)))
>    263                  return -EFAULT;
>    264
>    265          return ret;
>    266  }
> 
> regards,
> dan carpenter

Thanks
Barry

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