Re: [PATCH] bitmap: fix bitmap_find_free_region()

On Fri, 19 Dec 2008, Johannes Weiner wrote:

On Fri, Dec 19, 2008 at 04:09:42PM +0100, Guennadi Liakhovetski wrote:
Yes, this is the i.MX31 video capture driver (patch has once been
submitted, new version in work). The thing is, we reserve and declare a
fixed size coherent region in the board code at start-up, but then what
the driver allocates depends on what a user-space application requests -
what size video frame and how many buffers. And the driver does not check
how much coherent memory it has available, it relies on
dma_alloc_coherent() to tell. In fact, the actual i.MX31 camera driver
knows nothing about this allocation, this all happens automatically in
soc_camera.c::soc_camera_mmap() ->
videobuf-dma-contig.c::__videobuf_mmap_mapper().

I think we should add the check, WARN if it's true and return an
appropriate error number. It will be handled gracefully then and we
still know which callsite screwed up.

Well, given above you'd get warning each time the user requests too big a
frame or too many buffers, which is not nice.

Indeed, that's BS then.

The region size information is local to the dma-coherent code. So
either the callsite keeps track of the size itself or, if it can not
as in your case, we should check for the correct size on exactly that
layer of code, as well. Does that sound reasonable?

Why do other driver not need this? I will look further into it. If
it's any use, here is a patch that does the check on the allocation
level.

Well, I don't quite understand. Doesn't bitmap_find_free_region() _have_
to return an error if the request cannot be satisfied? Isn't it what it
does if after scanning the bitmap no suitable free region is found? What's
the difference? Why don't we want to fix _that_ function? It also has
other users - currently I see 3 of them in the kernel:

mm/vmalloc.c
arch/powerpc/sysdev/msi_bitmap.c (5 occurrences)
arch/sh/kernel/cpu/sh4/sq.c

are we sure that they all are safe wrt to this problem?

Yes, I understand that the caller _can_ verify whether the requested
region at all fits into the bitmap, but from the PoV of offering a
consistent API this seems strange.

Thanks
Guennadi


---
dma-coherent: catch oversized requests to dma_alloc_from_coherent()

Prevent passing an order to bitmap_find_free_region() that is larger
than the actual bitmap can represent.

These requests can come from device drivers that have no idea how big
the dma region is and need to rely on dma_alloc_from_coherent() to
sort it out for them.

Reported-by: Guennadi Liakhovetski <lg@xxxxxxx>
Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>
---

diff --git a/kernel/dma-coherent.c b/kernel/dma-coherent.c
index f013a0c..56ea73e 100644
--- a/kernel/dma-coherent.c
+++ b/kernel/dma-coherent.c
@@ -112,6 +112,9 @@ int dma_alloc_from_coherent(struct device *dev, ssize_t size,
struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL;
int order = get_order(size);

+ if (unlikely(size > mem->size))
+ return 0;
+
if (mem) {
int page = bitmap_find_free_region(mem->bitmap, mem->size,
order);



---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@xxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: [PATCH] bitmap: fix bitmap_find_free_region()
    ... size is smaller than the entire bitmap. ... It is hard to believe, that this is a bug, last time this code was touched ... this is the i.MX31 video capture driver (patch has once been ... the driver allocates depends on what a user-space application requests - ...
    (Linux-Kernel)
  • Re: [PATCH] bitmap: fix bitmap_find_free_region()
    ... fixed size coherent region in the board code at start-up, ... the driver allocates depends on what a user-space application requests - ... And the driver does not check ... here is a patch that does the check on the allocation ...
    (Linux-Kernel)
  • Re: [PATCH 1/2] lld busy status exporting interface
    ... they detect busy state on its low-level device like host/bus/device. ... If the request stacking driver dispatches and submits requests to ... stacking driver can check it and stop dispatching requests if busy. ... Stacking drivers like request-based dm don't. ...
    (Linux-Kernel)
  • Re: which kind of driver to design for AoE protocol?
    ... With your idea of having requests distributed to ... We do have working ATA-over-Ethernet driver shipping. ... drivers have been very concerned about overlapping ... overlapping of CPU operations to really make things fly. ...
    (microsoft.public.development.device.drivers)
  • Re: [dm-devel] Re: dm: bounce_pfn limit added
    ... I2o on this node is able to handle only requests with up to 38 segments. ... In this way underlying device calls blk_recalc_rq_segmentsto recount number ... Unfortunately there is not any checks and when i2o driver handles this incorrect ... the patch propagates the restriction of i2o ...
    (Linux-Kernel)