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.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_2 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 3E626C2D0B1 for ; Fri, 6 Dec 2019 07:53:34 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1E8DC2467A for ; Fri, 6 Dec 2019 07:53:33 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1E8DC2467A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=collabora.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 79EB16EA56; Fri, 6 Dec 2019 07:53:33 +0000 (UTC) Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3]) by gabe.freedesktop.org (Postfix) with ESMTPS id C83996EA56 for ; Fri, 6 Dec 2019 07:53:31 +0000 (UTC) Received: from localhost (unknown [IPv6:2a01:e0a:2c:6930:5cf4:84a1:2763:fe0d]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: bbrezillon) by bhuna.collabora.co.uk (Postfix) with ESMTPSA id 73DD229256E; Fri, 6 Dec 2019 07:53:30 +0000 (GMT) Date: Fri, 6 Dec 2019 08:53:27 +0100 From: Boris Brezillon To: Rob Herring Subject: Re: [PATCH 2/8] drm/panfrost: Fix a race in panfrost_ioctl_madvise() Message-ID: <20191206085327.66a8c479@collabora.com> In-Reply-To: References: <20191129135908.2439529-1-boris.brezillon@collabora.com> <20191129135908.2439529-3-boris.brezillon@collabora.com> <20191129153310.2f9c80e1@collabora.com> Organization: Collabora X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: stable , dri-devel , Alyssa Rosenzweig , Steven Price Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" T24gVGh1LCA1IERlYyAyMDE5IDE3OjA4OjAyIC0wNjAwClJvYiBIZXJyaW5nIDxyb2JoK2R0QGtl cm5lbC5vcmc+IHdyb3RlOgoKPiBPbiBGcmksIE5vdiAyOSwgMjAxOSBhdCA4OjMzIEFNIEJvcmlz IEJyZXppbGxvbgo+IDxib3Jpcy5icmV6aWxsb25AY29sbGFib3JhLmNvbT4gd3JvdGU6Cj4gPgo+ ID4gT24gRnJpLCAyOSBOb3YgMjAxOSAxNDoyNDo0OCArMDAwMAo+ID4gU3RldmVuIFByaWNlIDxz dGV2ZW4ucHJpY2VAYXJtLmNvbT4gd3JvdGU6Cj4gPiAgCj4gPiA+IE9uIDI5LzExLzIwMTkgMTM6 NTksIEJvcmlzIEJyZXppbGxvbiB3cm90ZTogIAo+ID4gPiA+IElmIDIgdGhyZWFkcyBjaGFuZ2Ug dGhlIE1BRFZJU0UgcHJvcGVydHkgb2YgdGhlIHNhbWUgQk8gaW4gcGFyYWxsZWwgd2UKPiA+ID4g PiBtaWdodCBlbmQgdXAgd2l0aCBhbiBzaG1lbS0+bWFkdiB2YWx1ZSB0aGF0J3MgaW5jb25zaXN0 ZW50IHdpdGggdGhlCj4gPiA+ID4gcHJlc2VuY2Ugb2YgdGhlIEJPIGluIHRoZSBzaHJpbmtlciBs aXN0LiAgCj4gPiA+Cj4gPiA+IEknbSBhIGJpdCB3b3JyaWVkIGZyb20gdGhlIHBvaW50IG9mIHZp ZXcgb2YgdXNlciBzcGFjZSBzYW5pdHkgdGhhdCB5b3UKPiA+ID4gb2JzZXJ2ZWQgdGhpcyAtIGJ1 dCBjbGVhcmx5IHRoZSBrZXJuZWwgc2hvdWxkIGJlIHJvYnVzdCEgIAo+ID4KPiA+IEl0J3Mgbm90 IHNvbWV0aGluZyBJIG9ic2VydmVkLCBqdXN0IGZvdW5kIHRoZSByYWNlIGJ5IGluc3BlY3Rpbmcg dGhlCj4gPiBjb2RlLCBhbmQgSSB0aG91Z2h0IGl0IHdhcyB3b3J0aCBmaXhpbmcgaXQuICAKPiAK PiBJJ20gbm90IHNvIHN1cmUgdGhlcmUncyBhIHJhY2UuCgpJJ20gcHJldHR5IHN1cmUgdGhlcmUn cyBvbmU6CgpUMAkJCQlUMQoKbG9jayhwYWdlcykKbWFkdiA9IDEKdW5sb2NrKHBhZ2VzKQoKCQkJ CWxvY2socGFnZXMpCgkJCQltYWR2ID0gMAoJCQkJdW5sb2NrKHBhZ2VzKQoKCQkJCWxvY2soc2hy aW5rZXIpCgkJCQlyZW1vdmVfZnJvbV9saXN0KGJvKQoJCQkJdW5sb2NrKHNocmlua2VyKQoKbG9j ayhzaHJpbmtlcikKYWRkX3RvX2xpc3QoYm8pCnVubG9jayhzaHJpbmtlcikKCllvdSBlbmQgdXAg d2l0aCBtYWR2ID0gMCBhbmQgdGhlIEJPIGlzIGFkZGVkIHRvIHRoZSBsaXN0LgoKPiBJZiB0aGVy ZSBpcywgd2Ugc3RpbGwgY2hlY2sgbWFkdiB2YWx1ZQo+IHdoZW4gcHVyZ2luZywgc28gaXQgd291 bGQgYmUgaGFybWxlc3MgZXZlbiBpZiB0aGUgc3RhdGUgaXMKPiBpbmNvbnNpc3RlbnQuCgpJbmRl ZWQuIE5vdGUgdGhhdCB5b3UgY291bGQgYWxzbyBoYXZlIHRoaXMgb3RoZXIgc2l0dWF0aW9uIHdo ZXJlIHRoZSBCTwppcyBtYXJrZWQgcHVyZ2VhYmxlIGJ1dCBub3QgcHJlc2VudCBpbiB0aGUgbGlz dC4gSW4gdGhhdCBjYXNlIGl0IHdpbGwKbmV2ZXIgYmUgcHVyZ2VkLCBidXQgaXQncyBraW5kYSB1 c2VyIHNwYWNlIGZhdWx0IGFueXdheS4gSSBhZ3JlZSwgbm9uZQpvZiB0aGlzIHByb2JsZW1zIGFy ZSBjcml0aWNhbCwgYW5kIEknbSBmaW5lIGxlYXZpbmcgaXQgdW5maXhlZCBhcyBsb25nCmFzIGl0 J3MgZG9jdW1lbnRlZCBzb21ld2hlcmUgdGhhdCB0aGUgcmFjZSBleGlzdCBhbmQgaXMgaGFybWxl c3MuCgo+IAo+ID4gPiA+IFRoZSBlYXNpZXN0IHNvbHV0aW9uIHRvIGZpeCB0aGF0IGlzIHRvIHBy b3RlY3QgdGhlCj4gPiA+ID4gZHJtX2dlbV9zaG1lbV9tYWR2aXNlKCkgY2FsbCB3aXRoIHRoZSBz aHJpbmtlciBsb2NrLgo+ID4gPiA+Cj4gPiA+ID4gRml4ZXM6IDAxM2I2NTEwMTMxNSAoImRybS9w YW5mcm9zdDogQWRkIG1hZHZpc2UgYW5kIHNocmlua2VyIHN1cHBvcnQiKQo+ID4gPiA+IENjOiA8 c3RhYmxlQHZnZXIua2VybmVsLm9yZz4KPiA+ID4gPiBTaWduZWQtb2ZmLWJ5OiBCb3JpcyBCcmV6 aWxsb24gPGJvcmlzLmJyZXppbGxvbkBjb2xsYWJvcmEuY29tPiAgCj4gPiA+Cj4gPiA+IFJldmll d2VkLWJ5OiBTdGV2ZW4gUHJpY2UgPHN0ZXZlbi5wcmljZUBhcm0uY29tPiAgCj4gPgo+ID4gVGhh bmtzLgo+ID4gIAo+ID4gPiAgCj4gPiA+ID4gLS0tCj4gPiA+ID4gIGRyaXZlcnMvZ3B1L2RybS9w YW5mcm9zdC9wYW5mcm9zdF9kcnYuYyB8IDkgKysrKy0tLS0tCj4gPiA+ID4gIDEgZmlsZSBjaGFu Z2VkLCA0IGluc2VydGlvbnMoKyksIDUgZGVsZXRpb25zKC0pCj4gPiA+ID4KPiA+ID4gPiBkaWZm IC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL3BhbmZyb3N0L3BhbmZyb3N0X2Rydi5jIGIvZHJpdmVy cy9ncHUvZHJtL3BhbmZyb3N0L3BhbmZyb3N0X2Rydi5jCj4gPiA+ID4gaW5kZXggZjIxYmM4YTdl ZTNhLi5lZmMwYTI0ZDFmNGMgMTAwNjQ0Cj4gPiA+ID4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL3Bh bmZyb3N0L3BhbmZyb3N0X2Rydi5jCj4gPiA+ID4gKysrIGIvZHJpdmVycy9ncHUvZHJtL3BhbmZy b3N0L3BhbmZyb3N0X2Rydi5jCj4gPiA+ID4gQEAgLTM0NywyMCArMzQ3LDE5IEBAIHN0YXRpYyBp bnQgcGFuZnJvc3RfaW9jdGxfbWFkdmlzZShzdHJ1Y3QgZHJtX2RldmljZSAqZGV2LCB2b2lkICpk YXRhLAo+ID4gPiA+ICAgICAgICAgICAgIHJldHVybiAtRU5PRU5UOwo+ID4gPiA+ICAgICB9Cj4g PiA+ID4KPiA+ID4gPiArICAgbXV0ZXhfbG9jaygmcGZkZXYtPnNocmlua2VyX2xvY2spOwo+ID4g PiA+ICAgICBhcmdzLT5yZXRhaW5lZCA9IGRybV9nZW1fc2htZW1fbWFkdmlzZShnZW1fb2JqLCBh cmdzLT5tYWR2KTsgIAo+IAo+IFRoaXMgbWVhbnMgd2Ugbm93IGhvbGQgdGhlIHNocmlua2VyX2xv Y2sgd2hpbGUgd2UgdGFrZSB0aGUgcGFnZXNfbG9jay4KPiBJcyBsb2NrZGVwIGhhcHB5IHdpdGgg dGhpcyBjaGFuZ2U/IEkgc3VzcGVjdCBub3QgZ2l2ZW4gYWxsIHRoZSBmdW4gSQo+IGhhZCBnZXR0 aW5nIGxvY2tkZXAgaGFwcHkuCgpJIGhhdmUgdGVzdGVkIHdpdGggbG9ja2RlcCBlbmFibGVkIGFu ZCBpdCdzIGFsbCBnb29kIGZyb20gbG9ja2RlcCBQb1YKYmVjYXVzZSB0aGUgbG9ja3MgYXJlIHRh a2VuIGluIHRoZSBzYW1lIG9yZGVyIGluIHRoZSBtYWR2aXNlKCkgYW5kCnNjaGlua2VyX3NjYW4o KSBwYXRoIChmaXJzdCB0aGUgc2hyaW5rZXIgbG9jaywgdGhlbiB0aGUgcGFnZXMgbG9jaykuCgpO b3RlIHRoYXQgcGF0Y2ggNyBpbnRyb2R1Y2VzIGEgZGVhZGxvY2sgaW4gdGhlIHNocmlua2VyIHBh dGgsIGJ1dCB0aGlzCmlzIHVucmVsYXRlZCB0byB0aGlzIHNocmlua2VyIGxvY2sgYmVpbmcgdGFr ZW4gZWFybGllciBpbiBtYWR2aXNlCihkcm1fZ2VtX3B1dF9wYWdlcygpIGlzIGNhbGxlZCB3aGls ZSB0aGUgcGFnZXMgbG9jayBpcyBhbHJlYWR5IGhlbGQpLgpfX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZl bEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFp bG1hbi9saXN0aW5mby9kcmktZGV2ZWw= 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.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_2 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 55F19C43603 for ; Fri, 6 Dec 2019 07:53:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3142F2467B for ; Fri, 6 Dec 2019 07:53:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726509AbfLFHxc (ORCPT ); Fri, 6 Dec 2019 02:53:32 -0500 Received: from bhuna.collabora.co.uk ([46.235.227.227]:37260 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726184AbfLFHxc (ORCPT ); Fri, 6 Dec 2019 02:53:32 -0500 Received: from localhost (unknown [IPv6:2a01:e0a:2c:6930:5cf4:84a1:2763:fe0d]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: bbrezillon) by bhuna.collabora.co.uk (Postfix) with ESMTPSA id 73DD229256E; Fri, 6 Dec 2019 07:53:30 +0000 (GMT) Date: Fri, 6 Dec 2019 08:53:27 +0100 From: Boris Brezillon To: Rob Herring Cc: Steven Price , Tomeu Vizoso , Alyssa Rosenzweig , stable , dri-devel Subject: Re: [PATCH 2/8] drm/panfrost: Fix a race in panfrost_ioctl_madvise() Message-ID: <20191206085327.66a8c479@collabora.com> In-Reply-To: References: <20191129135908.2439529-1-boris.brezillon@collabora.com> <20191129135908.2439529-3-boris.brezillon@collabora.com> <20191129153310.2f9c80e1@collabora.com> Organization: Collabora X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org On Thu, 5 Dec 2019 17:08:02 -0600 Rob Herring wrote: > On Fri, Nov 29, 2019 at 8:33 AM Boris Brezillon > wrote: > > > > On Fri, 29 Nov 2019 14:24:48 +0000 > > Steven Price wrote: > > > > > On 29/11/2019 13:59, Boris Brezillon wrote: > > > > If 2 threads change the MADVISE property of the same BO in parallel we > > > > might end up with an shmem->madv value that's inconsistent with the > > > > presence of the BO in the shrinker list. > > > > > > I'm a bit worried from the point of view of user space sanity that you > > > observed this - but clearly the kernel should be robust! > > > > It's not something I observed, just found the race by inspecting the > > code, and I thought it was worth fixing it. > > I'm not so sure there's a race. I'm pretty sure there's one: T0 T1 lock(pages) madv = 1 unlock(pages) lock(pages) madv = 0 unlock(pages) lock(shrinker) remove_from_list(bo) unlock(shrinker) lock(shrinker) add_to_list(bo) unlock(shrinker) You end up with madv = 0 and the BO is added to the list. > If there is, we still check madv value > when purging, so it would be harmless even if the state is > inconsistent. Indeed. Note that you could also have this other situation where the BO is marked purgeable but not present in the list. In that case it will never be purged, but it's kinda user space fault anyway. I agree, none of this problems are critical, and I'm fine leaving it unfixed as long as it's documented somewhere that the race exist and is harmless. > > > > > The easiest solution to fix that is to protect the > > > > drm_gem_shmem_madvise() call with the shrinker lock. > > > > > > > > Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support") > > > > Cc: > > > > Signed-off-by: Boris Brezillon > > > > > > Reviewed-by: Steven Price > > > > Thanks. > > > > > > > > > --- > > > > drivers/gpu/drm/panfrost/panfrost_drv.c | 9 ++++----- > > > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c > > > > index f21bc8a7ee3a..efc0a24d1f4c 100644 > > > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > > > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > > > > @@ -347,20 +347,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data, > > > > return -ENOENT; > > > > } > > > > > > > > + mutex_lock(&pfdev->shrinker_lock); > > > > args->retained = drm_gem_shmem_madvise(gem_obj, args->madv); > > This means we now hold the shrinker_lock while we take the pages_lock. > Is lockdep happy with this change? I suspect not given all the fun I > had getting lockdep happy. I have tested with lockdep enabled and it's all good from lockdep PoV because the locks are taken in the same order in the madvise() and schinker_scan() path (first the shrinker lock, then the pages lock). Note that patch 7 introduces a deadlock in the shrinker path, but this is unrelated to this shrinker lock being taken earlier in madvise (drm_gem_put_pages() is called while the pages lock is already held).