This way drivers can't grow crazy ideas any more, and it also
helps a bit in reviewing EXPORT_SYMBOLS.
v2: Even more stuff. Unfortunately we can't move drm_vm_open_locked
because exynos does some horrible stuff with it.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
drm_gem_get_pages() currently allows passing a 'gfp' parameter that is
passed to shmem combined with mapping_gfp_mask(). Given that the default
mapping_gfp_mask() is GFP_HIGHUSER, it is _very_ unlikely that anyone will
ever make use of that parameter. In fact, all drivers currently pass
redundant flags or 0.
This patch removes the 'gfp' parameter. The only reason to keep it is to
remove flags like __GFP_WAIT. But in its current form, it can only be used
to add flags. So to remove __GFP_WAIT, you'd have to drop it from the
mapping_gfp_mask, which again is stupid as this mask is used by shmem-core
for other allocations, too.
If any driver ever requires that parameter, we can introduce a new helper
that takes the raw 'gfp' parameter. The caller'd be responsible to combine
it with mapping_gfp_mask() in a suitable way. The current
drm_gem_get_pages() helper would then simply use mapping_gfp_mask() and
call the new helper. This is what shmem_read_mapping_pages{_gfp,} does
right now.
Moreover, the gfp-zone flag-usage is not obvious: If you pass a modified
zone, shmem core will WARN() or even BUG(). In other words, the following
must be true for 'gfp' passed to shmem_read_mapping_pages_gfp():
gfp_zone(mapping_gfp_mask(mapping)) == gfp_zone(gfp)
Add a comment to drm_gem_read_pages() explaining that constraint.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
shmem supports page-relocations during swapin since quite some time. It
was implemented in:
commit bde05d1ccd
Author: Hugh Dickins <hughd@google.com>
Date: Tue May 29 15:06:38 2012 -0700
shmem: replace page if mapping excludes its zone
The gem-comment about wrongly placed DMA32 pages is no longer valid.
Replace it with a proper comment but keep the BUG_ON() to verify correct
shmem behavior.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Here's my drm documentation update and driver api polish pull request.
Alex reviewed the entire pile, I've applied a little bit of spelling
polish in a few places since then and otherwise the Usual Suspects (David,
Rob, ...) don't seem up to have another look at it (I've poked them on
irc). So I think it's as good as it gets ;-)
Note that I've dropped the final imx breaker patch since that's blocked on
imx getting sane. Once that's landed I'll ping you to pick up that
straggler.
* 'drm-docs' of ssh://people.freedesktop.org/~danvet/drm: (34 commits)
drm/imx: remove drm_mode_connector_detach_encoder harder
drm: kerneldoc polish for drm_crtc.c
drm: kerneldoc polish for drm_crtc_helper.c
drm: drop error code for drm_helper_resume_force_mode
drm/crtc-helper: remove LOCKING from kerneldoc
drm: remove return value from drm_helper_mode_fill_fb_struct
drm/doc: Fix misplaced </para>
drm: remove drm_display_mode->private_size
drm: polish function kerneldoc for drm_modes.[hc]
drm/modes: drop maxPitch from drm_mode_validate_size
drm/modes: drop return value from drm_display_mode_from_videomode
drm/modes: remove drm_mode_height/width
drm: extract drm_modes.h for drm_crtc.h functions
drm: move drm_mode related functions into drm_modes.c
drm/doc: Repleace LOCKING kerneldoc sections in drm_modes.c
drm/doc: Integrate drm_modes.c kerneldoc
drm/kms: rip out drm_mode_connector_detach_encoder
drm/doc: Add function reference documentation for drm_mm.c
drm/doc: Overview documentation for drm_mm.c
drm/mm: Remove MM_UNUSED_TARGET
...
There is no need to initialize this variable, so drop it. Otherwise, the
compiler won't warn if we use it unintialized.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
All drivers currently need to clean up the vma-node manually. There is no
fancy logic involved so lets just clean it up unconditionally. The
vma-manager correctly catches multiple calls so we are fine.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
At least drm/i915 expects that the obj->dev pointer is set even in
failure paths. Specifically when the shmem initialization fails we
call i915_gem_object_free which needs to deref obj->base.dev to get at
the slab pointer in the device private structure. And the shmem
allocation can easily fail when userspace is hitting open file limits.
Doing the structure init even when the shmem file allocation fails
prevents this Oops.
This is a regression from
commit 89c8233f82
Author: David Herrmann <dh.herrmann@gmail.com>
Date: Thu Jul 11 11:56:32 2013 +0200
drm/gem: simplify object initialization
v2: Add regression note which Chris supplied.
Testcase: igt/gem_fd_exhaustion
Reported-and-Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
References: http://lists.freedesktop.org/archives/intel-gfx/2014-January/038433.html
Cc: stable@vger.kernel.org
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Cc: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
This was hidden in a generic void * dev->mm_private. But only ever
used for gem. But thanks to this fake generic pretension no one
noticed that Rob's drm drivers are now all broken.
So just give the offset manager a type pointer and fix up msm, omapdrm
and tilcdc.
v2: Fixup compile fail.
v3: Fixup rebase fail that David spotted.
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
There's no reason to keep a reference to objects in the name idr. Each
handle to an object has a reference to the object and just before we
destroy the last handle we take the object out of the name idr. Thus,
if an object is in the name idr, there's at least one reference to the
object.
Or to put it another way, the name idr reference will never keep the
object alive. It just looks like it, which is confusing.
Signed-off-by: Kristian Høgsberg <krh@bitplanet.net>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Drivers that don't support PRIME will not have initialized the PRIME
specific private component of struct drm_file. If called for such
drivers, the drm_gem_remove_prime_handles() function will crash. Fix
it by checking for PRIME support prior to removing the PRIME handles.
Signed-off-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Dave Airlie <airlied@gmail.com>
We implement automatic vma mmap() access management for all drivers using
gem_mmap. We use the vma manager to add each open-file that creates a
gem-handle to the vma-node of the underlying gem object. Once the handle
is destroyed, we drop the open-file again.
This allows us to use drm_vma_node_is_allowed() on _any_ gem object to see
whether an open-file is granted access. In drm_gem_mmap() we use this to
verify that unprivileged users cannot guess gem offsets and map arbitrary
buffers.
Note that this manages access for _all_ gem users (also TTM+GEM), but the
actual access checks are only done for drm_gem_mmap(). TTM drivers use the
TTM mmap helpers, which need to do that separately.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
The VMA offset manager uses a device-global address-space. Hence, any
user can currently map any offset-node they want. They only need to guess
the right offset. If we wanted per open-file offset spaces, we'd either
need VM_NONLINEAR mappings or multiple "struct address_space" trees. As
both doesn't really scale, we implement access management in the VMA
manager itself.
We use an rb-tree to store open-files for each VMA node. On each mmap
call, GEM, TTM or the drivers must check whether the current user is
allowed to map this file.
We add a separate lock for each node as there is no generic lock available
for the caller to protect the node easily.
As we currently don't know whether an object may be used for mmap(), we
have to do access management for all objects. If it turns out to slow down
handle creation/deletion significantly, we can optimize it in several
ways:
- Most times only a single filp is added per bo so we could use a static
"struct file *main_filp" which is checked/added/removed first before we
fall back to the rbtree+drm_vma_offset_file.
This could be even done lockless with rcu.
- Let user-space pass a hint whether mmap() should be supported on the
bo and avoid access-management if not.
- .. there are probably more ideas once we have benchmarks ..
v2: add drm_vma_node_verify_access() helper
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
... not only when the dma-buf is freshly created. In contrived
examples someone else could have exported/imported the dma-buf already
and handed us the gem object with a flink name. If such on object gets
reexported as a dma_buf we won't have it in the handle cache already,
which breaks the guarantee that for dma-buf imports we always hand
back an existing handle if there is one.
This is exercised by igt/prime_self_import/with_one_bo_two_files
Now if we extend the locked sections just a notch more we can also
plug th racy buf/handle cache setup in handle_to_fd:
If evil userspace races a concurrent gem close against a prime export
operation we can end up tearing down the gem handle before the dma buf
handle cache is set up. When handle_to_fd gets around to adding the
handle to the cache there will be no one left to clean it up,
effectily leaking the bo (and the dma-buf, since the handle cache
holds a ref on the dma-buf):
Thread A Thread B
handle_to_fd:
lookup gem object from handle
creates new dma_buf
gem_close on the same handle
obj->dma_buf is set, but file priv buf
handle cache has no entry
obj->handle_count drops to 0
drm_prime_add_buf_handle sets up the handle cache
-> We have a dma-buf reference in the handle cache, but since the
handle_count of the gem object already dropped to 0 no on will clean
it up. When closing the drm device fd we'll hit the WARN_ON in
drm_prime_destroy_file_private.
The important change is to extend the critical section of the
filp->prime.lock to cover the gem handle lookup. This serializes with
a concurrent gem handle close.
This leak is exercised by igt/prime_self_import/export-vs-gem_close-race
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
with the reworking semantics and locking of the obj->dma_buf pointer
this pointer is always set as long as there's still a gem handle
around and a dma_buf associated with this gem object.
Also, the per file-priv lookup-cache for dma-buf importing is also
unified between foreign and native objects.
Hence we don't need to special case the clean any more and can simply
drop the clause which only runs for foreing objects, i.e. with
obj->import_attach set.
Note that with this change (actually with the previous one to always
set up obj->dma_buf even for foreign objects) it is no longer required
to set obj->import_attach when importing a foreing object. So update
comments accordingly, too.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
The export dma-buf cache is semantically similar to an flink name. So
semantically it makes sense to treat it the same and remove the name
(i.e. the dma_buf pointer) and its references when the last gem handle
disappears.
Again we need to be careful, but double so: Not just could someone
race and export with a gem close ioctl (so we need to recheck
obj->handle_count again when assigning the new name), but multiple
exports can also race against each another. This is prevented by
holding the dev->object_name_lock across the entire section which
touches obj->dma_buf.
With the new scheme we also need to reinstate the obj->dma_buf link at
import time (in case the only reference userspace has held in-between
was through the dma-buf fd and not through any native gem handle). For
simplicity we don't check whether it's a native object but
unconditionally set up that link - with the new scheme of removing the
obj->dma_buf reference when the last handle disappears we can do that.
To make it clear that this is not just for exported buffers anymore
als rename it from export_dma_buf to dma_buf.
To make sure that now one can race a fd_to_handle or handle_to_fd with
gem_close we use the same tricks as in flink of extending the
dev->object_name_locking critical section. With this change we finally
have a guaranteed 1:1 relationship (at least for native objects)
between gem objects and dma-bufs, even accounting for races (which can
happen since the dma-buf itself holds a reference while in-flight).
This prevent igt/prime_self_import/export-vs-gem_close-race from
Oopsing the kernel. There is still a leak though since the per-file
priv dma-buf/handle cache handling is racy. That will be fixed in a
later patch.
v2: Remove the bogus dma_buf_put from the export_and_register_object
failure path if we've raced with the handle count dropping to 0.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
The gem flink name holds a reference onto the object itself, and this
self-reference would prevent an flink'ed object from every being
freed. To break that loop we remove the flink name when the last
userspace handle disappears, i.e. when obj->handle_count reaches 0.
Now in gem_open we drop the dev->object_name_lock between the flink
name lookup and actually adding the handle. This means a concurrent
gem_close of the last handle could result in the flink name getting
reaped right inbetween, i.e.
Thread 1 Thread 2
gem_open gem_close
flink -> obj lookup
handle_count drops to 0
remove flink name
create_handle
handle_count++
If someone now flinks this object again, we'll get a new flink name.
We can close this race by removing the lock dropping and making the
entire lookup+handle_create sequence atomic. Unfortunately to still be
able to share the handle_create logic this requires a
handle_create_tail function which drops the lock - we can't hold the
object_name_lock while calling into a driver's ->gem_open callback.
Note that for flink fixing this race isn't really important, since
racing gem_open against gem_close is clearly a userspace bug. And no
matter how the race ends, we won't leak any references.
But with dma-buf where the userspace dma-buf fd itself is refcounted
this is a valid sequence and hence we should fix it. Therefore this
patch here is just a warm-up exercise (and for consistency between
flink buffer sharing and dma-buf buffer sharing with self-imports).
Also note that this extension of the critical section in gem_open
protected by dev->object_name_lock only works because it's now a
mutex: A spinlock would conflict with the potential memory allocation
in idr_preload().
This is exercises by igt/gem_flink_race/flink_name.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
I want to wrap the creation of a dma-buf from a gem object in it,
so that the obj->export_dma_buf cache can be atomically filled in.
Instead of creating a new mutex just for that variable I've figured
I can reuse the existing dev->object_name_lock, especially since
the new semantics will exactly mirror the flink obj->name already
protected by that lock.
v2: idr_preload/idr_preload_end is now an atomic section, so need to
move the mutex locking outside.
[airlied: fix up conflict with patch to make debugfs use lock]
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
No one outside of drm should use this, the official interfaces are
drm_gem_handle_create and drm_gem_handle_delete. The handle refcounting
is purely an implementation detail of gem.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
This is the 2nd attempt, I've always been a bit dissatisified with the
tricky nature of the first one:
http://lists.freedesktop.org/archives/dri-devel/2012-July/025451.html
The issue is that the flink ioctl can race with calling gem_close on
the last gem handle. In that case we'll end up with a zero handle
count, but an flink name (and it's corresponding reference). Which
results in a neat space leak.
In my first attempt I've solved this by rechecking the handle count.
But fundamentally the issue is that ->handle_count isn't your usual
refcount - it can be resurrected from 0 among other things.
For those special beasts atomic_t often suggest way more ordering that
it actually guarantees. To prevent being tricked by those hairy
semantics take the easy way out and simply protect the handle with the
existing dev->object_name_lock.
With that change implemented it's dead easy to fix the flink vs. gem
close reace: When we try to create the name we simply have to check
whether there's still officially a gem handle around and if not refuse
to create the flink name. Since the handle count decrement and flink
name destruction is now also protected by that lock the reace is gone
and we can't ever leak the flink reference again.
Outside of the drm core only the exynos driver looks at the handle
count, and tbh I have no idea why (it's just for debug dmesg output
luckily).
I've considered inlining the drm_gem_object_handle_free, but I plan to
add more name-like things (like the exported dma_buf) to this scheme,
so it's clearer to leave the handle freeing in its own function.
This is exercised by the new gem_flink_race i-g-t testcase, which on
my snb leaks gem objects at a rate of roughly 1k objects/s.
v2: Fix up the error path handling in handle_create and make it more
robust by simply calling object_handle_unreference.
v3: Fix up the handle_unreference logic bug - atomic_dec_and_test
retursn 1 for 0. Oops.
v4: Squash in inlining of drm_gem_object_handle_reference as suggested
by Dave Airlie and add a note that we now have a testcase.
Cc: Dave Airlie <airlied@gmail.com>
Cc: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Trying to drop a reference we don't have is a pretty serious bug.
Trying to paper over it is an even worse offense.
So scream into dmesg with a big WARN in case that ever happens.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Calling this function with a NULL object is simply a bug, so papering
over a NULL object not a good idea.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
We have three callers of this function now and it's neither
performance critical nor really small. So an inline function feels
like overkill and unecessarily separates the different parts of the
code.
Since all callers of drm_gem_object_handle_free are now in drm_gem.c
we can make that static (and remove the unused EXPORT_SYMBOL). To
avoid a forward declaration move it (and drm_gem_object_free_bug) up a
bit.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Daniel Vetter
David Herrmann
Dave Airlie
Kristian Hogsberg
Thierry Reding