From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sudip Mukherjee Subject: Re: [PATCH] drm/gma500: fix double freeing Date: Fri, 2 Oct 2015 21:26:41 +0530 Message-ID: <20151002155641.GA16809@sudip-pc> References: <1441803040-15998-1-git-send-email-sudipm.mukherjee@gmail.com> <20150924155725.GE10109@sudip-pc> <20150930061241.GC3500@sudip-pc> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail-pa0-f52.google.com (mail-pa0-f52.google.com [209.85.220.52]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8E5456E2F6 for ; Fri, 2 Oct 2015 08:56:53 -0700 (PDT) Received: by pacex6 with SMTP id ex6so110083261pac.0 for ; Fri, 02 Oct 2015 08:56:53 -0700 (PDT) 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: Patrik Jakobsson Cc: Daniel Vetter , linux-kernel , dri-devel List-Id: dri-devel@lists.freedesktop.org T24gVGh1LCBPY3QgMDEsIDIwMTUgYXQgMDc6MDc6MzNQTSArMDIwMCwgUGF0cmlrIEpha29ic3Nv biB3cm90ZToKPiBPbiBXZWQsIFNlcCAzMCwgMjAxNSBhdCA4OjEyIEFNLCBTdWRpcCBNdWtoZXJq ZWUKPiA8c3VkaXBtLm11a2hlcmplZUBnbWFpbC5jb20+IHdyb3RlOgo+ID4gT24gVHVlLCBTZXAg MjksIDIwMTUgYXQgMDM6MjA6MzVQTSArMDIwMCwgUGF0cmlrIEpha29ic3NvbiB3cm90ZToKPiA+ PiBPbiBUaHUsIFNlcCAyNCwgMjAxNSBhdCA1OjU3IFBNLCBTdWRpcCBNdWtoZXJqZWUKPiA+PiA8 c3VkaXBtLm11a2hlcmplZUBnbWFpbC5jb20+IHdyb3RlOgo+ID4+ID4gT24gV2VkLCBTZXAgMDks IDIwMTUgYXQgMDY6MjA6NDBQTSArMDUzMCwgU3VkaXAgTXVraGVyamVlIHdyb3RlOgo+ID4+ID4+ IElmIGJhY2tpbmctPnN0b2xlbiBpcyB0cnVlIHRoZW4gd2Ugd2VyZSBmcmVlaW5nIGJhY2tpbmcg YnkgY2FsbGluZwo+ID4+ID4+IHBzYl9ndHRfZnJlZV9yYW5nZSgpIGJ1dCB3ZSBjYWxsZWQgaXQg YWdhaW4gYWZ0ZXIgdW5sb2NraW5nIHRoZSBtdXRleC4KPiA+PiA+PiBMZXRzIG1ha2UgaXQgTlVM TCBhZnRlciBmcmVlaW5nIGluIHBzYl9ndHRfZnJlZV9yYW5nZSgpIGFuZCBjaGVjayBmb3IKPiA+ PiA+PiBOVUxMIGJlZm9yZSBjYWxsaW5nIHRoZSBmdW5jdGlvbiBmb3IgdGhlIHNlY29uZCB0aW1l Lgo+ID4+ID4+Cj4gPj4gPj4gU2lnbmVkLW9mZi1ieTogU3VkaXAgTXVraGVyamVlIDxzdWRpcEB2 ZWN0b3JpbmRpYS5vcmc+Cj4gPj4gPj4gLS0tCj4gPj4gPiBIaSBQYXRyaWssCj4gPj4gPiBBIGdl bnRsZSBwaW5nLgo+ID4+ID4KPiA+PiA+IHJlZ2FyZHMKPiA+PiA+IHN1ZGlwCj4gPj4KPiA+PiBI aSwgc29ycnkgZm9yIHRoZSBsYXRlIHJlcGx5Lgo+ID4+Cj4gPj4gV2h5IGFyZSB3ZSBmcmVlaW5n IHRoZSByYW5nZSB0d2ljZSBpbiB0aGUgZmlyc3QgY2FzZT8KPiA+IEkgdGhpbmssCj4gPiBpZiBi YWNraW5nLT5zdG9sZW4gaXMgdHJ1ZSB0aGVuIGJhY2tpbmcgaXMgcmVsZWFzZWQgdXNpbmcKPiA+ IHBzYl9ndHRfZnJlZV9yYW5nZSgpIGJ1dCBpZiBiYWNraW5nLT5zdG9sZW4gaXMgZmFsc2UgdGhl biB0aGUgZ2VtIG9iamVjdAo+ID4gaXMgZnJlZWQgYnV0IHRoZSBiYWNraW5nIGlzIG5vdCB5ZXQg ZnJlZWQuIFRvIGZyZWUgdGhhdCBiYWNraW5nCj4gPiBwc2JfZ3R0X2ZyZWVfcmFuZ2UoKSBoYXMg YmVlbiBjYWxsZWQgc2Vjb25kIHRpbWUuIE15IHBhdGNoIHRyaWVkIHRvIGZpeAo+ID4gdGhlIHBv c3NpYmlsaXR5IG9mIGJhY2tpbmctPnN0b2xlbiBiZWluZyB0cnVlIGFuZCBiYWNraW5nIGJlaW5n IGZyZWVkIDIKPiA+IHRpbWVzLgo+ID4KPiA+IHJlZ2FyZHMKPiA+IHN1ZGlwCj4gCj4gVGhlcmUg YXJlIHNvbWUgc3BlY2lhbCBoYW5kbGluZyBvZiB0aGUgc3RvbGVuIGZyYW1lYnVmZmVyIHRoYXQg SSBkb24ndAo+IHJlbWVtYmVyIGVudGlyZWx5IGJ1dCB0aGUgYmFzaWMgY29uY2VwdCBpcyB0aGF0 IHdlIGZyZWUgdGhlIGJhY2tpbmcKPiB3aGVuIHdlIGRyb3AgdGhlIGxhc3QgcmVmZXJlbmNlIG9u IGEgZ2VtIG9iamVjdC4gVGhhdCB3aWxsIHRyaWdnZXIgYQo+IHBzYl9ndHRfZnJlZV9yYW5nZSgp LiBTbyBpbiB0aGlzIGNhc2UgaXQgbG9va3MgdG8gbWUgdGhhdCB0aGUgZXh0cmEKPiBmcmVlIGlz IG5vdCBuZWVkZWQgYXQgYWxsLiBUaGF0J3MgbXkgcXVpY2sgcmVhc29uaW5nLCBmZWVsIGZyZWUg dG8KPiBwcm92ZSBtZSB3cm9uZyA6KQoKSW4gdGhpcyBjYXNlIHdlIGFyZSBhbGxvY2F0aW5nIGJh Y2tpbmcgdXNpbmcgcHNiZmJfYWxsb2MoKSBhbmQgc28KYmFja2luZy0+c3RvbGVuIGlzIGFsd2F5 cyB0cnVlLiBTbyB3ZSBjYW4gcmVtb3ZlIHRoZSBiYWNraW5nLT5zdG9sZW4KY29uZGl0aW9uLiBB bmQgaWYgZHJtX2ZiX2hlbHBlcl9hbGxvY19mYmkoKSBmYWlscyB0aGVuIHdlCmFyZSBqdW1waW5n IHRvIG91dF9lcnIxLiBTbyB0aGUgZml0c3QgZnJlZSB3aWxsIG5vdCBiZSBuZWVkZWQuCgpkaWZm IC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2dtYTUwMC9mcmFtZWJ1ZmZlci5jIGIvZHJpdmVycy9n cHUvZHJtL2dtYTUwMC9mcmFtZWJ1ZmZlci5jCmluZGV4IDJlYWYxYjMuLjkzMmYwN2IgMTAwNjQ0 Ci0tLSBhL2RyaXZlcnMvZ3B1L2RybS9nbWE1MDAvZnJhbWVidWZmZXIuYworKysgYi9kcml2ZXJz L2dwdS9kcm0vZ21hNTAwL2ZyYW1lYnVmZmVyLmMKQEAgLTQ2NiwxMSArNDY2LDYgQEAgc3RhdGlj IGludCBwc2JmYl9jcmVhdGUoc3RydWN0IHBzYl9mYmRldiAqZmJkZXYsCiAJbXV0ZXhfdW5sb2Nr KCZkZXYtPnN0cnVjdF9tdXRleCk7CiAJcmV0dXJuIDA7CiBvdXRfdW5yZWY6Ci0JaWYgKGJhY2tp bmctPnN0b2xlbikKLQkJcHNiX2d0dF9mcmVlX3JhbmdlKGRldiwgYmFja2luZyk7Ci0JZWxzZQot CQlkcm1fZ2VtX29iamVjdF91bnJlZmVyZW5jZSgmYmFja2luZy0+Z2VtKTsKLQogCWRybV9mYl9o ZWxwZXJfcmVsZWFzZV9mYmkoJmZiZGV2LT5wc2JfZmJfaGVscGVyKTsKIG91dF9lcnIxOgogCW11 dGV4X3VubG9jaygmZGV2LT5zdHJ1Y3RfbXV0ZXgpOwoKCklmIGl0IGlzIG9rLCBJIGNhbiBzdWJt aXQgdGhlIHYyLgoKcmVnYXJkcwpzdWRpcApfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5m cmVlZGVza3RvcC5vcmcKaHR0cDovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3Rp bmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752621AbbJBP4y (ORCPT ); Fri, 2 Oct 2015 11:56:54 -0400 Received: from mail-pa0-f53.google.com ([209.85.220.53]:36020 "EHLO mail-pa0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752080AbbJBP4x (ORCPT ); Fri, 2 Oct 2015 11:56:53 -0400 Date: Fri, 2 Oct 2015 21:26:41 +0530 From: Sudip Mukherjee To: Patrik Jakobsson Cc: David Airlie , Daniel Vetter , linux-kernel , dri-devel Subject: Re: [PATCH] drm/gma500: fix double freeing Message-ID: <20151002155641.GA16809@sudip-pc> References: <1441803040-15998-1-git-send-email-sudipm.mukherjee@gmail.com> <20150924155725.GE10109@sudip-pc> <20150930061241.GC3500@sudip-pc> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Oct 01, 2015 at 07:07:33PM +0200, Patrik Jakobsson wrote: > On Wed, Sep 30, 2015 at 8:12 AM, Sudip Mukherjee > wrote: > > On Tue, Sep 29, 2015 at 03:20:35PM +0200, Patrik Jakobsson wrote: > >> On Thu, Sep 24, 2015 at 5:57 PM, Sudip Mukherjee > >> wrote: > >> > On Wed, Sep 09, 2015 at 06:20:40PM +0530, Sudip Mukherjee wrote: > >> >> If backing->stolen is true then we were freeing backing by calling > >> >> psb_gtt_free_range() but we called it again after unlocking the mutex. > >> >> Lets make it NULL after freeing in psb_gtt_free_range() and check for > >> >> NULL before calling the function for the second time. > >> >> > >> >> Signed-off-by: Sudip Mukherjee > >> >> --- > >> > Hi Patrik, > >> > A gentle ping. > >> > > >> > regards > >> > sudip > >> > >> Hi, sorry for the late reply. > >> > >> Why are we freeing the range twice in the first case? > > I think, > > if backing->stolen is true then backing is released using > > psb_gtt_free_range() but if backing->stolen is false then the gem object > > is freed but the backing is not yet freed. To free that backing > > psb_gtt_free_range() has been called second time. My patch tried to fix > > the possibility of backing->stolen being true and backing being freed 2 > > times. > > > > regards > > sudip > > There are some special handling of the stolen framebuffer that I don't > remember entirely but the basic concept is that we free the backing > when we drop the last reference on a gem object. That will trigger a > psb_gtt_free_range(). So in this case it looks to me that the extra > free is not needed at all. That's my quick reasoning, feel free to > prove me wrong :) In this case we are allocating backing using psbfb_alloc() and so backing->stolen is always true. So we can remove the backing->stolen condition. And if drm_fb_helper_alloc_fbi() fails then we are jumping to out_err1. So the fitst free will not be needed. diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index 2eaf1b3..932f07b 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -466,11 +466,6 @@ static int psbfb_create(struct psb_fbdev *fbdev, mutex_unlock(&dev->struct_mutex); return 0; out_unref: - if (backing->stolen) - psb_gtt_free_range(dev, backing); - else - drm_gem_object_unreference(&backing->gem); - drm_fb_helper_release_fbi(&fbdev->psb_fb_helper); out_err1: mutex_unlock(&dev->struct_mutex); If it is ok, I can submit the v2. regards sudip