From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerome Glisse Subject: Re: [PATCH] dma-buf: fix debugfs versus rcu and fence dumping Date: Thu, 6 Dec 2018 10:21:17 -0500 Message-ID: <20181206152116.GA3544@redhat.com> References: <20181206014103.1364-1-jglisse@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by gabe.freedesktop.org (Postfix) with ESMTPS id E224E6E070 for ; Thu, 6 Dec 2018 15:21:20 +0000 (UTC) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: "Koenig, Christian" Cc: Daniel Vetter , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "linaro-mm-sig@lists.linaro.org" , "stable@vger.kernel.org" , =?iso-8859-1?Q?St=E9phane?= Marchesin , "linux-media@vger.kernel.org" List-Id: dri-devel@lists.freedesktop.org T24gVGh1LCBEZWMgMDYsIDIwMTggYXQgMDg6MDk6MjhBTSArMDAwMCwgS29lbmlnLCBDaHJpc3Rp YW4gd3JvdGU6Cj4gQW0gMDYuMTIuMTggdW0gMDI6NDEgc2NocmllYiBqZ2xpc3NlQHJlZGhhdC5j b206Cj4gPiBGcm9tOiBKw6lyw7RtZSBHbGlzc2UgPGpnbGlzc2VAcmVkaGF0LmNvbT4KPiA+Cj4g PiBUaGUgZGVidWdmcyB0YWtlIHJlZmVyZW5jZSBvbiBmZW5jZSB3aXRob3V0IGRyb3BwaW5nIHRo ZW0uIEFsc28gdGhlCj4gPiByY3Ugc2VjdGlvbiBhcmUgbm90IHdlbGwgYmFsYW5jZS4gRml4IGFs bCB0aGF0IC4uLgo+ID4KPiA+IFNpZ25lZC1vZmYtYnk6IErDqXLDtG1lIEdsaXNzZSA8amdsaXNz ZUByZWRoYXQuY29tPgo+ID4gQ2M6IENocmlzdGlhbiBLw7ZuaWcgPGNocmlzdGlhbi5rb2VuaWdA YW1kLmNvbT4KPiA+IENjOiBEYW5pZWwgVmV0dGVyIDxkYW5pZWwudmV0dGVyQGZmd2xsLmNoPgo+ ID4gQ2M6IFN1bWl0IFNlbXdhbCA8c3VtaXQuc2Vtd2FsQGxpbmFyby5vcmc+Cj4gPiBDYzogbGlu dXgtbWVkaWFAdmdlci5rZXJuZWwub3JnCj4gPiBDYzogZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNr dG9wLm9yZwo+ID4gQ2M6IGxpbmFyby1tbS1zaWdAbGlzdHMubGluYXJvLm9yZwo+ID4gQ2M6IFN0 w6lwaGFuZSBNYXJjaGVzaW4gPG1hcmNoZXVAY2hyb21pdW0ub3JnPgo+ID4gQ2M6IHN0YWJsZUB2 Z2VyLmtlcm5lbC5vcmcKPiAKPiBXZWxsIE5BSywgeW91IGFyZSBub3cgdGFraW5nIHRoZSBSQ1Ug bG9jayB0d2ljZSBhbmQgZHJvcHBpbmcgdGhlIFJDVSBhbmQgCj4gc3RpbGwgYWNjZXNzaW5nIGZv YmogaGFzIGEgaHVnZSBwb3RlbnRpYWwgZm9yIGFjY2Vzc2luZyBmcmVlZCB1cCBtZW1vcnkuCj4g Cj4gVGhlIG9ubHkgY29ycmVjdCB0aGluZyBJIGNhbiBzZWUgaGVyZSBpcyB0byBncmFiIGEgcmVm ZXJlbmNlIHRvIHRoZSAKPiBmZW5jZSBiZWZvcmUgcHJpbnRpbmcgYW55IGluZm8gb24gaXQsCj4g Q2hyaXN0aWFuLgoKSHUgPyBUaGF0IGlzIGV4YWN0bHkgd2hhdCBpIGFtIGRvaW5nLCB0YWtlIHJl ZmVyZW5jZSB1bmRlciByY3UsCnJjdV91bmxvY2sgcHJpbnQgdGhlIGZlbmNlIGluZm8sIGRyb3Ag dGhlIGZlbmNlIHJlZmVyZW5jZSwgcmN1CmxvY2sgcmluc2UgYW5kIHJlcGVhdCAuLi4KCk5vdGUg dGhhdCB0aGUgZm9iaiBpbiBfZXhpc3RpbmdfIGNvZGUgaXMgYWNjZXNzIG91dHNpZGUgdGhlIHJj dQplbmQgdGhhdCB0aGVyZSBpcyBhbiByY3UgaW1iYWxhbmNlIGluIHRoYXQgY29kZSBpZSBhIGxv bmxlbHkKcmN1X3VubG9jayBhZnRlciB0aGUgZm9yIGxvb3AuCgpTbyB0aGF0IHRoZSBleGlzdGlu ZyBjb2RlIGlzIGJyb2tlbi4KCj4gCj4gPiAtLS0KPiA+ICAgZHJpdmVycy9kbWEtYnVmL2RtYS1i dWYuYyB8IDExICsrKysrKysrKy0tCj4gPiAgIDEgZmlsZSBjaGFuZ2VkLCA5IGluc2VydGlvbnMo KyksIDIgZGVsZXRpb25zKC0pCj4gPgo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZG1hLWJ1Zi9k bWEtYnVmLmMgYi9kcml2ZXJzL2RtYS1idWYvZG1hLWJ1Zi5jCj4gPiBpbmRleCAxMzg4NDQ3NGQx NTguLmY2ZjRkZTQyYWM0OSAxMDA2NDQKPiA+IC0tLSBhL2RyaXZlcnMvZG1hLWJ1Zi9kbWEtYnVm LmMKPiA+ICsrKyBiL2RyaXZlcnMvZG1hLWJ1Zi9kbWEtYnVmLmMKPiA+IEBAIC0xMDUxLDI0ICsx MDUxLDMxIEBAIHN0YXRpYyBpbnQgZG1hX2J1Zl9kZWJ1Z19zaG93KHN0cnVjdCBzZXFfZmlsZSAq cywgdm9pZCAqdW51c2VkKQo+ID4gICAJCQlmb2JqID0gcmN1X2RlcmVmZXJlbmNlKHJvYmotPmZl bmNlKTsKPiA+ICAgCQkJc2hhcmVkX2NvdW50ID0gZm9iaiA/IGZvYmotPnNoYXJlZF9jb3VudCA6 IDA7Cj4gPiAgIAkJCWZlbmNlID0gcmN1X2RlcmVmZXJlbmNlKHJvYmotPmZlbmNlX2V4Y2wpOwo+ ID4gKwkJCWZlbmNlID0gZG1hX2ZlbmNlX2dldF9yY3UoZmVuY2UpOwo+ID4gICAJCQlpZiAoIXJl YWRfc2VxY291bnRfcmV0cnkoJnJvYmotPnNlcSwgc2VxKSkKPiA+ICAgCQkJCWJyZWFrOwo+ID4g ICAJCQlyY3VfcmVhZF91bmxvY2soKTsKPiA+ICAgCQl9Cj4gPiAtCj4gPiAtCQlpZiAoZmVuY2Up Cj4gPiArCQlpZiAoZmVuY2UpIHsKPiA+ICAgCQkJc2VxX3ByaW50ZihzLCAiXHRFeGNsdXNpdmUg ZmVuY2U6ICVzICVzICVzc2lnbmFsbGVkXG4iLAo+ID4gICAJCQkJICAgZmVuY2UtPm9wcy0+Z2V0 X2RyaXZlcl9uYW1lKGZlbmNlKSwKPiA+ICAgCQkJCSAgIGZlbmNlLT5vcHMtPmdldF90aW1lbGlu ZV9uYW1lKGZlbmNlKSwKPiA+ICAgCQkJCSAgIGRtYV9mZW5jZV9pc19zaWduYWxlZChmZW5jZSkg PyAiIiA6ICJ1biIpOwo+ID4gKwkJCWRtYV9mZW5jZV9wdXQoZmVuY2UpOwo+ID4gKwkJfQo+ID4g Kwo+ID4gKwkJcmN1X3JlYWRfbG9jaygpOwo+ID4gICAJCWZvciAoaSA9IDA7IGkgPCBzaGFyZWRf Y291bnQ7IGkrKykgewo+ID4gICAJCQlmZW5jZSA9IHJjdV9kZXJlZmVyZW5jZShmb2JqLT5zaGFy ZWRbaV0pOwo+ID4gICAJCQlpZiAoIWRtYV9mZW5jZV9nZXRfcmN1KGZlbmNlKSkKPiA+ICAgCQkJ CWNvbnRpbnVlOwo+ID4gKwkJCXJjdV9yZWFkX3VubG9jaygpOwo+ID4gICAJCQlzZXFfcHJpbnRm KHMsICJcdFNoYXJlZCBmZW5jZTogJXMgJXMgJXNzaWduYWxsZWRcbiIsCj4gPiAgIAkJCQkgICBm ZW5jZS0+b3BzLT5nZXRfZHJpdmVyX25hbWUoZmVuY2UpLAo+ID4gICAJCQkJICAgZmVuY2UtPm9w cy0+Z2V0X3RpbWVsaW5lX25hbWUoZmVuY2UpLAo+ID4gICAJCQkJICAgZG1hX2ZlbmNlX2lzX3Np Z25hbGVkKGZlbmNlKSA/ICIiIDogInVuIik7Cj4gPiArCQkJZG1hX2ZlbmNlX3B1dChmZW5jZSk7 Cj4gPiArCQkJcmN1X3JlYWRfbG9jaygpOwo+ID4gICAJCX0KPiA+ICAgCQlyY3VfcmVhZF91bmxv Y2soKTsKPiA+ICAgCj4gCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9w Lm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1k ZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED, USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D8BB1C04EB8 for ; Thu, 6 Dec 2018 15:21:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A356420989 for ; Thu, 6 Dec 2018 15:21:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A356420989 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726013AbeLFPVV (ORCPT ); Thu, 6 Dec 2018 10:21:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56876 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725879AbeLFPVU (ORCPT ); Thu, 6 Dec 2018 10:21:20 -0500 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 68284CA369; Thu, 6 Dec 2018 15:21:20 +0000 (UTC) Received: from redhat.com (ovpn-122-74.rdu2.redhat.com [10.10.122.74]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 0354D608C4; Thu, 6 Dec 2018 15:21:18 +0000 (UTC) Date: Thu, 6 Dec 2018 10:21:17 -0500 From: Jerome Glisse To: "Koenig, Christian" Cc: "linux-kernel@vger.kernel.org" , Daniel Vetter , Sumit Semwal , "linux-media@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "linaro-mm-sig@lists.linaro.org" , =?iso-8859-1?Q?St=E9phane?= Marchesin , "stable@vger.kernel.org" Subject: Re: [PATCH] dma-buf: fix debugfs versus rcu and fence dumping Message-ID: <20181206152116.GA3544@redhat.com> References: <20181206014103.1364-1-jglisse@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 06 Dec 2018 15:21:20 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Dec 06, 2018 at 08:09:28AM +0000, Koenig, Christian wrote: > Am 06.12.18 um 02:41 schrieb jglisse@redhat.com: > > From: Jérôme Glisse > > > > The debugfs take reference on fence without dropping them. Also the > > rcu section are not well balance. Fix all that ... > > > > Signed-off-by: Jérôme Glisse > > Cc: Christian König > > Cc: Daniel Vetter > > Cc: Sumit Semwal > > Cc: linux-media@vger.kernel.org > > Cc: dri-devel@lists.freedesktop.org > > Cc: linaro-mm-sig@lists.linaro.org > > Cc: Stéphane Marchesin > > Cc: stable@vger.kernel.org > > Well NAK, you are now taking the RCU lock twice and dropping the RCU and > still accessing fobj has a huge potential for accessing freed up memory. > > The only correct thing I can see here is to grab a reference to the > fence before printing any info on it, > Christian. Hu ? That is exactly what i am doing, take reference under rcu, rcu_unlock print the fence info, drop the fence reference, rcu lock rinse and repeat ... Note that the fobj in _existing_ code is access outside the rcu end that there is an rcu imbalance in that code ie a lonlely rcu_unlock after the for loop. So that the existing code is broken. > > > --- > > drivers/dma-buf/dma-buf.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > > index 13884474d158..f6f4de42ac49 100644 > > --- a/drivers/dma-buf/dma-buf.c > > +++ b/drivers/dma-buf/dma-buf.c > > @@ -1051,24 +1051,31 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) > > fobj = rcu_dereference(robj->fence); > > shared_count = fobj ? fobj->shared_count : 0; > > fence = rcu_dereference(robj->fence_excl); > > + fence = dma_fence_get_rcu(fence); > > if (!read_seqcount_retry(&robj->seq, seq)) > > break; > > rcu_read_unlock(); > > } > > - > > - if (fence) > > + if (fence) { > > seq_printf(s, "\tExclusive fence: %s %s %ssignalled\n", > > fence->ops->get_driver_name(fence), > > fence->ops->get_timeline_name(fence), > > dma_fence_is_signaled(fence) ? "" : "un"); > > + dma_fence_put(fence); > > + } > > + > > + rcu_read_lock(); > > for (i = 0; i < shared_count; i++) { > > fence = rcu_dereference(fobj->shared[i]); > > if (!dma_fence_get_rcu(fence)) > > continue; > > + rcu_read_unlock(); > > seq_printf(s, "\tShared fence: %s %s %ssignalled\n", > > fence->ops->get_driver_name(fence), > > fence->ops->get_timeline_name(fence), > > dma_fence_is_signaled(fence) ? "" : "un"); > > + dma_fence_put(fence); > > + rcu_read_lock(); > > } > > rcu_read_unlock(); > > > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 6 Dec 2018 10:21:17 -0500 From: Jerome Glisse To: "Koenig, Christian" Cc: "linux-kernel@vger.kernel.org" , Daniel Vetter , Sumit Semwal , "linux-media@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "linaro-mm-sig@lists.linaro.org" , =?iso-8859-1?Q?St=E9phane?= Marchesin , "stable@vger.kernel.org" Subject: Re: [PATCH] dma-buf: fix debugfs versus rcu and fence dumping Message-ID: <20181206152116.GA3544@redhat.com> References: <20181206014103.1364-1-jglisse@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-ID: On Thu, Dec 06, 2018 at 08:09:28AM +0000, Koenig, Christian wrote: > Am 06.12.18 um 02:41 schrieb jglisse@redhat.com: > > From: J�r�me Glisse > > > > The debugfs take reference on fence without dropping them. Also the > > rcu section are not well balance. Fix all that ... > > > > Signed-off-by: J�r�me Glisse > > Cc: Christian K�nig > > Cc: Daniel Vetter > > Cc: Sumit Semwal > > Cc: linux-media@vger.kernel.org > > Cc: dri-devel@lists.freedesktop.org > > Cc: linaro-mm-sig@lists.linaro.org > > Cc: St�phane Marchesin > > Cc: stable@vger.kernel.org > > Well NAK, you are now taking the RCU lock twice and dropping the RCU and > still accessing fobj has a huge potential for accessing freed up memory. > > The only correct thing I can see here is to grab a reference to the > fence before printing any info on it, > Christian. Hu ? That is exactly what i am doing, take reference under rcu, rcu_unlock print the fence info, drop the fence reference, rcu lock rinse and repeat ... Note that the fobj in _existing_ code is access outside the rcu end that there is an rcu imbalance in that code ie a lonlely rcu_unlock after the for loop. So that the existing code is broken. > > > --- > > drivers/dma-buf/dma-buf.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > > index 13884474d158..f6f4de42ac49 100644 > > --- a/drivers/dma-buf/dma-buf.c > > +++ b/drivers/dma-buf/dma-buf.c > > @@ -1051,24 +1051,31 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) > > fobj = rcu_dereference(robj->fence); > > shared_count = fobj ? fobj->shared_count : 0; > > fence = rcu_dereference(robj->fence_excl); > > + fence = dma_fence_get_rcu(fence); > > if (!read_seqcount_retry(&robj->seq, seq)) > > break; > > rcu_read_unlock(); > > } > > - > > - if (fence) > > + if (fence) { > > seq_printf(s, "\tExclusive fence: %s %s %ssignalled\n", > > fence->ops->get_driver_name(fence), > > fence->ops->get_timeline_name(fence), > > dma_fence_is_signaled(fence) ? "" : "un"); > > + dma_fence_put(fence); > > + } > > + > > + rcu_read_lock(); > > for (i = 0; i < shared_count; i++) { > > fence = rcu_dereference(fobj->shared[i]); > > if (!dma_fence_get_rcu(fence)) > > continue; > > + rcu_read_unlock(); > > seq_printf(s, "\tShared fence: %s %s %ssignalled\n", > > fence->ops->get_driver_name(fence), > > fence->ops->get_timeline_name(fence), > > dma_fence_is_signaled(fence) ? "" : "un"); > > + dma_fence_put(fence); > > + rcu_read_lock(); > > } > > rcu_read_unlock(); > > >