Message ID | 1331913881-13105-1-git-send-email-rob.clark@linaro.org (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1S8ZeC-0007nB-Vr for patchwork@linuxtv.org; Fri, 16 Mar 2012 17:04:57 +0100 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.75/mailfrontend-2) with esmtp for <patchwork@linuxtv.org> id 1S8ZeC-0005CQ-H7; Fri, 16 Mar 2012 17:04:56 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031435Ab2CPQEx (ORCPT <rfc822;patchwork@linuxtv.org>); Fri, 16 Mar 2012 12:04:53 -0400 Received: from mail-gx0-f174.google.com ([209.85.161.174]:43118 "EHLO mail-gx0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031330Ab2CPQEv (ORCPT <rfc822;linux-media@vger.kernel.org>); Fri, 16 Mar 2012 12:04:51 -0400 Received: by gghe5 with SMTP id e5so4339306ggh.19 for <linux-media@vger.kernel.org>; Fri, 16 Mar 2012 09:04:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id:x-mailer; bh=ppVZz8hU4kruT8aF8CkUxNtay3nrWuh5XvLrpW+tOfk=; b=H1fTj0Mf0/MjV97FT2tRe9qhdRGC4nFfxJsuGd18gunRsbcTAe+OHDyDroB27/DBAF bCfJAEMcDetfJR9p/mf9qApNsKb4xgO7buzSjSxS/I5DPRa6QakeFIxW/ZAkMPfV/IWa 6BmZ+4j7l9UxeIcL/1o51KNtDyfZ9KwfNnnh60D4Xd1y+MLwUIafhVtGmPiJtbhTOQyu Y66gzgKYwizCJfCkL/rciNOWqTrcqEfVTpSHKHwGio9rcCoUJ8JWHJLkibpYPeu4v2Jj bALj9kd8SHQ87ipVxoFn/UnogiTgytz+/fR+xkJvAPCtFb98K7GQfWYI9ssCXbbF+5jx tbNw== Received: by 10.60.30.66 with SMTP id q2mr3643828oeh.25.1331913891064; Fri, 16 Mar 2012 09:04:51 -0700 (PDT) Received: from localhost (ppp-70-129-134-19.dsl.rcsntx.swbell.net. [70.129.134.19]) by mx.google.com with ESMTPS id s2sm4000119oea.2.2012.03.16.09.04.44 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 16 Mar 2012 09:04:44 -0700 (PDT) From: Rob Clark <rob.clark@linaro.org> To: linaro-mm-sig@lists.linaro.org, dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org Cc: patches@linaro.org, sumit.semwal@linaro.org, daniel@ffwll.ch, airlied@redhat.com, Rob Clark <rob@ti.com> Subject: [PATCH] dma-buf: add get_dma_buf() Date: Fri, 16 Mar 2012 11:04:41 -0500 Message-Id: <1331913881-13105-1-git-send-email-rob.clark@linaro.org> X-Mailer: git-send-email 1.7.5.4 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 5.6.1.2065439, Antispam-Engine: 2.7.2.376379, Antispam-Data: 2012.3.16.155131 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_1400_1499 0, BODY_SIZE_2000_LESS 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, DATE_TZ_NA 0, DATE_TZ_NEG_0500 0, __ANY_URI 0, __CP_URI_IN_BODY 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __MIME_TEXT_ONLY 0, __MULTIPLE_RCPTS_CC_X2 0, __SANE_MSGID 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_NO_WWW 0, __URI_NS ' |
Commit Message
Rob Clark
March 16, 2012, 4:04 p.m. UTC
From: Rob Clark <rob@ti.com> Works in a similar way to get_file(), and is needed in cases such as when the exporter needs to also keep a reference to the dmabuf (that is later released with a dma_buf_put()), and possibly other similar cases. Signed-off-by: Rob Clark <rob@ti.com> --- include/linux/dma-buf.h | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-)
Comments
On Fri, Mar 16, 2012 at 4:04 PM, Rob Clark <rob.clark@linaro.org> wrote: > From: Rob Clark <rob@ti.com> > > Works in a similar way to get_file(), and is needed in cases such as > when the exporter needs to also keep a reference to the dmabuf (that > is later released with a dma_buf_put()), and possibly other similar > cases. > > Signed-off-by: Rob Clark <rob@ti.com> Reviewed-by: Dave Airlie <airlied@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16 March 2012 23:23, Dave Airlie <airlied@gmail.com> wrote: > On Fri, Mar 16, 2012 at 4:04 PM, Rob Clark <rob.clark@linaro.org> wrote: >> From: Rob Clark <rob@ti.com> >> >> Works in a similar way to get_file(), and is needed in cases such as >> when the exporter needs to also keep a reference to the dmabuf (that >> is later released with a dma_buf_put()), and possibly other similar >> cases. >> >> Signed-off-by: Rob Clark <rob@ti.com> > > Reviewed-by: Dave Airlie <airlied@redhat.com> > Thanks; pulled into for-next. BR, ~me. > _______________________________________________ > Linaro-mm-sig mailing list > Linaro-mm-sig@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/linaro-mm-sig -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Mar 18, 2012 at 01:12:22PM +0530, Sumit Semwal wrote: > On 16 March 2012 23:23, Dave Airlie <airlied@gmail.com> wrote: > > On Fri, Mar 16, 2012 at 4:04 PM, Rob Clark <rob.clark@linaro.org> wrote: > >> From: Rob Clark <rob@ti.com> > >> > >> Works in a similar way to get_file(), and is needed in cases such as > >> when the exporter needs to also keep a reference to the dmabuf (that > >> is later released with a dma_buf_put()), and possibly other similar > >> cases. > >> > >> Signed-off-by: Rob Clark <rob@ti.com> > > > > Reviewed-by: Dave Airlie <airlied@redhat.com> > > > Thanks; pulled into for-next. I'm back from vacation and already grumpily complaining about dma-buf patches ;-) For consistency with dma_buf_put we should call this dma_buf_get instead of get_dma_buf ... I'll write a bikeshed patch on top of your tree. -Daniel
On Sun, Mar 18, 2012 at 08:04:53PM +0100, Daniel Vetter wrote: > On Sun, Mar 18, 2012 at 01:12:22PM +0530, Sumit Semwal wrote: > > On 16 March 2012 23:23, Dave Airlie <airlied@gmail.com> wrote: > > > On Fri, Mar 16, 2012 at 4:04 PM, Rob Clark <rob.clark@linaro.org> wrote: > > >> From: Rob Clark <rob@ti.com> > > >> > > >> Works in a similar way to get_file(), and is needed in cases such as > > >> when the exporter needs to also keep a reference to the dmabuf (that > > >> is later released with a dma_buf_put()), and possibly other similar > > >> cases. > > >> > > >> Signed-off-by: Rob Clark <rob@ti.com> > > > > > > Reviewed-by: Dave Airlie <airlied@redhat.com> > > > > > Thanks; pulled into for-next. > > I'm back from vacation and already grumpily complaining about dma-buf > patches ;-) For consistency with dma_buf_put we should call this > dma_buf_get instead of get_dma_buf ... I'll write a bikeshed patch on top > of your tree. Oops, there's already a dma_buf_get around - Rob and Dave pointed that out on irc to dense me. And I can't come up with a saner naming scheme. I'll retract my bikeshed. -Daniel
Hi, I think I discovered an interesting issue with dma_buf. I found out that dma_buf_fd does not increase reference count for dma_buf::file. This leads to potential kernel crash triggered by user space. Please, take a look on the scenario below: The applications spawns two thread. One of them is exporting DMABUF. Thread I | Thread II | Comments -----------------------+-------------------+----------------------------------- dbuf = dma_buf_export | | dma_buf is creates, refcount is 1 fd = dma_buf_fd(dbuf) | | assume fd is set to 42, refcount is still 1 | close(42) | The file descriptor is closed asynchronously, dbuf's refcount drops to 0 | dma_buf_release | dbuf structure is freed, dbuf becomes a dangling pointer int size = dbuf->size; | | the dbuf is dereferenced, causing a kernel crash -----------------------+-------------------+----------------------------------- I think that the problem could be fixed in two ways. a) forcing driver developer to call get_dma_buf just before calling dma_buf_fd. b) increasing dma_buf->file's reference count at dma_buf_fd I prefer solution (b) because it prevents symmetry between dma_buf_fd and close. I mean that dma_buf_fd increases reference count, close decreases it. What is your opinion about the issue? Regards, Tomasz Stanislawski On 03/16/2012 05:04 PM, Rob Clark wrote: > From: Rob Clark <rob@ti.com> > > Works in a similar way to get_file(), and is needed in cases such as > when the exporter needs to also keep a reference to the dmabuf (that > is later released with a dma_buf_put()), and possibly other similar > cases. > > Signed-off-by: Rob Clark <rob@ti.com> > --- -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 22, 2012 at 03:47:12PM +0200, Tomasz Stanislawski wrote: > Hi, > I think I discovered an interesting issue with dma_buf. > I found out that dma_buf_fd does not increase reference > count for dma_buf::file. This leads to potential kernel > crash triggered by user space. Please, take a look on > the scenario below: > > The applications spawns two thread. One of them is exporting DMABUF. > > Thread I | Thread II | Comments > -----------------------+-------------------+----------------------------------- > dbuf = dma_buf_export | | dma_buf is creates, refcount is 1 > fd = dma_buf_fd(dbuf) | | assume fd is set to 42, refcount is still 1 > | close(42) | The file descriptor is closed asynchronously, dbuf's refcount drops to 0 > | dma_buf_release | dbuf structure is freed, dbuf becomes a dangling pointer > int size = dbuf->size; | | the dbuf is dereferenced, causing a kernel crash > -----------------------+-------------------+----------------------------------- > > I think that the problem could be fixed in two ways. > a) forcing driver developer to call get_dma_buf just before calling dma_buf_fd. > b) increasing dma_buf->file's reference count at dma_buf_fd > > I prefer solution (b) because it prevents symmetry between dma_buf_fd and close. > I mean that dma_buf_fd increases reference count, close decreases it. > > What is your opinion about the issue? I guess most exporters would like to hang onto the exported dma_buf a bit and hence need a reference (e.g. to cache the dma_buf as long as the underlying buffer object exists). So I guess we can change the semantics of dma_buf_fd from transferring the reference you currently have (and hence forbidding any further access by the caller) to grabbing a reference of it's on for the fd that is created. -Daniel
On 05/22/2012 04:32 PM, Daniel Vetter wrote: > On Tue, May 22, 2012 at 03:47:12PM +0200, Tomasz Stanislawski wrote: >> Hi, >> I think I discovered an interesting issue with dma_buf. >> I found out that dma_buf_fd does not increase reference >> count for dma_buf::file. This leads to potential kernel >> crash triggered by user space. Please, take a look on >> the scenario below: >> >> The applications spawns two thread. One of them is exporting DMABUF. >> >> Thread I | Thread II | Comments >> -----------------------+-------------------+----------------------------------- >> dbuf = dma_buf_export | | dma_buf is creates, refcount is 1 >> fd = dma_buf_fd(dbuf) | | assume fd is set to 42, refcount is still 1 >> | close(42) | The file descriptor is closed asynchronously, dbuf's refcount drops to 0 >> | dma_buf_release | dbuf structure is freed, dbuf becomes a dangling pointer >> int size = dbuf->size; | | the dbuf is dereferenced, causing a kernel crash >> -----------------------+-------------------+----------------------------------- >> >> I think that the problem could be fixed in two ways. >> a) forcing driver developer to call get_dma_buf just before calling dma_buf_fd. >> b) increasing dma_buf->file's reference count at dma_buf_fd >> >> I prefer solution (b) because it prevents symmetry between dma_buf_fd and close. >> I mean that dma_buf_fd increases reference count, close decreases it. >> >> What is your opinion about the issue? > > I guess most exporters would like to hang onto the exported dma_buf a bit > and hence need a reference (e.g. to cache the dma_buf as long as the > underlying buffer object exists). So I guess we can change the semantics > of dma_buf_fd from transferring the reference you currently have (and > hence forbidding any further access by the caller) to grabbing a reference > of it's on for the fd that is created. > -Daniel Hi Daniel, Would it be simpler, safer and more intuitive if dma_buf_fd increased dmabuf->file's reference counter? Regards, Tomasz Stanislawski -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 22, 2012 at 5:00 PM, Tomasz Stanislawski <t.stanislaws@samsung.com> wrote: > On 05/22/2012 04:32 PM, Daniel Vetter wrote: >> On Tue, May 22, 2012 at 03:47:12PM +0200, Tomasz Stanislawski wrote: >>> Hi, >>> I think I discovered an interesting issue with dma_buf. >>> I found out that dma_buf_fd does not increase reference >>> count for dma_buf::file. This leads to potential kernel >>> crash triggered by user space. Please, take a look on >>> the scenario below: >>> >>> The applications spawns two thread. One of them is exporting DMABUF. >>> >>> Thread I | Thread II | Comments >>> -----------------------+-------------------+----------------------------------- >>> dbuf = dma_buf_export | | dma_buf is creates, refcount is 1 >>> fd = dma_buf_fd(dbuf) | | assume fd is set to 42, refcount is still 1 >>> | close(42) | The file descriptor is closed asynchronously, dbuf's refcount drops to 0 >>> | dma_buf_release | dbuf structure is freed, dbuf becomes a dangling pointer >>> int size = dbuf->size; | | the dbuf is dereferenced, causing a kernel crash >>> -----------------------+-------------------+----------------------------------- >>> >>> I think that the problem could be fixed in two ways. >>> a) forcing driver developer to call get_dma_buf just before calling dma_buf_fd. >>> b) increasing dma_buf->file's reference count at dma_buf_fd >>> >>> I prefer solution (b) because it prevents symmetry between dma_buf_fd and close. >>> I mean that dma_buf_fd increases reference count, close decreases it. >>> >>> What is your opinion about the issue? >> >> I guess most exporters would like to hang onto the exported dma_buf a bit >> and hence need a reference (e.g. to cache the dma_buf as long as the >> underlying buffer object exists). So I guess we can change the semantics >> of dma_buf_fd from transferring the reference you currently have (and >> hence forbidding any further access by the caller) to grabbing a reference >> of it's on for the fd that is created. >> -Daniel > > Hi Daniel, > Would it be simpler, safer and more intuitive if dma_buf_fd increased > dmabuf->file's reference counter? That's actually what I wanted to say. Message seems to have been lost in transit ;-) -Daniel
On Tue, May 22, 2012 at 4:05 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, May 22, 2012 at 5:00 PM, Tomasz Stanislawski > <t.stanislaws@samsung.com> wrote: >> On 05/22/2012 04:32 PM, Daniel Vetter wrote: >>> On Tue, May 22, 2012 at 03:47:12PM +0200, Tomasz Stanislawski wrote: >>>> Hi, >>>> I think I discovered an interesting issue with dma_buf. >>>> I found out that dma_buf_fd does not increase reference >>>> count for dma_buf::file. This leads to potential kernel >>>> crash triggered by user space. Please, take a look on >>>> the scenario below: >>>> >>>> The applications spawns two thread. One of them is exporting DMABUF. >>>> >>>> Thread I | Thread II | Comments >>>> -----------------------+-------------------+----------------------------------- >>>> dbuf = dma_buf_export | | dma_buf is creates, refcount is 1 >>>> fd = dma_buf_fd(dbuf) | | assume fd is set to 42, refcount is still 1 >>>> | close(42) | The file descriptor is closed asynchronously, dbuf's refcount drops to 0 >>>> | dma_buf_release | dbuf structure is freed, dbuf becomes a dangling pointer >>>> int size = dbuf->size; | | the dbuf is dereferenced, causing a kernel crash >>>> -----------------------+-------------------+----------------------------------- >>>> >>>> I think that the problem could be fixed in two ways. >>>> a) forcing driver developer to call get_dma_buf just before calling dma_buf_fd. >>>> b) increasing dma_buf->file's reference count at dma_buf_fd >>>> >>>> I prefer solution (b) because it prevents symmetry between dma_buf_fd and close. >>>> I mean that dma_buf_fd increases reference count, close decreases it. >>>> >>>> What is your opinion about the issue? >>> >>> I guess most exporters would like to hang onto the exported dma_buf a bit >>> and hence need a reference (e.g. to cache the dma_buf as long as the >>> underlying buffer object exists). So I guess we can change the semantics >>> of dma_buf_fd from transferring the reference you currently have (and >>> hence forbidding any further access by the caller) to grabbing a reference >>> of it's on for the fd that is created. >>> -Daniel >> >> Hi Daniel, >> Would it be simpler, safer and more intuitive if dma_buf_fd increased >> dmabuf->file's reference counter? > > That's actually what I wanted to say. Message seems to have been lost > in transit ;-) Now I've thought about it and Tomasz has pointed it out I agree, Now we just have to work out when to drop that reference, which I don't see anyone addressing :-) I love lifetime rules. Dave. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 22, 2012 at 9:13 AM, Dave Airlie <airlied@gmail.com> wrote: > On Tue, May 22, 2012 at 4:05 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Tue, May 22, 2012 at 5:00 PM, Tomasz Stanislawski >> <t.stanislaws@samsung.com> wrote: >>> On 05/22/2012 04:32 PM, Daniel Vetter wrote: >>>> On Tue, May 22, 2012 at 03:47:12PM +0200, Tomasz Stanislawski wrote: >>>>> Hi, >>>>> I think I discovered an interesting issue with dma_buf. >>>>> I found out that dma_buf_fd does not increase reference >>>>> count for dma_buf::file. This leads to potential kernel >>>>> crash triggered by user space. Please, take a look on >>>>> the scenario below: >>>>> >>>>> The applications spawns two thread. One of them is exporting DMABUF. >>>>> >>>>> Thread I | Thread II | Comments >>>>> -----------------------+-------------------+----------------------------------- >>>>> dbuf = dma_buf_export | | dma_buf is creates, refcount is 1 >>>>> fd = dma_buf_fd(dbuf) | | assume fd is set to 42, refcount is still 1 >>>>> | close(42) | The file descriptor is closed asynchronously, dbuf's refcount drops to 0 >>>>> | dma_buf_release | dbuf structure is freed, dbuf becomes a dangling pointer >>>>> int size = dbuf->size; | | the dbuf is dereferenced, causing a kernel crash >>>>> -----------------------+-------------------+----------------------------------- >>>>> >>>>> I think that the problem could be fixed in two ways. >>>>> a) forcing driver developer to call get_dma_buf just before calling dma_buf_fd. >>>>> b) increasing dma_buf->file's reference count at dma_buf_fd >>>>> >>>>> I prefer solution (b) because it prevents symmetry between dma_buf_fd and close. >>>>> I mean that dma_buf_fd increases reference count, close decreases it. >>>>> >>>>> What is your opinion about the issue? >>>> >>>> I guess most exporters would like to hang onto the exported dma_buf a bit >>>> and hence need a reference (e.g. to cache the dma_buf as long as the >>>> underlying buffer object exists). So I guess we can change the semantics >>>> of dma_buf_fd from transferring the reference you currently have (and >>>> hence forbidding any further access by the caller) to grabbing a reference >>>> of it's on for the fd that is created. >>>> -Daniel >>> >>> Hi Daniel, >>> Would it be simpler, safer and more intuitive if dma_buf_fd increased >>> dmabuf->file's reference counter? >> >> That's actually what I wanted to say. Message seems to have been lost >> in transit ;-) > > Now I've thought about it and Tomasz has pointed it out I agree, > > Now we just have to work out when to drop that reference, which I > don't see anyone addressing :-) > > I love lifetime rules. I think in the GEM case, where we keep a ref in obj->export_dma_buf, we can drop the extra ref to the dmabuf (if we decide dma_buf_fd() increases the refcnt), as long as we be sure to NULL out obj->export_dma_buf from dmabuf_ops->release (which is unfortunately in each driver at the moment).. This way obj->export_dma_buf behaves as a sort of weak-reference, not causing a memory leak. BR, -R > Dave. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index cbdff81..25eb287 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -132,6 +132,20 @@ struct dma_buf_attachment { void *priv; }; +/** + * get_dma_buf - convenience wrapper for get_file. + * @dmabuf: [in] pointer to dma_buf + * + * Increments the reference count on the dma-buf, needed in case of drivers + * that either need to create additional references to the dmabuf on the + * kernel side. For example, an exporter that needs to keep a dmabuf ptr + * so that subsequent exports don't create a new dmabuf. + */ +static inline void get_dma_buf(struct dma_buf *dmabuf) +{ + get_file(dmabuf->file); +} + #ifdef CONFIG_DMA_SHARED_BUFFER struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, struct device *dev);