* [bug report] dma-mapping: add benchmark support for streaming DMA APIs
@ 2020-12-09 7:00 Dan Carpenter
2020-12-09 10:01 ` Song Bao Hua (Barry Song)
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2020-12-09 7:00 UTC (permalink / raw)
To: song.bao.hua; +Cc: iommu
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).
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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [bug report] dma-mapping: add benchmark support for streaming DMA APIs
2020-12-09 7:00 [bug report] dma-mapping: add benchmark support for streaming DMA APIs Dan Carpenter
@ 2020-12-09 10:01 ` Song Bao Hua (Barry Song)
2020-12-09 10:06 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Song Bao Hua (Barry Song) @ 2020-12-09 10:01 UTC (permalink / raw)
To: Dan Carpenter; +Cc: iommu
> -----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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] dma-mapping: add benchmark support for streaming DMA APIs
2020-12-09 10:01 ` Song Bao Hua (Barry Song)
@ 2020-12-09 10:06 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2020-12-09 10:06 UTC (permalink / raw)
To: Song Bao Hua (Barry Song); +Cc: iommu
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-12-09 10:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-09 7:00 [bug report] dma-mapping: add benchmark support for streaming DMA APIs Dan Carpenter
2020-12-09 10:01 ` Song Bao Hua (Barry Song)
2020-12-09 10:06 ` Dan Carpenter
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).