From owner-freebsd-hackers@freebsd.org Thu Jun 14 11:36:50 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 3805B101F9A4 for ; Thu, 14 Jun 2018 11:36:50 +0000 (UTC) (envelope-from pratiy0100@gmail.com) Received: from mail-wm0-x242.google.com (mail-wm0-x242.google.com [IPv6:2a00:1450:400c:c09::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 9419D85809; Thu, 14 Jun 2018 11:36:49 +0000 (UTC) (envelope-from pratiy0100@gmail.com) Received: by mail-wm0-x242.google.com with SMTP id p126-v6so9997419wmb.2; Thu, 14 Jun 2018 04:36:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=HFp8rPvNIHboXcg99GxHGX4sRkszBw1x94RGaVisS7Q=; b=Y3Oef10nrBmzIXb3XzNYMZ4LYK2ZPNmcF1l7D0jiCPelhwhCVxOATQu2R6duUwTsKV SFuTTFCw686t6A7qhJMM6iExF/G4P1YKy19C2TqZWsXIGPXHMQaS0yyr0MmBGMpgTDAH geiV/SK8QLynTpyZaJOVPoH835Nk0w//03nKGiujV8WoVWvNirnGlrS4VcKqmFTXbmEe 6bAxAFi/vB89s2rpbRkLgofXaW9lWWEsiiCN9HGnIBq60+VvEfsW45qQ4Fxz1/NsGMcB ps4sFVQmyKixTQ2ohyw0lQ/7ZcmONvLrxJLiMepXc2FmBWhA9aI6fUb16tKbecD6Uafz neOA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=HFp8rPvNIHboXcg99GxHGX4sRkszBw1x94RGaVisS7Q=; b=P++sJAIv6QFS+K37Y+i1iuMSEByvdbrrzXY1A1g/FbBiJdrOso2xePADw6S9vOfuux DHCWkisijAwc1XNOSfkenfH44oAp/JuZ3ve2H4IMt3nEuHxQZukyooL+tTPWAuBy2JXd xcCp8rQyv1Sw4dnfNDpuwi8jQ8I6OVdn9twpj91ePRZgITc8pwy53InP2mu8nK1uFMh6 GcBcw+vIXDZV1NnchFb3351VFtKcdujamTIj/WLiyw+6g43ceK6IGu0kqT3ZoOcHwG0i bl1irUF3uyA3WedeHKYdhIhNnC/QnLDcd3ckzARtIBgcVnDFJcxPJtY+gsXfFEJTaZcF 94pA== X-Gm-Message-State: APt69E0CD0ubww5wRKS8pVGAcUXTrLuLm8lRnHO3Ck8mN+RG94VkM/Pj Gky5/xnZSOgE+rEK/tz4x90oC9bR6O9TvE01wkQ= X-Google-Smtp-Source: ADUXVKLLo8zYKGzYJVg+JC4rRzPOSoK7yE66WAhkLZZnjXPit4EGyMmeQTwr1scvl8nVd8NML99SOBsE0oldZM3kqtw= X-Received: by 2002:a50:ad78:: with SMTP id z53-v6mr2368985edc.306.1528976208587; Thu, 14 Jun 2018 04:36:48 -0700 (PDT) MIME-Version: 1.0 References: <8170cb61-8c85-3dc8-1e16-bdb6a2ada7db@selasky.org> In-Reply-To: <8170cb61-8c85-3dc8-1e16-bdb6a2ada7db@selasky.org> From: Pratyush Yadav Date: Thu, 14 Jun 2018 17:06:12 +0530 Message-ID: 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: hps@selasky.org Cc: freebsd-hackers@freebsd.org, =?UTF-8?Q?Roger_Pau_Monn=C3=A9?= , Edward Napierala Content-Type: text/plain; charset="UTF-8" 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 11:36:50 -0000 On Thu, Jun 14, 2018 at 4:23 PM Hans Petter Selasky 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