Bug in subr_bus_dma.c's bus_dmamap_load() when multiple dma maps are created from a single tag on x86

[Pratyush Yadav]

Hi everyone,

I was looking at the busdma code for x86 and I notice that the claim
in the bus_dma(9) man page might not be correct: "Multiple maps can be
associated with one DMA tag."

Looking at the code of sys/x86/x86/busdma_bounce.c (which I believe is
the default busdma implementation for x86), I realize that a map does
not necessarily use all the segments of the dma tag (specified by the
parameter nsegments on tag creation).

So how many segments does a map actually use? Looking at
busdma_bounce.c:644,690 it seems we can calculate that from the
pointer segp. It contains the index of starting segment on entrace to
the load function, and the ending segment on exit.

Sounds all fine so far. But then if we look at map_complete()
(busdma_bounce.c:908), it returns the entire segments array of the dma
*tag*. I emphasize tag because the tag's segments array holds the
segments of all the maps created with the tag.

Going a layer up, sys/kern/subr_bus_dma.c:328 simply passes nsegs(=
-1) in the segp parameter to load, not bothering to check if some
segments have been used already by other maps. It then calculates the
number of segments used using the value of nsegs after return and
calls map_complete() to get the segments array. It then passes the
segments array to the driver callback with the number of segments,
nsegs.

Effectively, to the driver it seems that the map's segments start from
index 0 and go till nsegs - 1. This is not true if another map has
been loaded in the meantime with the same tag.

We are possibly over-writing the previous map's segments.
busdma_bounce.c:669 simply uses segs[0] if the value of segp is -1,
which is what bus_dmamap_load() in subr_bus_dma.c:328 passes to
_bus_dmamap_load_buffer(). segs[0] would also be used by any
subsequent map's load. This means the previous map's values are
over-written.

This might cause problems when the driver is in its callback using the
segments array and another map is loaded.

Unfortunately, I do not posses the knowledge or experience with
FreeBSD's drivers (drivers in general) to test for this bug.

I have not looked at the code in other architectures. The same problem
might present itself there as well.

If I made a mistake somewhere in my reasoning, let me know.

[Hans Petter Selasky]

Post by Pratyush Yadav
Hi everyone,
I was looking at the busdma code for x86 and I notice that the claim
in the bus_dma(9) man page might not be correct: "Multiple maps can be
associated with one DMA tag."
Looking at the code of sys/x86/x86/busdma_bounce.c (which I believe is
the default busdma implementation for x86), I realize that a map does
not necessarily use all the segments of the dma tag (specified by the
parameter nsegments on tag creation).
So how many segments does a map actually use? Looking at
busdma_bounce.c:644,690 it seems we can calculate that from the
pointer segp. It contains the index of starting segment on entrace to
the load function, and the ending segment on exit.
Sounds all fine so far. But then if we look at map_complete()
(busdma_bounce.c:908), it returns the entire segments array of the dma
*tag*. I emphasize tag because the tag's segments array holds the
segments of all the maps created with the tag.
Going a layer up, sys/kern/subr_bus_dma.c:328 simply passes nsegs(=
-1) in the segp parameter to load, not bothering to check if some
segments have been used already by other maps. It then calculates the
number of segments used using the value of nsegs after return and
calls map_complete() to get the segments array. It then passes the
segments array to the driver callback with the number of segments,
nsegs.
Effectively, to the driver it seems that the map's segments start from
index 0 and go till nsegs - 1. This is not true if another map has
been loaded in the meantime with the same tag.
We are possibly over-writing the previous map's segments.
busdma_bounce.c:669 simply uses segs[0] if the value of segp is -1,
which is what bus_dmamap_load() in subr_bus_dma.c:328 passes to
_bus_dmamap_load_buffer(). segs[0] would also be used by any
subsequent map's load. This means the previous map's values are
over-written.
This might cause problems when the driver is in its callback using the
segments array and another map is loaded.
Unfortunately, I do not posses the knowledge or experience with
FreeBSD's drivers (drivers in general) to test for this bug.
I have not looked at the code in other architectures. The same problem
might present itself there as well.
If I made a mistake somewhere in my reasoning, let me know.

Hi,

Use of bus_dmamap_load() on the same tag must be serialized using a
mutex. Maybe it is not so clearly documented. Else like you see the
temporary segs[] list may be overwritten.

--HPS

[Pratyush Yadav]

Post by Hans Petter Selasky

Post by Pratyush Yadav
Hi everyone,
I was looking at the busdma code for x86 and I notice that the claim
in the bus_dma(9) man page might not be correct: "Multiple maps can be
associated with one DMA tag."
Looking at the code of sys/x86/x86/busdma_bounce.c (which I believe is
the default busdma implementation for x86), I realize that a map does
not necessarily use all the segments of the dma tag (specified by the
parameter nsegments on tag creation).
So how many segments does a map actually use? Looking at
busdma_bounce.c:644,690 it seems we can calculate that from the
pointer segp. It contains the index of starting segment on entrace to
the load function, and the ending segment on exit.
Sounds all fine so far. But then if we look at map_complete()
(busdma_bounce.c:908), it returns the entire segments array of the dma
*tag*. I emphasize tag because the tag's segments array holds the
segments of all the maps created with the tag.
Going a layer up, sys/kern/subr_bus_dma.c:328 simply passes nsegs(=
-1) in the segp parameter to load, not bothering to check if some
segments have been used already by other maps. It then calculates the
number of segments used using the value of nsegs after return and
calls map_complete() to get the segments array. It then passes the
segments array to the driver callback with the number of segments,
nsegs.
Effectively, to the driver it seems that the map's segments start from
index 0 and go till nsegs - 1. This is not true if another map has
been loaded in the meantime with the same tag.
We are possibly over-writing the previous map's segments.
busdma_bounce.c:669 simply uses segs[0] if the value of segp is -1,
which is what bus_dmamap_load() in subr_bus_dma.c:328 passes to
_bus_dmamap_load_buffer(). segs[0] would also be used by any
subsequent map's load. This means the previous map's values are
over-written.
This might cause problems when the driver is in its callback using the
segments array and another map is loaded.
Unfortunately, I do not posses the knowledge or experience with
FreeBSD's drivers (drivers in general) to test for this bug.
I have not looked at the code in other architectures. The same problem
might present itself there as well.
If I made a mistake somewhere in my reasoning, let me know.

Hi,
Use of bus_dmamap_load() on the same tag must be serialized using a
mutex. Maybe it is not so clearly documented. Else like you see the
temporary segs[] list may be overwritten.

The man page does say "the most efficient way to protect the tag
resources is through the lock that the driver uses." But I never
thought it meant that the loads (among other things) must be
serialized. Maybe it is better to explicitly mention it in the man
page?