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

[Pratyush Yadav]

On Thu, Jun 14, 2018 at 4:23 PM Hans Petter Selasky <hps at selasky.org> wrote:
>
> On 06/14/18 10:59, Pratyush Yadav wrote:
> > 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?

-- 
Regards,
Pratyush Yadav