Date: Thu, 14 Jun 2018 17:06:12 +0530 From: Pratyush Yadav <pratiy0100@gmail.com> To: hps@selasky.org Cc: freebsd-hackers@freebsd.org, =?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: <CA%2BX=3TK1=b2xo4NyvqDy33wVmQcp%2B0CzkkY-yr0T3NdywDz0Qw@mail.gmail.com> In-Reply-To: <8170cb61-8c85-3dc8-1e16-bdb6a2ada7db@selasky.org> References: <CA%2BX=3TL1yeLvwPFqt0GAG7qxKC3Gv9mWzdY84=WD%2BSeLRihv1A@mail.gmail.com> <8170cb61-8c85-3dc8-1e16-bdb6a2ada7db@selasky.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Jun 14, 2018 at 4:23 PM Hans Petter Selasky <hps@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CA%2BX=3TK1=b2xo4NyvqDy33wVmQcp%2B0CzkkY-yr0T3NdywDz0Qw>