* [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
@ 2021-08-04 9:08 Dan Carpenter
0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2021-08-04 9:08 UTC (permalink / raw)
To: umalhi; +Cc: linux-rdma
[ This is an ancient bug, but the bug is clear to see, but unfortunately
it's probably less easy to fix. - dan ]
Hello Upinder Malhi,
The patch e3cf00d0a87f: "IB/usnic: Add Cisco VIC low-level hardware
driver" from Sep 10, 2013, leads to the following static checker
warning:
fs/kernfs/dir.c:476 kernfs_drain()
warn: sleeping in atomic context
drivers/infiniband/hw/usnic/usnic_ib_verbs.c
190 if (usnic_ib_share_vf) {
191 /* Try to find resouces on a used vf which is in pd */
192 dev_list = usnic_uiom_get_dev_list(pd->umem_pd);
193 if (IS_ERR(dev_list))
194 return ERR_CAST(dev_list);
195 for (i = 0; dev_list[i]; i++) {
196 dev = dev_list[i];
197 vf = dev_get_drvdata(dev);
198 spin_lock(&vf->lock);
^^^^^^^^^^^^^^^^^^^^^
We're holding a spin lock.
199 vnic = vf->vnic;
200 if (!usnic_vnic_check_room(vnic, res_spec)) {
201 usnic_dbg("Found used vnic %s from %s\n",
202 dev_name(&us_ibdev->ib_dev.dev),
203 pci_name(usnic_vnic_get_pdev(
204 vnic)));
205 qp_grp = usnic_ib_qp_grp_create(us_ibdev->ufdev,
^^^^^^^^^^^^^^^^^^^^^^
The create function calls usnic_ib_sysfs_qpn_add() which does a
kobject_init_and_add(). Unfortunately kobject allocations have many
sleeps inside them so it can't be done while holding a spinlock.
Same thing for the other usnic_ib_qp_grp_create() later on in the same
function.
206 vf, pd,
207 res_spec,
208 trans_spec);
209
210 spin_unlock(&vf->lock);
211 goto qp_grp_check;
212 }
213 spin_unlock(&vf->lock);
214
215 }
216 usnic_uiom_free_dev_list(dev_list);
217 dev_list = NULL;
218 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread* [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
@ 2021-07-05 10:23 Dan Carpenter
2021-07-05 13:47 ` Robin Murphy
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2021-07-05 10:23 UTC (permalink / raw)
To: umalhi; +Cc: iommu, linux-rdma
[ Ancient code, but the bug seems real enough still. -dan ]
Hello Upinder Malhi,
The patch e3cf00d0a87f: "IB/usnic: Add Cisco VIC low-level hardware
driver" from Sep 10, 2013, leads to the following static checker
warning:
drivers/iommu/iommu.c:2482 iommu_map()
warn: sleeping in atomic context
drivers/infiniband/hw/usnic/usnic_uiom.c
244 static int usnic_uiom_map_sorted_intervals(struct list_head *intervals,
245 struct usnic_uiom_reg *uiomr)
This function is always called from usnic_uiom_reg_get() which is holding
spin_lock(&pd->lock); so it can't sleep.
246 {
247 int i, err;
248 size_t size;
249 struct usnic_uiom_chunk *chunk;
250 struct usnic_uiom_interval_node *interval_node;
251 dma_addr_t pa;
252 dma_addr_t pa_start = 0;
253 dma_addr_t pa_end = 0;
254 long int va_start = -EINVAL;
255 struct usnic_uiom_pd *pd = uiomr->pd;
256 long int va = uiomr->va & PAGE_MASK;
257 int flags = IOMMU_READ | IOMMU_CACHE;
258
259 flags |= (uiomr->writable) ? IOMMU_WRITE : 0;
260 chunk = list_first_entry(&uiomr->chunk_list, struct usnic_uiom_chunk,
261 list);
262 list_for_each_entry(interval_node, intervals, link) {
263 iter_chunk:
264 for (i = 0; i < chunk->nents; i++, va += PAGE_SIZE) {
265 pa = sg_phys(&chunk->page_list[i]);
266 if ((va >> PAGE_SHIFT) < interval_node->start)
267 continue;
268
269 if ((va >> PAGE_SHIFT) == interval_node->start) {
270 /* First page of the interval */
271 va_start = va;
272 pa_start = pa;
273 pa_end = pa;
274 }
275
276 WARN_ON(va_start == -EINVAL);
277
278 if ((pa_end + PAGE_SIZE != pa) &&
279 (pa != pa_start)) {
280 /* PAs are not contiguous */
281 size = pa_end - pa_start + PAGE_SIZE;
282 usnic_dbg("va 0x%lx pa %pa size 0x%zx flags 0x%x",
283 va_start, &pa_start, size, flags);
284 err = iommu_map(pd->domain, va_start, pa_start,
285 size, flags);
The iommu_map() function sleeps.
286 if (err) {
287 usnic_err("Failed to map va 0x%lx pa %pa size 0x%zx with err %d\n",
288 va_start, &pa_start, size, err);
289 goto err_out;
290 }
291 va_start = va;
292 pa_start = pa;
293 pa_end = pa;
294 }
295
296 if ((va >> PAGE_SHIFT) == interval_node->last) {
297 /* Last page of the interval */
298 size = pa - pa_start + PAGE_SIZE;
299 usnic_dbg("va 0x%lx pa %pa size 0x%zx flags 0x%x\n",
300 va_start, &pa_start, size, flags);
301 err = iommu_map(pd->domain, va_start, pa_start,
302 size, flags);
iommu_map() again.
303 if (err) {
304 usnic_err("Failed to map va 0x%lx pa %pa size 0x%zx with err %d\n",
305 va_start, &pa_start, size, err);
306 goto err_out;
307 }
308 break;
309 }
310
311 if (pa != pa_start)
312 pa_end += PAGE_SIZE;
313 }
314
315 if (i == chunk->nents) {
316 /*
317 * Hit last entry of the chunk,
318 * hence advance to next chunk
319 */
320 chunk = list_first_entry(&chunk->list,
321 struct usnic_uiom_chunk,
322 list);
323 goto iter_chunk;
324 }
325 }
326
327 return 0;
328
329 err_out:
330 usnic_uiom_unmap_sorted_intervals(intervals, pd);
331 return err;
332 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
2021-07-05 10:23 Dan Carpenter
@ 2021-07-05 13:47 ` Robin Murphy
2021-07-05 15:21 ` Jason Gunthorpe
0 siblings, 1 reply; 7+ messages in thread
From: Robin Murphy @ 2021-07-05 13:47 UTC (permalink / raw)
To: Dan Carpenter, umalhi; +Cc: linux-rdma, iommu
On 2021-07-05 11:23, Dan Carpenter wrote:
> [ Ancient code, but the bug seems real enough still. -dan ]
>
> Hello Upinder Malhi,
>
> The patch e3cf00d0a87f: "IB/usnic: Add Cisco VIC low-level hardware
> driver" from Sep 10, 2013, leads to the following static checker
> warning:
>
> drivers/iommu/iommu.c:2482 iommu_map()
> warn: sleeping in atomic context
>
> drivers/infiniband/hw/usnic/usnic_uiom.c
> 244 static int usnic_uiom_map_sorted_intervals(struct list_head *intervals,
> 245 struct usnic_uiom_reg *uiomr)
>
> This function is always called from usnic_uiom_reg_get() which is holding
> spin_lock(&pd->lock); so it can't sleep.
FWIW back in those days it wasn't really well defined whether
iommu_map() was callable from non-sleeping contexts (the arch/arm DMA
API code relied on it, for instance). It was only formalised 2 years ago
by 781ca2de89ba ("iommu: Add gfp parameter to iommu_ops::map") which
introduced the might_sleep() check that's firing there. I guess these
calls want to be updated to iommu_map_atomic() now.
Robin.
> 246 {
> 247 int i, err;
> 248 size_t size;
> 249 struct usnic_uiom_chunk *chunk;
> 250 struct usnic_uiom_interval_node *interval_node;
> 251 dma_addr_t pa;
> 252 dma_addr_t pa_start = 0;
> 253 dma_addr_t pa_end = 0;
> 254 long int va_start = -EINVAL;
> 255 struct usnic_uiom_pd *pd = uiomr->pd;
> 256 long int va = uiomr->va & PAGE_MASK;
> 257 int flags = IOMMU_READ | IOMMU_CACHE;
> 258
> 259 flags |= (uiomr->writable) ? IOMMU_WRITE : 0;
> 260 chunk = list_first_entry(&uiomr->chunk_list, struct usnic_uiom_chunk,
> 261 list);
> 262 list_for_each_entry(interval_node, intervals, link) {
> 263 iter_chunk:
> 264 for (i = 0; i < chunk->nents; i++, va += PAGE_SIZE) {
> 265 pa = sg_phys(&chunk->page_list[i]);
> 266 if ((va >> PAGE_SHIFT) < interval_node->start)
> 267 continue;
> 268
> 269 if ((va >> PAGE_SHIFT) == interval_node->start) {
> 270 /* First page of the interval */
> 271 va_start = va;
> 272 pa_start = pa;
> 273 pa_end = pa;
> 274 }
> 275
> 276 WARN_ON(va_start == -EINVAL);
> 277
> 278 if ((pa_end + PAGE_SIZE != pa) &&
> 279 (pa != pa_start)) {
> 280 /* PAs are not contiguous */
> 281 size = pa_end - pa_start + PAGE_SIZE;
> 282 usnic_dbg("va 0x%lx pa %pa size 0x%zx flags 0x%x",
> 283 va_start, &pa_start, size, flags);
> 284 err = iommu_map(pd->domain, va_start, pa_start,
> 285 size, flags);
>
> The iommu_map() function sleeps.
>
> 286 if (err) {
> 287 usnic_err("Failed to map va 0x%lx pa %pa size 0x%zx with err %d\n",
> 288 va_start, &pa_start, size, err);
> 289 goto err_out;
> 290 }
> 291 va_start = va;
> 292 pa_start = pa;
> 293 pa_end = pa;
> 294 }
> 295
> 296 if ((va >> PAGE_SHIFT) == interval_node->last) {
> 297 /* Last page of the interval */
> 298 size = pa - pa_start + PAGE_SIZE;
> 299 usnic_dbg("va 0x%lx pa %pa size 0x%zx flags 0x%x\n",
> 300 va_start, &pa_start, size, flags);
> 301 err = iommu_map(pd->domain, va_start, pa_start,
> 302 size, flags);
>
> iommu_map() again.
>
> 303 if (err) {
> 304 usnic_err("Failed to map va 0x%lx pa %pa size 0x%zx with err %d\n",
> 305 va_start, &pa_start, size, err);
> 306 goto err_out;
> 307 }
> 308 break;
> 309 }
> 310
> 311 if (pa != pa_start)
> 312 pa_end += PAGE_SIZE;
> 313 }
> 314
> 315 if (i == chunk->nents) {
> 316 /*
> 317 * Hit last entry of the chunk,
> 318 * hence advance to next chunk
> 319 */
> 320 chunk = list_first_entry(&chunk->list,
> 321 struct usnic_uiom_chunk,
> 322 list);
> 323 goto iter_chunk;
> 324 }
> 325 }
> 326
> 327 return 0;
> 328
> 329 err_out:
> 330 usnic_uiom_unmap_sorted_intervals(intervals, pd);
> 331 return err;
> 332 }
>
> regards,
> dan carpenter
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
2021-07-05 13:47 ` Robin Murphy
@ 2021-07-05 15:21 ` Jason Gunthorpe
2021-07-05 15:39 ` Robin Murphy
2021-07-05 15:45 ` Dan Carpenter
0 siblings, 2 replies; 7+ messages in thread
From: Jason Gunthorpe @ 2021-07-05 15:21 UTC (permalink / raw)
To: Robin Murphy, Christian Benvenuti, Nelson Escobar
Cc: Dan Carpenter, umalhi, linux-rdma, iommu
On Mon, Jul 05, 2021 at 02:47:36PM +0100, Robin Murphy wrote:
> On 2021-07-05 11:23, Dan Carpenter wrote:
> > [ Ancient code, but the bug seems real enough still. -dan ]
> >
> > Hello Upinder Malhi,
> >
> > The patch e3cf00d0a87f: "IB/usnic: Add Cisco VIC low-level hardware
> > driver" from Sep 10, 2013, leads to the following static checker
> > warning:
> >
> > drivers/iommu/iommu.c:2482 iommu_map()
> > warn: sleeping in atomic context
> >
> > drivers/infiniband/hw/usnic/usnic_uiom.c
> > 244 static int usnic_uiom_map_sorted_intervals(struct list_head *intervals,
> > 245 struct usnic_uiom_reg *uiomr)
> >
> > This function is always called from usnic_uiom_reg_get() which is holding
> > spin_lock(&pd->lock); so it can't sleep.
>
> FWIW back in those days it wasn't really well defined whether iommu_map()
> was callable from non-sleeping contexts (the arch/arm DMA API code relied on
> it, for instance). It was only formalised 2 years ago by 781ca2de89ba
> ("iommu: Add gfp parameter to iommu_ops::map") which introduced the
> might_sleep() check that's firing there. I guess these calls want to be
> updated to iommu_map_atomic() now.
Does this mean this driver doesn't work at all upstream? I would be
quite interested to delete it.
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
2021-07-05 15:21 ` Jason Gunthorpe
@ 2021-07-05 15:39 ` Robin Murphy
2021-07-05 15:45 ` Dan Carpenter
1 sibling, 0 replies; 7+ messages in thread
From: Robin Murphy @ 2021-07-05 15:39 UTC (permalink / raw)
To: Jason Gunthorpe, Christian Benvenuti, Nelson Escobar
Cc: Dan Carpenter, umalhi, linux-rdma, iommu
On 2021-07-05 16:21, Jason Gunthorpe wrote:
> On Mon, Jul 05, 2021 at 02:47:36PM +0100, Robin Murphy wrote:
>> On 2021-07-05 11:23, Dan Carpenter wrote:
>>> [ Ancient code, but the bug seems real enough still. -dan ]
>>>
>>> Hello Upinder Malhi,
>>>
>>> The patch e3cf00d0a87f: "IB/usnic: Add Cisco VIC low-level hardware
>>> driver" from Sep 10, 2013, leads to the following static checker
>>> warning:
>>>
>>> drivers/iommu/iommu.c:2482 iommu_map()
>>> warn: sleeping in atomic context
>>>
>>> drivers/infiniband/hw/usnic/usnic_uiom.c
>>> 244 static int usnic_uiom_map_sorted_intervals(struct list_head *intervals,
>>> 245 struct usnic_uiom_reg *uiomr)
>>>
>>> This function is always called from usnic_uiom_reg_get() which is holding
>>> spin_lock(&pd->lock); so it can't sleep.
>>
>> FWIW back in those days it wasn't really well defined whether iommu_map()
>> was callable from non-sleeping contexts (the arch/arm DMA API code relied on
>> it, for instance). It was only formalised 2 years ago by 781ca2de89ba
>> ("iommu: Add gfp parameter to iommu_ops::map") which introduced the
>> might_sleep() check that's firing there. I guess these calls want to be
>> updated to iommu_map_atomic() now.
>
> Does this mean this driver doesn't work at all upstream? I would be
> quite interested to delete it.
I think the only time it's actually in trouble is on AMD hardware if one
of those iommu_map() calls has to allocate a new pagetable page and that
allocation has to go down whichever reclaim path actually sleeps.
Historically all the other IOMMU drivers it might have come into contact
with already used GFP_ATOMIC for their internal allocations anyway (AMD
was the only one using a mutex instead of a spinlock internally), and
some like intel-iommu still haven't relaxed that even now.
Robin.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
2021-07-05 15:21 ` Jason Gunthorpe
2021-07-05 15:39 ` Robin Murphy
@ 2021-07-05 15:45 ` Dan Carpenter
1 sibling, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2021-07-05 15:45 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Robin Murphy, Christian Benvenuti, Nelson Escobar, umalhi,
linux-rdma, iommu
On Mon, Jul 05, 2021 at 12:21:38PM -0300, Jason Gunthorpe wrote:
> On Mon, Jul 05, 2021 at 02:47:36PM +0100, Robin Murphy wrote:
> > On 2021-07-05 11:23, Dan Carpenter wrote:
> > > [ Ancient code, but the bug seems real enough still. -dan ]
> > >
> > > Hello Upinder Malhi,
> > >
> > > The patch e3cf00d0a87f: "IB/usnic: Add Cisco VIC low-level hardware
> > > driver" from Sep 10, 2013, leads to the following static checker
> > > warning:
> > >
> > > drivers/iommu/iommu.c:2482 iommu_map()
> > > warn: sleeping in atomic context
> > >
> > > drivers/infiniband/hw/usnic/usnic_uiom.c
> > > 244 static int usnic_uiom_map_sorted_intervals(struct list_head *intervals,
> > > 245 struct usnic_uiom_reg *uiomr)
> > >
> > > This function is always called from usnic_uiom_reg_get() which is holding
> > > spin_lock(&pd->lock); so it can't sleep.
> >
> > FWIW back in those days it wasn't really well defined whether iommu_map()
> > was callable from non-sleeping contexts (the arch/arm DMA API code relied on
> > it, for instance). It was only formalised 2 years ago by 781ca2de89ba
> > ("iommu: Add gfp parameter to iommu_ops::map") which introduced the
> > might_sleep() check that's firing there. I guess these calls want to be
> > updated to iommu_map_atomic() now.
>
> Does this mean this driver doesn't work at all upstream? I would be
> quite interested to delete it.
It just means it hasn't been used with CONFIG_DEBUG_ATOMIC_SLEEP enabled
within the past two years.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* [bug report] IB/usnic: Add Cisco VIC low-level hardware driver
@ 2017-10-12 12:57 Dan Carpenter
0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2017-10-12 12:57 UTC (permalink / raw)
To: umalhi-FYB4Gu1CFyUAvxtiuMwx3w; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
Hello Upinder Malhi,
The patch e3cf00d0a87f: "IB/usnic: Add Cisco VIC low-level hardware
driver" from Sep 10, 2013, leads to the following static checker
warning:
drivers/infiniband/hw/usnic/usnic_uiom.c:366 usnic_uiom_reg_get()
warn: overflowed symbol reused: 'size'
drivers/infiniband/hw/usnic/usnic_uiom.c
335 struct usnic_uiom_reg *usnic_uiom_reg_get(struct usnic_uiom_pd *pd,
336 unsigned long addr, size_t size,
^^^^^^^^^^^
The size is comes from the user. The call tree is:
ib_uverbs_reg_mr() <-- does a copy_from_user()
-> pd->device->reg_user_mr()
-> usnic_ib_reg_mr()
-> usnic_uiom_reg_get()
337 int writable, int dmasync)
338 {
339 struct usnic_uiom_reg *uiomr;
340 unsigned long va_base, vpn_start, vpn_last;
341 unsigned long npages;
342 int offset, err;
343 LIST_HEAD(sorted_diff_intervals);
344
345 /*
346 * Intel IOMMU map throws an error if a translation entry is
347 * changed from read to write. This module may not unmap
348 * and then remap the entry after fixing the permission
349 * b/c this open up a small windows where hw DMA may page fault
350 * Hence, make all entries to be writable.
351 */
352 writable = 1;
353
354 va_base = addr & PAGE_MASK;
355 offset = addr & ~PAGE_MASK;
356 npages = PAGE_ALIGN(size + offset) >> PAGE_SHIFT;
^^^^^^^^^^^^^^^^^^^^^^^^^
size + offset can wrap and PAGE_ALIGN() has an addition as well.
357 vpn_start = (addr & PAGE_MASK) >> PAGE_SHIFT;
358 vpn_last = vpn_start + npages - 1;
359
360 uiomr = kmalloc(sizeof(*uiomr), GFP_KERNEL);
361 if (!uiomr)
362 return ERR_PTR(-ENOMEM);
363
364 uiomr->va = va_base;
365 uiomr->offset = offset;
366 uiomr->length = size;
^^^^
The test is trying to be a tiny bit smart by not immediately complaining
about the user controlled integer overflow but instead waiting to see if
we use "size" again. I don't know if this code has a real bug. I'm
casting about trying to find a heuristic which is easy enough for static
analysis but doesn't have too many false positives.
367 uiomr->writable = writable;
368 uiomr->pd = pd;
369
370 err = usnic_uiom_get_pages(addr, size, writable, dmasync,
371 &uiomr->chunk_list);
372 if (err) {
373 usnic_err("Failed get_pages vpn [0x%lx,0x%lx] err %d\n",
374 vpn_start, vpn_last, err);
375 goto out_free_uiomr;
376 }
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-08-04 9:08 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-04 9:08 [bug report] IB/usnic: Add Cisco VIC low-level hardware driver Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2021-07-05 10:23 Dan Carpenter
2021-07-05 13:47 ` Robin Murphy
2021-07-05 15:21 ` Jason Gunthorpe
2021-07-05 15:39 ` Robin Murphy
2021-07-05 15:45 ` Dan Carpenter
2017-10-12 12:57 Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox