From owner-freebsd-hackers@freebsd.org Thu Jun 14 10:53:24 2018 Return-Path: Delivered-To: freebsd-hackers@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id D5D82101BF41 for ; Thu, 14 Jun 2018 10:53:24 +0000 (UTC) (envelope-from hps@selasky.org) Received: from mail.turbocat.net (turbocat.net [88.99.82.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 6779283AEA; Thu, 14 Jun 2018 10:53:24 +0000 (UTC) (envelope-from hps@selasky.org) Received: from hps2016.home.selasky.org (unknown [62.141.128.70]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.turbocat.net (Postfix) with ESMTPSA id 67236260199; Thu, 14 Jun 2018 12:53:22 +0200 (CEST) Subject: Re: Bug in subr_bus_dma.c's bus_dmamap_load() when multiple dma maps are created from a single tag on x86 To: Pratyush Yadav , freebsd-hackers@freebsd.org Cc: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= , Edward Napierala References: From: Hans Petter Selasky Message-ID: <8170cb61-8c85-3dc8-1e16-bdb6a2ada7db@selasky.org> Date: Thu, 14 Jun 2018 12:53:06 +0200 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Jun 2018 10:53:25 -0000 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