| Message ID | 1343035613-30104-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 23, 2012 at 7:26 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Well, actually add some, because currently there's exactly none: > - in handle_to_fd we only take the file_priv's prime lock, but the > underlying gem object we're exporting isn't per-file. > - in each driver's dma_buf->release callback we don't grab any locks > at all. Finally got to this patch and applied it to my test box and it explodes in the following start X, bind udl as an output slave, set a mode on the udl, then kill the X server, I get an oops on i915/radeon/nouveau, like [ 8046.363596] general protection fault: 0000 [#1] SMP [ 8046.364012] Modules linked in: fuse lockd rfcomm sunrpc bnep tpm_bios ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_filter ip6_tables nf_conntrack_ ipv4 nf_defrag_ipv4 xt_state nf_conntrack snd_hda_codec_conexant arc4 iwldvm mac80211 coretemp kvm_intel snd_hda_intel snd_hda_codec kvm btusb snd_hwdep bluet ooth iwlwifi i915 snd_seq snd_seq_device microcode snd_pcm cfg80211 snd_page_alloc i2c_i801 lpc_ich e1000e mfd_core snd_timer wmi thinkpad_acpi snd soundcore rfkill video uinput sdhci_pci sdhci udl syscopyarea sysfillrect sysimgblt firewire_ohci mmc_core drm_usb firewire_core crc_itu_t yenta_socket radeon i2c_algo_ bit drm_kms_helper ttm drm i2c_core [ 8046.364012] CPU 0 [ 8046.364012] Pid: 6947, comm: Xorg Tainted: G W 3.6.0-rc3+ #7 LENOVO 406334M/406334M [ 8046.364012] RIP: 0010:[<ffffffffa0388e0c>] [<ffffffffa0388e0c>] i915_gem_unmap_dma_buf+0x5c/0xb0 [i915] [ 8046.364012] RSP: 0018:ffff88002c22dc28 EFLAGS: 00010296 [ 8046.364012] RAX: 0000000000000500 RBX: ffff880007d5d6d8 RCX: 0000000000000000 [ 8046.364012] RDX: 0000000000000500 RSI: ffff8800570ca000 RDI: ffff88005b2ea5a8 [ 8046.364012] RBP: ffff88002c22dc68 R08: 0000000000000000 R09: 0000000000000000 [ 8046.364012] R10: 0000000000000000 R11: 0000000000000001 R12: ffff88005b2ea5a8 [ 8046.364012] R13: 0000000000000000 R14: 6b6b6b6b6b6b6b6b R15: ffff8800570ca000 [ 8046.364012] FS: 00007f79955759c0(0000) GS:ffff880078c00000(0000) knlGS:0000000000000000 [ 8046.364012] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 8046.364012] CR2: 00000031808bab90 CR3: 0000000001c0b000 CR4: 00000000000007f0 [ 8046.364012] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 8046.364012] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 [ 8046.364012] Process Xorg (pid: 6947, threadinfo ffff88002c22c000, task ffff88005d680000) [ 8046.364012] Stack: [ 8046.364012] ffff880058d167b0 ffff880000000500 ffff88002c22dc48 ffff88005f6f0f50 [ 8046.364012] ffff88005d9622f8 ffff88005d962290 0000000000000000 ffffffffffffffff [ 8046.364012] ffff88002c22dc78 ffffffff81409182 ffff88002c22dc98 ffffffffa002cfeb [ 8046.364012] Call Trace: [ 8046.364012] [<ffffffff81409182>] dma_buf_unmap_attachment+0x22/0x40 [ 8046.364012] [<ffffffffa002cfeb>] drm_prime_gem_destroy+0x2b/0x50 [drm] [ 8046.364012] [<ffffffffa01f9cb9>] udl_gem_free_object+0x39/0x70 [udl] [ 8046.364012] [<ffffffffa001695a>] drm_gem_object_free+0x2a/0x30 [drm] [ 8046.364012] [<ffffffffa00171a8>] drm_gem_object_release_handle+0xa8/0xd0 [drm] [ 8046.364012] [<ffffffff81319b85>] idr_for_each+0xe5/0x180 [ 8046.364012] [<ffffffffa0017100>] ? drm_gem_vm_open+0x70/0x70 [drm] [ 8046.364012] [<ffffffff810bea2d>] ? trace_hardirqs_on+0xd/0x10 [ 8046.364012] [<ffffffffa0017804>] drm_gem_release+0x24/0x40 [drm] [ 8046.364012] [<ffffffffa0015e1a>] drm_release+0x55a/0x5f0 [drm] [ 8046.364012] [<ffffffff811a908a>] __fput+0xfa/0x2d0 [ 8046.364012] [<ffffffff811a926e>] ____fput+0xe/0x10 [ 8046.364012] [<ffffffff81076c89>] task_work_run+0x69/0xa0 [ 8046.364012] [<ffffffff81056aee>] do_exit+0xa0e/0xb20 [ 8046.364012] [<ffffffff816887d5>] ? retint_swapgs+0x13/0x1b [ 8046.364012] [<ffffffff81056f4c>] do_group_exit+0x4c/0xc0 which implies some reference count is off or some object is freed in the wrong order. Dave.
On Fri, Aug 24, 2012 at 02:13:33PM +1000, Dave Airlie wrote: > On Mon, Jul 23, 2012 at 7:26 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > Well, actually add some, because currently there's exactly none: > > - in handle_to_fd we only take the file_priv's prime lock, but the > > underlying gem object we're exporting isn't per-file. > > - in each driver's dma_buf->release callback we don't grab any locks > > at all. > > Finally got to this patch and applied it to my test box and it > explodes in the following > > start X, bind udl as an output slave, set a mode on the udl, then kill > the X server, > > I get an oops on i915/radeon/nouveau, like I have to admit that I don't see what's happening here :( But thinking some more about the locking issues and race my patch tried to fix I concluded that it's the wrong approach anyway - we still have ugly issues left. The fundamental problem is that for the exporter we have 2 refcounts, the gem refcount and the dma_buf refcount, but we should only destroy the gem object + it's dma_buf export if both are 0. And keep everything around otherwise to avoid nasty issues with re-export or reaping userspace-facing resources like the mmap_offset. But I haven't yet grown clue how to handle this any better. Slightly related: Jani discovered that we seem to have a leak, putting him on cc just in case he's the one with clue ;-) I've looked again at the other two patches inspired by this one, which fix some races between gem names & gem flink. And I think those two are still valid. Yours, Daniel > > [ 8046.363596] general protection fault: 0000 [#1] SMP > [ 8046.364012] Modules linked in: fuse lockd rfcomm sunrpc bnep > tpm_bios ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_filter > ip6_tables nf_conntrack_ > ipv4 nf_defrag_ipv4 xt_state nf_conntrack snd_hda_codec_conexant arc4 > iwldvm mac80211 coretemp kvm_intel snd_hda_intel snd_hda_codec kvm > btusb snd_hwdep bluet > ooth iwlwifi i915 snd_seq snd_seq_device microcode snd_pcm cfg80211 > snd_page_alloc i2c_i801 lpc_ich e1000e mfd_core snd_timer wmi > thinkpad_acpi snd soundcore > rfkill video uinput sdhci_pci sdhci udl syscopyarea sysfillrect > sysimgblt firewire_ohci mmc_core drm_usb firewire_core crc_itu_t > yenta_socket radeon i2c_algo_ > bit drm_kms_helper ttm drm i2c_core > [ 8046.364012] CPU 0 > [ 8046.364012] Pid: 6947, comm: Xorg Tainted: G W 3.6.0-rc3+ > #7 LENOVO 406334M/406334M > [ 8046.364012] RIP: 0010:[<ffffffffa0388e0c>] [<ffffffffa0388e0c>] > i915_gem_unmap_dma_buf+0x5c/0xb0 [i915] > [ 8046.364012] RSP: 0018:ffff88002c22dc28 EFLAGS: 00010296 > [ 8046.364012] RAX: 0000000000000500 RBX: ffff880007d5d6d8 RCX: 0000000000000000 > [ 8046.364012] RDX: 0000000000000500 RSI: ffff8800570ca000 RDI: ffff88005b2ea5a8 > [ 8046.364012] RBP: ffff88002c22dc68 R08: 0000000000000000 R09: 0000000000000000 > [ 8046.364012] R10: 0000000000000000 R11: 0000000000000001 R12: ffff88005b2ea5a8 > [ 8046.364012] R13: 0000000000000000 R14: 6b6b6b6b6b6b6b6b R15: ffff8800570ca000 > [ 8046.364012] FS: 00007f79955759c0(0000) GS:ffff880078c00000(0000) > knlGS:0000000000000000 > [ 8046.364012] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 8046.364012] CR2: 00000031808bab90 CR3: 0000000001c0b000 CR4: 00000000000007f0 > [ 8046.364012] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > [ 8046.364012] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 > [ 8046.364012] Process Xorg (pid: 6947, threadinfo ffff88002c22c000, > task ffff88005d680000) > [ 8046.364012] Stack: > [ 8046.364012] ffff880058d167b0 ffff880000000500 ffff88002c22dc48 > ffff88005f6f0f50 > [ 8046.364012] ffff88005d9622f8 ffff88005d962290 0000000000000000 > ffffffffffffffff > [ 8046.364012] ffff88002c22dc78 ffffffff81409182 ffff88002c22dc98 > ffffffffa002cfeb > [ 8046.364012] Call Trace: > [ 8046.364012] [<ffffffff81409182>] dma_buf_unmap_attachment+0x22/0x40 > [ 8046.364012] [<ffffffffa002cfeb>] drm_prime_gem_destroy+0x2b/0x50 [drm] > [ 8046.364012] [<ffffffffa01f9cb9>] udl_gem_free_object+0x39/0x70 [udl] > [ 8046.364012] [<ffffffffa001695a>] drm_gem_object_free+0x2a/0x30 [drm] > [ 8046.364012] [<ffffffffa00171a8>] > drm_gem_object_release_handle+0xa8/0xd0 [drm] > [ 8046.364012] [<ffffffff81319b85>] idr_for_each+0xe5/0x180 > [ 8046.364012] [<ffffffffa0017100>] ? drm_gem_vm_open+0x70/0x70 [drm] > [ 8046.364012] [<ffffffff810bea2d>] ? trace_hardirqs_on+0xd/0x10 > [ 8046.364012] [<ffffffffa0017804>] drm_gem_release+0x24/0x40 [drm] > [ 8046.364012] [<ffffffffa0015e1a>] drm_release+0x55a/0x5f0 [drm] > [ 8046.364012] [<ffffffff811a908a>] __fput+0xfa/0x2d0 > [ 8046.364012] [<ffffffff811a926e>] ____fput+0xe/0x10 > [ 8046.364012] [<ffffffff81076c89>] task_work_run+0x69/0xa0 > [ 8046.364012] [<ffffffff81056aee>] do_exit+0xa0e/0xb20 > [ 8046.364012] [<ffffffff816887d5>] ? retint_swapgs+0x13/0x1b > [ 8046.364012] [<ffffffff81056f4c>] do_group_exit+0x4c/0xc0 > > which implies some reference count is off or some object is freed in > the wrong order. > > Dave.
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index fbe0842..6be7c2e 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -94,6 +94,7 @@ drm_gem_init(struct drm_device *dev) spin_lock_init(&dev->object_name_lock); idr_init(&dev->object_name_idr); + mutex_init(&dev->prime_lock); mm = kzalloc(sizeof(struct drm_gem_mm), GFP_KERNEL); if (!mm) { @@ -208,9 +209,9 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) drm_prime_remove_imported_buf_handle(&filp->prime, obj->import_attach->dmabuf); } - if (obj->export_dma_buf) { + if (obj->dma_buf) { drm_prime_remove_imported_buf_handle(&filp->prime, - obj->export_dma_buf); + obj->dma_buf); } } @@ -604,6 +605,9 @@ static void drm_gem_object_ref_bug(struct kref *list_kref) * Removes any name for the object. Note that this must be * called before drm_gem_object_free or we'll be touching * freed memory + * + * Must _not_ be called while the dev->struct_mutex lock is held, or any + * driver-specific lock that is required in the dma buf's ->release callback. */ void drm_gem_object_handle_free(struct drm_gem_object *obj) { @@ -625,6 +629,18 @@ void drm_gem_object_handle_free(struct drm_gem_object *obj) } else spin_unlock(&dev->object_name_lock); + mutex_lock(&dev->prime_lock); + if (obj->dma_buf) { + struct dma_buf *buf = obj->dma_buf; + obj->dma_buf = NULL; + mutex_unlock(&dev->prime_lock); + + /* We still hold a reference to the underlying GEM object, so + * this will free at most the dma buf object itself. */ + dma_buf_put(buf); + } else + mutex_unlock(&dev->prime_lock); + } EXPORT_SYMBOL(drm_gem_object_handle_free); diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index f546ff9..4f80374 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -84,36 +84,47 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, return 0; } - if (obj->export_dma_buf) { - get_dma_buf(obj->export_dma_buf); - *prime_fd = dma_buf_fd(obj->export_dma_buf, flags); - drm_gem_object_unreference_unlocked(obj); + mutex_lock(&dev->prime_lock); + if (obj->dma_buf) { + get_dma_buf(obj->dma_buf); } else { buf = dev->driver->gem_prime_export(dev, obj, flags); if (IS_ERR(buf)) { /* normally the created dma-buf takes ownership of the ref, * but if that fails then drop the ref */ - drm_gem_object_unreference_unlocked(obj); - mutex_unlock(&file_priv->prime.lock); - return PTR_ERR(buf); + ret = PTR_ERR(buf); + goto out; } - obj->export_dma_buf = buf; - *prime_fd = dma_buf_fd(buf, flags); + /* Allocate a reference for the dma buf. */ + drm_gem_object_reference(obj); + + /* Allocate a reference for the re-export cache. */ + get_dma_buf(buf); + obj->dma_buf = buf; } /* if we've exported this buffer the cheat and add it to the import list * so we get the correct handle back */ ret = drm_prime_add_imported_buf_handle(&file_priv->prime, - obj->export_dma_buf, handle); - if (ret) { - drm_gem_object_unreference_unlocked(obj); - mutex_unlock(&file_priv->prime.lock); - return ret; - } + obj->dma_buf, handle); + if (ret) + goto out; + /* Only set up the userspace fd once everything is done. */ + *prime_fd = dma_buf_fd(obj->dma_buf, flags); +out: + mutex_unlock(&dev->prime_lock); mutex_unlock(&file_priv->prime.lock); - return 0; + + /* Check whether someone sneaky dropped the last userspace gem handle, + * clean up the mess if so. */ + if (atomic_read(&obj->handle_count) == 0) + drm_gem_object_handle_free(obj); + + drm_gem_object_unreference_unlocked(obj); + + return ret; } EXPORT_SYMBOL(drm_gem_prime_handle_to_fd); @@ -133,21 +144,38 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, ret = drm_prime_lookup_imported_buf_handle(&file_priv->prime, dma_buf, handle); if (!ret) { - ret = 0; - goto out_put; + mutex_unlock(&file_priv->prime.lock); + dma_buf_put(dma_buf); + + return 0; } /* never seen this one, need to import */ obj = dev->driver->gem_prime_import(dev, dma_buf); if (IS_ERR(obj)) { - ret = PTR_ERR(obj); - goto out_put; + mutex_unlock(&file_priv->prime.lock); + dma_buf_put(dma_buf); + + return PTR_ERR(obj); } + mutex_lock(&dev->prime_lock); + /* Refill the export cache - this is only really important if the dma + * buf was the only userspace visible handle left and we're re-importing + * into the original exporter, in which case we've cleared obj->dma_buf + * already. Because we will create a GEM userspace handle below we only + * need to check for gem_close races if that fails. + */ + if (!obj->dma_buf) { + obj->dma_buf = dma_buf; + get_dma_buf(dma_buf); + } else + WARN_ON(obj->dma_buf != dma_buf); + mutex_unlock(&dev->prime_lock); + ret = drm_gem_handle_create(file_priv, obj, handle); - drm_gem_object_unreference_unlocked(obj); if (ret) - goto out_put; + goto fail_handle; ret = drm_prime_add_imported_buf_handle(&file_priv->prime, dma_buf, *handle); @@ -155,6 +183,8 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, goto fail; mutex_unlock(&file_priv->prime.lock); + drm_gem_object_unreference_unlocked(obj); + return 0; fail: @@ -162,9 +192,13 @@ fail: * to detach.. which seems ok.. */ drm_gem_object_handle_unreference_unlocked(obj); -out_put: - dma_buf_put(dma_buf); +fail_handle: + if (atomic_read(&obj->handle_count) == 0) + drm_gem_object_handle_free(obj); mutex_unlock(&file_priv->prime.lock); + drm_gem_object_unreference_unlocked(obj); + dma_buf_put(dma_buf); + return ret; } EXPORT_SYMBOL(drm_gem_prime_fd_to_handle); diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c index 2749092..f270790 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c @@ -112,21 +112,7 @@ static void exynos_dmabuf_release(struct dma_buf *dmabuf) DRM_DEBUG_PRIME("%s\n", __FILE__); - /* - * exynos_dmabuf_release() call means that file object's - * f_count is 0 and it calls drm_gem_object_handle_unreference() - * to drop the references that these values had been increased - * at drm_prime_handle_to_fd() - */ - if (exynos_gem_obj->base.export_dma_buf == dmabuf) { - exynos_gem_obj->base.export_dma_buf = NULL; - - /* - * drop this gem object refcount to release allocated buffer - * and resources. - */ - drm_gem_object_unreference_unlocked(&exynos_gem_obj->base); - } + drm_gem_object_unreference_unlocked(&exynos_gem_obj->base); } static void *exynos_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf, diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index aa308e1..cf6bdec 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -67,11 +67,7 @@ static void i915_gem_dmabuf_release(struct dma_buf *dma_buf) { struct drm_i915_gem_object *obj = dma_buf->priv; - if (obj->base.export_dma_buf == dma_buf) { - /* drop the reference on the export fd holds */ - obj->base.export_dma_buf = NULL; - drm_gem_object_unreference_unlocked(&obj->base); - } + drm_gem_object_unreference_unlocked(&obj->base); } static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c index a25cf2c..4c82801 100644 --- a/drivers/gpu/drm/nouveau/nouveau_prime.c +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c @@ -59,10 +59,7 @@ static void nouveau_gem_dmabuf_release(struct dma_buf *dma_buf) { struct nouveau_bo *nvbo = dma_buf->priv; - if (nvbo->gem->export_dma_buf == dma_buf) { - nvbo->gem->export_dma_buf = NULL; - drm_gem_object_unreference_unlocked(nvbo->gem); - } + drm_gem_object_unreference_unlocked(nvbo->gem); } static void *nouveau_gem_kmap_atomic(struct dma_buf *dma_buf, unsigned long page_num) diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c index 6bef46a..28f79ac 100644 --- a/drivers/gpu/drm/radeon/radeon_prime.c +++ b/drivers/gpu/drm/radeon/radeon_prime.c @@ -59,11 +59,7 @@ static void radeon_gem_dmabuf_release(struct dma_buf *dma_buf) { struct radeon_bo *bo = dma_buf->priv; - if (bo->gem_base.export_dma_buf == dma_buf) { - DRM_ERROR("unreference dmabuf %p\n", &bo->gem_base); - bo->gem_base.export_dma_buf = NULL; - drm_gem_object_unreference_unlocked(&bo->gem_base); - } + drm_gem_object_unreference_unlocked(&bo->gem_base); } static void *radeon_gem_kmap_atomic(struct dma_buf *dma_buf, unsigned long page_num) diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d6b67bb..bc8a26a 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -668,10 +668,23 @@ struct drm_gem_object { void *driver_private; - /* dma buf exported from this GEM object */ - struct dma_buf *export_dma_buf; + /** dma_buf - dma buf associated with this GEM object + * + * This is a cache of the dma buf associated to this GEM object to + * ensure that there is only ever one dma buf for a given GEM object. + * Only kept around as long as there are still userspace handles around. + * If only the dma buf holds the last userspace-visible reference on + * this GEM object, dma_buf will be NULL, but set again on import. + * Protected by dev->prime_lock. + */ + struct dma_buf *dma_buf; - /* dma buf attachment backing this object */ + /** + * import_attach - dma buf attachment backing this object. + * + * Invariant over the lifetime + * of an object, hence needs no locking. + */ struct dma_buf_attachment *import_attach; }; @@ -1193,6 +1206,15 @@ struct drm_device { /*@{ */ spinlock_t object_name_lock; struct idr object_name_idr; + + /** + * prime_lock - protects dma buf state of exported GEM objects + * + * Specifically obj->dma_buf, but this can be used by drivers for + * other things. + */ + struct mutex prime_lock; + /*@} */ int switch_power_state; @@ -1651,24 +1673,6 @@ drm_gem_object_handle_reference(struct drm_gem_object *obj) } static inline void -drm_gem_object_handle_unreference(struct drm_gem_object *obj) -{ - if (obj == NULL) - return; - - if (atomic_read(&obj->handle_count) == 0) - return; - /* - * Must bump handle count first as this may be the last - * ref, in which case the object would disappear before we - * checked for a name - */ - if (atomic_dec_and_test(&obj->handle_count)) - drm_gem_object_handle_free(obj); - drm_gem_object_unreference(obj); -} - -static inline void drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj) { if (obj == NULL)
Well, actually add some, because currently there's exactly none: - in handle_to_fd we only take the file_priv's prime lock, but the underlying gem object we're exporting isn't per-file. - in each driver's dma_buf->release callback we don't grab any locks at all. Also, we're not protected by any object lifetime rules: We can export and GEM object then fclose that fd _while_ we re-export the same object again. If we're unlucky, we'll see a non-zero export_dma_buf, but the file's refcount is already zero. To fix this, we need two pieces: - We need to add some locking. I've opted for a new per-device lock to not entangle this locking with the already messy dev->struct_mutex misery. - We need to hold a reference onto the dma buf object while exported_dma_buf is non-NULL. Otherwise we'll always have races where the dma buf's file refcount is zero already, but export_dma_buf is still set. Now obviously this dma buf reference would lead to a refcount loop, so we need to break that at the right time. Do the same trick as for the gem flink name: As soon as all GEM userspace handles are gone (i.e. obj->handle_count == 0) we drop this dma_buf reference. This has a few funny complications: - We could hold the last dma buf reference (but by necessity not the last reference to the underlying GEM object). The only thing that can therefore happen is that the dma buf object gets freed and we call the driver's ->release callback. Which needs to drop its own reference to the underlying GEM object. To ensure that we don't end up with surprises kill the (already) unused handle_unreference variant and only expose the unlocked one (which is safer). - Userspace might race with us when exporting a dma_buf and close the last userspace GEM handle. Check for that and re-call handle_free to clean up the mess. Note that the current flink name code is suffering from the same race but doesn't hanlde it. A follow-up patch will fix this. - Userspace might drop all GEM handles, but still hold a reference through a dma buf object. To ensure that an export of this object after it has been imported will do the right thing, we also need to set up the dma buf cache again. Otherwise we could create a second dma buf when exporting it again. Also add some paranoia and check whether a re-import gives back the right dma_buf. Because this cache is now not only set on export but also on import and is always set if there's a dma buf associated for this GEM object (and some GEM userspace handles still around), drop the export_ prefix and just call it obj->dma_buf. Note that this patch also fixes another little race on the export side: As soon as we've called dma_buf_fd, userspace could sneak in and close the fd, hence reaping the last reference count to the dma_buf and destroying it. Avoid this use-after-free by doing the fd-installing last, once everything has been set up correctly. v2: Be careful to not drop any references while holding locks. This only really affected -ENOMEM error paths, but could lead to a deadlock on one of the prime locks. v3: Chris Wilson complained that a per-obj mutex is a bit much. So moved to the device again. Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_gem.c | 20 ++++++- drivers/gpu/drm/drm_prime.c | 82 ++++++++++++++++++++-------- drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 16 +----- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 6 +- drivers/gpu/drm/nouveau/nouveau_prime.c | 5 +- drivers/gpu/drm/radeon/radeon_prime.c | 6 +- include/drm/drmP.h | 46 +++++++++------- 7 files changed, 105 insertions(+), 76 deletions(-)