From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tvrtko Ursulin Subject: Re: [PATCH v2] drm/i915: Fix userptr deadlock with MAP_FIXED Date: Tue, 30 Jun 2015 15:52:52 +0100 Message-ID: <5592AD44.7080801@linux.intel.com> References: <1435576151-17803-1-git-send-email-chris@chris-wilson.co.uk> <1435576653-17973-1-git-send-email-chris@chris-wilson.co.uk> <20150629155713.GA29490@mwiniars-desk1.igk.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTP id CFF6E6E994 for ; Tue, 30 Jun 2015 07:52:54 -0700 (PDT) In-Reply-To: <20150629155713.GA29490@mwiniars-desk1.igk.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: =?UTF-8?B?TWljaGHFgiBXaW5pYXJza2k=?= , Chris Wilson Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org Ck9uIDA2LzI5LzIwMTUgMDQ6NTcgUE0sIE1pY2hhxYIgV2luaWFyc2tpIHdyb3RlOgo+IE9uIE1v biwgSnVuIDI5LCAyMDE1IGF0IDEyOjE3OjMzUE0gKzAxMDAsIENocmlzIFdpbHNvbiB3cm90ZToK Pj4gTWljaGHFgiBXaW5pYXJza2kgZm91bmQgYSByZWFsbHkgZXZpbCB3YXkgdG8gdHJpZ2dlciBh IHN0cnVjdF9tdXRleAo+PiBkZWFkbG9jayB3aXRoIHVzZXJwdHIuIEhlIGZvdW5kIHRoYXQgaWYg aGUgYWxsb2NhdGVkIGEgdXNlcnB0ciBibyBhbmQKPj4gdGhlbiBHVFQgbW1hcGVkIGFub3RoZXIg Ym8sIG9yIGV2ZW4gaXRzZWxmLCBhdCB0aGUgc2FtZSBhZGRyZXNzIGFzIHRoZQo+PiB1c2VycHRy IHVzaW5nIE1BUF9GSVhFRCwgaGUgY291bGQgdGhlbiBjYXVzZSBhIGRlYWRsb2NrIGFueSB0aW1l IHdlIHRoZW4KPj4gaGFkIHRvIGludmFsaWRhdGUgdGhlIEdUVCBtbWFwcGluZ3MgKHNvIGF0IHdp bGwpLgo+Pgo+PiBUbyBjb3VudGVyIGFjdCB0aGUgZGVhZGxvY2ssIHdlIG1ha2UgdGhlIG9ic2Vy dmF0aW9uIHRoYXQgd2hlbiB0aGUKPj4gTUFQX0ZJWEVEIGlzIG1hZGUgd2Ugd291bGQgaGF2ZSBh biBpbnZhbGlkYXRlX3JhbmdlIGV2ZW50IGZvciBvdXIKPj4gb2JqZWN0LiBBZnRlciB0aGF0IHdl IHNob3VsZCBubyBsb25nZXIgYWxpYXMgd2l0aCB0aGUgcm9ndWUgbW1hcHBpbmcuIElmCj4+IHdl IGFyZSB0aGVuIGFibGUgdG8gbWFyayB0aGUgb2JqZWN0IGFzIG5vIGxvbmdlciBpbiB1c2UgYWZ0 ZXIgdGhlIGZpcnN0Cj4+IGludmFsaWRhdGUsIHdlIGRvIG5vdCBuZWVkIHRvIGdyYWIgdGhlIHN0 cnVjdF9tdXRleCBmb3IgdGhlIHN1YnNlcXVlbnQKPj4gaW52YWxpZGF0aW9ucy4KPj4KPj4gVGhl IHBhdGNoIG1ha2VzIG9uZSBleWUtY2F0Y2hpbmcgY2hhbmdlLiBUaGF0IGlzIHRoZSByZW1vdmFs IHNlcmlhbD0wCj4+IGFmdGVyIGRldGVjdGluZyBhIHRvLWJlLWZyZWVkIG9iamVjdCBpbnNpZGUg dGhlIGludmFsaWRhdGUgd2Fsa2VyLiBJCj4+IGZlbHQgc2V0dGluZyBzZXJpYWw9MCB3YXMgYSBx dWVzdGlvbmFibGUgcGVzc2ltaXNhdGlvbjogaXQgZGVuaWVzIHVzIHRoZQo+PiBjaGFuY2UgdG8g cmV1c2UgdGhlIGN1cnJlbnQgaXRlcmF0b3IgZm9yIHRoZSBuZXh0IGxvb3AgKGJlZm9yZSBpdCBp cwo+PiBmcmVlZCkgYW5kIGJlaW5nIGV4cGxpY2l0IG1ha2VzIHRoZSByZWFkZXIgcXVlc3Rpb24g dGhlIHZhbGlkaXR5IG9mIHRoZQo+PiBsb2NraW5nIChzaW5jZSB0aGUgb2JqZWN0LWZyZWUgcmFj ZSBjb3VsZCBvY2N1ciBlbHNld2hlcmUpLiBUaGUKPj4gc2VyaWFsaXNhdGlvbiBvZiB0aGUgaXRl cmF0b3IgaXMgdGhyb3VnaCB0aGUgc3BpbmxvY2ssIGlmIHRoZSBvYmplY3QgaXMKPj4gZnJlZWQg YmVmb3JlIHRoZSBuZXh0IGxvb3AgdGhlbiB0aGUgbm90aWZpZXIuc2VyaWFsIHdpbGwgYmUgaW5j cmVtZW50ZWQKPj4gYW5kIHdlIHN0YXJ0IHRoZSB3YWxrIGZyb20gdGhlIGJlZ2lubmluZyBhcyB3 ZSBkZXRlY3QgdGhlIGludmFsaWQgY2FjaGUuCj4+Cj4+IHYyOiBHcmFtbWFyIGZpeGVzCj4+Cj4+ IFJlcG9ydGVkLWJ5OiBNaWNoYcWCIFdpbmlhcnNraSA8bWljaGFsLndpbmlhcnNraUBpbnRlbC5j b20+Cj4+IFRlc3RjYXNlOiBpZ3QvZ2VtX3VzZXJwdHJfYmxpdHMvbWFwLWZpeGVkKgo+PiBTaWdu ZWQtb2ZmLWJ5OiBDaHJpcyBXaWxzb24gPGNocmlzQGNocmlzLXdpbHNvbi5jby51az4KPj4gQ2M6 IE1pY2hhxYIgV2luaWFyc2tpIDxtaWNoYWwud2luaWFyc2tpQGludGVsLmNvbT4KPj4gQ2M6IFR2 cnRrbyBVcnN1bGluIDx0dnJ0a28udXJzdWxpbkBpbnRlbC5jb20+Cj4+IENjOiBzdGFibGVAdmdl ci5rZXJuZWwub3JnCj4+IC0tLQo+PiAgIGRyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVfZ2VtX3Vz ZXJwdHIuYyB8IDQzICsrKysrKysrKysrKysrKysrKysrKysrKysrKy0tLS0tLQo+PiAgIDEgZmls ZSBjaGFuZ2VkLCAzNiBpbnNlcnRpb25zKCspLCA3IGRlbGV0aW9ucygtKQo+Pgo+PiBkaWZmIC0t Z2l0IGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9nZW1fdXNlcnB0ci5jIGIvZHJpdmVycy9n cHUvZHJtL2k5MTUvaTkxNV9nZW1fdXNlcnB0ci5jCj4+IGluZGV4IGNiMzY3ZDlmNzkwOS4uZTE5 NjVkOGMwOGM4IDEwMDY0NAo+PiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X2dlbV91 c2VycHRyLmMKPj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9nZW1fdXNlcnB0ci5j Cj4+IEBAIC01OSw2ICs1OSw3IEBAIHN0cnVjdCBpOTE1X21tdV9vYmplY3Qgewo+PiAgIAlzdHJ1 Y3QgaW50ZXJ2YWxfdHJlZV9ub2RlIGl0Owo+PiAgIAlzdHJ1Y3QgbGlzdF9oZWFkIGxpbms7Cj4+ ICAgCXN0cnVjdCBkcm1faTkxNV9nZW1fb2JqZWN0ICpvYmo7Cj4+ICsJYm9vbCBhY3RpdmU7Cj4+ ICAgCWJvb2wgaXNfbGluZWFyOwo+PiAgIH07Cj4+Cj4+IEBAIC0xMTQsNyArMTE1LDggQEAgcmVz dGFydDoKPj4KPj4gICAJCW9iaiA9IG1vLT5vYmo7Cj4+Cj4+IC0JCWlmICgha3JlZl9nZXRfdW5s ZXNzX3plcm8oJm9iai0+YmFzZS5yZWZjb3VudCkpCj4+ICsJCWlmICghbW8tPmFjdGl2ZSB8fAo+ PiArCQkgICAgIWtyZWZfZ2V0X3VubGVzc196ZXJvKCZvYmotPmJhc2UucmVmY291bnQpKQo+PiAg IAkJCWNvbnRpbnVlOwo+Pgo+PiAgIAkJc3Bpbl91bmxvY2soJm1uLT5sb2NrKTsKPj4gQEAgLTE1 MSw3ICsxNTMsOCBAQCBzdGF0aWMgdm9pZCBpOTE1X2dlbV91c2VycHRyX21uX2ludmFsaWRhdGVf cmFuZ2Vfc3RhcnQoc3RydWN0IG1tdV9ub3RpZmllciAqX21uLAo+PiAgIAkJZWxzZQo+PiAgIAkJ CWl0ID0gaW50ZXJ2YWxfdHJlZV9pdGVyX2ZpcnN0KCZtbi0+b2JqZWN0cywgc3RhcnQsIGVuZCk7 Cj4+ICAgCQlpZiAoaXQgIT0gTlVMTCkgewo+PiAtCQkJb2JqID0gY29udGFpbmVyX29mKGl0LCBz dHJ1Y3QgaTkxNV9tbXVfb2JqZWN0LCBpdCktPm9iajsKPj4gKwkJCXN0cnVjdCBpOTE1X21tdV9v YmplY3QgKm1vID0KPj4gKwkJCQljb250YWluZXJfb2YoaXQsIHN0cnVjdCBpOTE1X21tdV9vYmpl Y3QsIGl0KTsKPj4KPj4gICAJCQkvKiBUaGUgbW11X29iamVjdCBpcyByZWxlYXNlZCBsYXRlIHdo ZW4gZGVzdHJveWluZyB0aGUKPj4gICAJCQkgKiBHRU0gb2JqZWN0IHNvIGl0IGlzIGVudGlyZWx5 IHBvc3NpYmxlIHRvIGdhaW4gYQo+PiBAQCAtMTYwLDExICsxNjMsOSBAQCBzdGF0aWMgdm9pZCBp OTE1X2dlbV91c2VycHRyX21uX2ludmFsaWRhdGVfcmFuZ2Vfc3RhcnQoc3RydWN0IG1tdV9ub3Rp ZmllciAqX21uLAo+PiAgIAkJCSAqIHRoZSBzdHJ1Y3RfbXV0ZXggLSBhbmQgY29uc2VxdWVudGx5 IHVzZSBpdCBhZnRlciBpdAo+PiAgIAkJCSAqIGlzIGZyZWVkIGFuZCB0aGVuIGRvdWJsZSBmcmVl IGl0Lgo+PiAgIAkJCSAqLwo+PiAtCQkJaWYgKCFrcmVmX2dldF91bmxlc3NfemVybygmb2JqLT5i YXNlLnJlZmNvdW50KSkgewo+PiAtCQkJCXNwaW5fdW5sb2NrKCZtbi0+bG9jayk7Cj4+IC0JCQkJ c2VyaWFsID0gMDsKPj4gLQkJCQljb250aW51ZTsKPj4gLQkJCX0KPj4gKwkJCWlmIChtby0+YWN0 aXZlICYmCj4+ICsJCQkgICAga3JlZl9nZXRfdW5sZXNzX3plcm8oJm1vLT5vYmotPmJhc2UucmVm Y291bnQpKQo+PiArCQkJCW9iaiA9IG1vLT5vYmo7Cj4+Cj4+ICAgCQkJc2VyaWFsID0gbW4tPnNl cmlhbDsKPj4gICAJCX0KPj4gQEAgLTYwNiw2ICs2MDcsMjAgQEAgX19pOTE1X2dlbV91c2VycHRy X2dldF9wYWdlc193b3JrZXIoc3RydWN0IHdvcmtfc3RydWN0ICpfd29yaykKPj4gICAJCXdha2Vf dXBfYWxsKCZ0b19pOTE1KGRldiktPm1tLnF1ZXVlKTsKPj4gICB9Cj4+Cj4+ICtzdGF0aWMgdm9p ZAo+PiArX19pOTE1X2dlbV91c2VycHRyX3NldF9hY3RpdmUoc3RydWN0IGRybV9pOTE1X2dlbV9v YmplY3QgKm9iaiwKPj4gKwkJCSAgICAgIGJvb2wgdmFsdWUpCj4+ICt7Cj4+ICsjaWYgZGVmaW5l ZChDT05GSUdfTU1VX05PVElGSUVSKQo+PiArCWlmIChvYmotPnVzZXJwdHIubW11X29iamVjdCA9 PSBOVUxMKQo+PiArCQlyZXR1cm47Cj4+ICsKPj4gKwlzcGluX2xvY2soJm9iai0+dXNlcnB0ci5t bXVfb2JqZWN0LT5tbi0+bG9jayk7Cj4+ICsJb2JqLT51c2VycHRyLm1tdV9vYmplY3QtPmFjdGl2 ZSA9IHZhbHVlOwo+PiArCXNwaW5fdW5sb2NrKCZvYmotPnVzZXJwdHIubW11X29iamVjdC0+bW4t PmxvY2spOwo+PiArI2VuZGlmCj4+ICt9Cj4+ICsKPj4gICBzdGF0aWMgaW50Cj4+ICAgaTkxNV9n ZW1fdXNlcnB0cl9nZXRfcGFnZXMoc3RydWN0IGRybV9pOTE1X2dlbV9vYmplY3QgKm9iaikKPj4g ICB7Cj4+IEBAIC02MTMsNiArNjI4LDE4IEBAIGk5MTVfZ2VtX3VzZXJwdHJfZ2V0X3BhZ2VzKHN0 cnVjdCBkcm1faTkxNV9nZW1fb2JqZWN0ICpvYmopCj4+ICAgCXN0cnVjdCBwYWdlICoqcHZlYzsK Pj4gICAJaW50IHBpbm5lZCwgcmV0Owo+Pgo+PiArCS8qIER1cmluZyBtbV9pbnZhbGlkYXRlX3Jh bmdlIHdlIG5lZWQgdG8gY2FuY2VsIGFueSB1c2VycHRyIHRoYXQKPj4gKwkgKiBvdmVybGFwcyB0 aGUgcmFuZ2UgYmVpbmcgaW52YWxpZGF0ZWQuIERvaW5nIHNvIHJlcXVpcmVzIHRoZQo+PiArCSAq IHN0cnVjdF9tdXRleCwgYW5kIHRoYXQgcmlza3MgcmVjdXJzaW9uLiBJbiBvcmRlciB0byBjYXVz ZQo+PiArCSAqIHJlY3Vyc2lvbiwgdGhlIHVzZXIgbXVzdCBhbGlhcyB0aGUgdXNlcnB0ciBhZGRy ZXNzIHNwYWNlIHdpdGgKPj4gKwkgKiBhIEdUVCBtbWFwcGluZyAocG9zc2libGUgd2l0aCBhIE1B UF9GSVhFRCkgLSB0aGVuIHdoZW4gd2UgaGF2ZQo+PiArCSAqIHRvIGludmFsaWRhdGUgdGhhdCBt bWFwaW5nLCBtbV9pbnZhbGlkYXRlX3JhbmdlIGlzIGNhbGxlZCB3aXRoCj4+ICsJICogdGhlIHVz ZXJwdHIgYWRkcmVzcyAqYW5kKiB0aGUgc3RydWN0X211dGV4IGhlbGQuICBUbyBwcmV2ZW50IHRo YXQKPj4gKwkgKiB3ZSBzZXQgYSBmbGFnIHVuZGVyIHRoZSBpOTE1X21tdV9ub3RpZmllciBzcGlu bG9jayB0byBpbmRpY2F0ZQo+PiArCSAqIHdoZXRoZXIgdGhpcyBvYmplY3QgaXMgdmFsaWQuCj4+ ICsJICovCj4+ICsJX19pOTE1X2dlbV91c2VycHRyX3NldF9hY3RpdmUob2JqLCB0cnVlKTsKPj4g Kwo+Cj4gVGhpcyB3aWxsIHNldCBtbXVfb2JqZWN0IHRvIGFjdGl2ZSBldmVuIGlmIHdlJ3JlIGV4 aXRpbmcgZWFybHkgZnJvbSBoZXJlCj4gKGJlY2F1c2Ugb2YgZXJyb3IgaGFuZGxpbmcpLCBjcmVh dGluZyBhbm90aGVyIHBvc3NpYmlsaXR5IGZvciBkZWFkbG9jay4KCkkgdGhpbmsgdGhpcyBkZWFk bG9jayBpcyByZXByb2R1Y2libGUgd2l0aG91dCBNQVBfRklYRUQsIHNvIGNvbW1pdCAKbWVzc2Fn ZSBzaG91bGQgYmUgcHJvYmFibHkgcmV3b3JkZWQgdG8gYWxsb3cgZm9yIHRoZSBtb3JlIGdlbmVy aWMgY2FzZS4KCkkgcmVwcm9kdWNlZCBpdCBsaWtlIHRoaXM6CgoxLiBtbWFwLCBnZW1fdXNlcnB0 ciwgbXVubWFwCjIuIENyZWF0ZSBhIG5vcm1hbCBiby4KMy4gTG9vcCBhIGJpdCBtbWFwcGluZyB0 aGUgYWJvdmUgdW50aWwgaXQgaGl0cyB0aGUgc2FtZSBhZGRyZXNzIGFzIHVzZXJwdHIuCjQuIFdy aXRlIHRvIHRoZSBCTyBtbWFwIHRvIHNldCBmYXVsdF9tYXBwYWJsZS4KNS4gc2V0X3RpbGluZyBv biBub3JtYWwgYm8gaGFuZGxlLgoKSSBhbSBzdGlsbCB0aGlua2luZyBhYm91dCB0aGlzIGFjdGl2 ZSBmbGFnIGluIHRoZSBhYm92ZSBzY2VuYXJpby4KCnVzZXJwdHItPmdldF9wYWdlcyBoYXNuJ3Qg YmVlbiBjYWxsZWQgYWJvdmUgc28gYWN0aXZlID09IGZhbHNlLiBJZiAKYmV0d2VlbiBzdGVwcyA0 IGFuZCA1IHdlIHRyaWdnZXIgZ2V0X3BhZ2VzLCB1c2VycHRyIHRyYW5zaXRpb25zIHRvIAphY3Rp dmUgYW5kIHNldF90aWxpbmcgZGVhZGxvY2tzLiBPciBJIHN0aWxsIG1pc3Npbmcgc29tZXRoaW5n PwoKUmVnYXJkcywKClR2cnRrbwpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fXwpJbnRlbC1nZnggbWFpbGluZyBsaXN0CkludGVsLWdmeEBsaXN0cy5mcmVlZGVz a3RvcC5vcmcKaHR0cDovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2lu dGVsLWdmeAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com ([192.55.52.88]:57889 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752120AbbF3OxO (ORCPT ); Tue, 30 Jun 2015 10:53:14 -0400 Message-ID: <5592AD44.7080801@linux.intel.com> Date: Tue, 30 Jun 2015 15:52:52 +0100 From: Tvrtko Ursulin MIME-Version: 1.0 To: =?UTF-8?B?TWljaGHFgiBXaW5pYXJza2k=?= , Chris Wilson CC: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: Fix userptr deadlock with MAP_FIXED References: <1435576151-17803-1-git-send-email-chris@chris-wilson.co.uk> <1435576653-17973-1-git-send-email-chris@chris-wilson.co.uk> <20150629155713.GA29490@mwiniars-desk1.igk.intel.com> In-Reply-To: <20150629155713.GA29490@mwiniars-desk1.igk.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: stable-owner@vger.kernel.org List-ID: On 06/29/2015 04:57 PM, Michał Winiarski wrote: > On Mon, Jun 29, 2015 at 12:17:33PM +0100, Chris Wilson wrote: >> Michał Winiarski found a really evil way to trigger a struct_mutex >> deadlock with userptr. He found that if he allocated a userptr bo and >> then GTT mmaped another bo, or even itself, at the same address as the >> userptr using MAP_FIXED, he could then cause a deadlock any time we then >> had to invalidate the GTT mmappings (so at will). >> >> To counter act the deadlock, we make the observation that when the >> MAP_FIXED is made we would have an invalidate_range event for our >> object. After that we should no longer alias with the rogue mmapping. If >> we are then able to mark the object as no longer in use after the first >> invalidate, we do not need to grab the struct_mutex for the subsequent >> invalidations. >> >> The patch makes one eye-catching change. That is the removal serial=0 >> after detecting a to-be-freed object inside the invalidate walker. I >> felt setting serial=0 was a questionable pessimisation: it denies us the >> chance to reuse the current iterator for the next loop (before it is >> freed) and being explicit makes the reader question the validity of the >> locking (since the object-free race could occur elsewhere). The >> serialisation of the iterator is through the spinlock, if the object is >> freed before the next loop then the notifier.serial will be incremented >> and we start the walk from the beginning as we detect the invalid cache. >> >> v2: Grammar fixes >> >> Reported-by: Michał Winiarski >> Testcase: igt/gem_userptr_blits/map-fixed* >> Signed-off-by: Chris Wilson >> Cc: Michał Winiarski >> Cc: Tvrtko Ursulin >> Cc: stable@vger.kernel.org >> --- >> drivers/gpu/drm/i915/i915_gem_userptr.c | 43 +++++++++++++++++++++++++++------ >> 1 file changed, 36 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c >> index cb367d9f7909..e1965d8c08c8 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c >> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c >> @@ -59,6 +59,7 @@ struct i915_mmu_object { >> struct interval_tree_node it; >> struct list_head link; >> struct drm_i915_gem_object *obj; >> + bool active; >> bool is_linear; >> }; >> >> @@ -114,7 +115,8 @@ restart: >> >> obj = mo->obj; >> >> - if (!kref_get_unless_zero(&obj->base.refcount)) >> + if (!mo->active || >> + !kref_get_unless_zero(&obj->base.refcount)) >> continue; >> >> spin_unlock(&mn->lock); >> @@ -151,7 +153,8 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, >> else >> it = interval_tree_iter_first(&mn->objects, start, end); >> if (it != NULL) { >> - obj = container_of(it, struct i915_mmu_object, it)->obj; >> + struct i915_mmu_object *mo = >> + container_of(it, struct i915_mmu_object, it); >> >> /* The mmu_object is released late when destroying the >> * GEM object so it is entirely possible to gain a >> @@ -160,11 +163,9 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn, >> * the struct_mutex - and consequently use it after it >> * is freed and then double free it. >> */ >> - if (!kref_get_unless_zero(&obj->base.refcount)) { >> - spin_unlock(&mn->lock); >> - serial = 0; >> - continue; >> - } >> + if (mo->active && >> + kref_get_unless_zero(&mo->obj->base.refcount)) >> + obj = mo->obj; >> >> serial = mn->serial; >> } >> @@ -606,6 +607,20 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) >> wake_up_all(&to_i915(dev)->mm.queue); >> } >> >> +static void >> +__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, >> + bool value) >> +{ >> +#if defined(CONFIG_MMU_NOTIFIER) >> + if (obj->userptr.mmu_object == NULL) >> + return; >> + >> + spin_lock(&obj->userptr.mmu_object->mn->lock); >> + obj->userptr.mmu_object->active = value; >> + spin_unlock(&obj->userptr.mmu_object->mn->lock); >> +#endif >> +} >> + >> static int >> i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) >> { >> @@ -613,6 +628,18 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj) >> struct page **pvec; >> int pinned, ret; >> >> + /* During mm_invalidate_range we need to cancel any userptr that >> + * overlaps the range being invalidated. Doing so requires the >> + * struct_mutex, and that risks recursion. In order to cause >> + * recursion, the user must alias the userptr address space with >> + * a GTT mmapping (possible with a MAP_FIXED) - then when we have >> + * to invalidate that mmaping, mm_invalidate_range is called with >> + * the userptr address *and* the struct_mutex held. To prevent that >> + * we set a flag under the i915_mmu_notifier spinlock to indicate >> + * whether this object is valid. >> + */ >> + __i915_gem_userptr_set_active(obj, true); >> + > > This will set mmu_object to active even if we're exiting early from here > (because of error handling), creating another possibility for deadlock. I think this deadlock is reproducible without MAP_FIXED, so commit message should be probably reworded to allow for the more generic case. I reproduced it like this: 1. mmap, gem_userptr, munmap 2. Create a normal bo. 3. Loop a bit mmapping the above until it hits the same address as userptr. 4. Write to the BO mmap to set fault_mappable. 5. set_tiling on normal bo handle. I am still thinking about this active flag in the above scenario. userptr->get_pages hasn't been called above so active == false. If between steps 4 and 5 we trigger get_pages, userptr transitions to active and set_tiling deadlocks. Or I still missing something? Regards, Tvrtko