From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/prime: keep a reference from the handle to exported dma-buf (v2) Date: Thu, 20 Dec 2012 15:36:43 +0100 Message-ID: <20121220143643.GG5737@phenom.ffwll.local> References: <1355974747-24271-1-git-send-email-airlied@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f44.google.com (mail-ee0-f44.google.com [74.125.83.44]) by gabe.freedesktop.org (Postfix) with ESMTP id CF1A8E5EF9 for ; Thu, 20 Dec 2012 06:35:11 -0800 (PST) Received: by mail-ee0-f44.google.com with SMTP id b47so1761356eek.17 for ; Thu, 20 Dec 2012 06:35:10 -0800 (PST) Content-Disposition: inline In-Reply-To: <1355974747-24271-1-git-send-email-airlied@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Dave Airlie Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Thu, Dec 20, 2012 at 01:39:07PM +1000, Dave Airlie wrote: > From: Dave Airlie > > This is likely the simplest solution to the problem, and seems > to work fine. > > When we export a dma_buf from a gem object, we keep track of it > with the handle, we take a reference to the dma_buf. When we close > the handle (i.e. userspace is finished with the buffer), we drop > the reference to the dma_buf, and it gets collected. > > I'd like to refrain from too much in this patch as I'd like it to > go to stable, so future cleanups should look at maybe reducing > the obj storing two pointers etc. > > v1.1: move export symbol line back up. > > v2: okay I had to do a bit more, as the first patch showed a leak > on one of my tests, that I found using the dma-buf debugfs support, > the problem case is exporting a buffer twice with the same handle, > we'd add another export handle for it unnecessarily, however > we now fail if we try to export the same object with a different gem handle, > however I'm not sure if that is a case I want to support, and I've > gotten the code to WARN_ON if we hit something like that. > > Signed-off-by: Dave Airlie [snip] > -void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf) > +static void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf) > { > struct drm_prime_member *member, *safe; > > @@ -349,4 +380,16 @@ void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_f > } > mutex_unlock(&prime_fpriv->lock); > } > + > +void drm_prime_remove_imported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf) > +{ > + drm_prime_remove_buf_handle(prime_fpriv, dma_buf); > +} > EXPORT_SYMBOL(drm_prime_remove_imported_buf_handle); > + > +void drm_prime_remove_exported_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf) > +{ > + drm_prime_remove_buf_handle(prime_fpriv, dma_buf); > + dma_buf_put(dma_buf); We need some form of protection to ensure that the removal of the export_dma_buf and ref dropping is atomic vs. lookups. Atm there's no protection, which means if you e.g. transfer a gem_bo from one process to another with dma_buf (dropping the gem_handle right away), we'll get a leak: [first proces] 1. handle_to_fd 2. gem_close, dropping the above internal dma-buf ref 3. pass dma-buf to other process [other process] 4. fd_to_handle see export_dma_buf != NULL, doesn't re-add export reference 5. close dma_buf fd, freeing it 6. gem_close tries to unref the now free'd dma_buf For added fun, try to careful race steps 2. against 4. (or for a different kind of fun, against 5. but that requires more evil afterwards to blow up). Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch