Date: Thu, 14 Jun 2018 12:53:06 +0200 From: Hans Petter Selasky <hps@selasky.org> To: Pratyush Yadav <pratiy0100@gmail.com>, freebsd-hackers@freebsd.org Cc: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= <royger@freebsd.org>, Edward Napierala <trasz@freebsd.org> Subject: Re: Bug in subr_bus_dma.c's bus_dmamap_load() when multiple dma maps are created from a single tag on x86 Message-ID: <8170cb61-8c85-3dc8-1e16-bdb6a2ada7db@selasky.org> In-Reply-To: <CA%2BX=3TL1yeLvwPFqt0GAG7qxKC3Gv9mWzdY84=WD%2BSeLRihv1A@mail.gmail.com> References: <CA%2BX=3TL1yeLvwPFqt0GAG7qxKC3Gv9mWzdY84=WD%2BSeLRihv1A@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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. --HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8170cb61-8c85-3dc8-1e16-bdb6a2ada7db>