[+CC drm folks, see the following threads:
http://lkml.kernel.org/r/[email protected]
http://lkml.kernel.org/r/1490352808-7187-1-git-send-email-penguin-ker...@i-love.sakura.ne.jp
]
On 03/24/2017 07:17 PM, Matthew Wilcox wrote:
> On Fri, Mar 24, 2017 at 06:05:45PM +0300, Andrey Ryabinin wrote:
>> Just fix the drm code. There is zero point in releasing memory under
>> spinlock.
>
> I disagree. The spinlock has to be held while deleting from the hash
> table.
And what makes you think so?
There are too places where spinlock held during drm_ht_remove();
1) The first one is an obvious crap in ttm_object_device_release():
void ttm_object_device_release(struct ttm_object_device **p_tdev)
{
struct ttm_object_device *tdev = *p_tdev;
*p_tdev = NULL;
spin_lock(&tdev->object_lock);
drm_ht_remove(&tdev->object_hash);
spin_unlock(&tdev->object_lock);
kfree(tdev);
}
Obviously this spin_lock has no use here and it can be removed. There should
be no concurrent access to tdev at this point, because that would mean immediate
use-afte-free.
2) The second case is in ttm_object_file_release() calls drm_ht_remove() under
tfile->lock
And drm_ht_remove() does:
void drm_ht_remove(struct drm_open_hash *ht)
{
if (ht->table) {
kvfree(ht->table);
ht->table = NULL;
}
}
Let's assume that we have some other code accessing ht->table and racing
against ttm_object_file_release()->drm_ht_remove().
This would mean that such code must do the following:
a) take spin_lock(&tfile->lock)
b) check ht->table for being non-NULL and only after that it can dereference
ht->table.
But I don't see any code checking ht->table for NULL. So if race against
drm_ht_remove()
is possible, this code is already broken and this spin_lock doesn't save us
from NULL-ptr
deref.
So, either we already protected from such scenarios (e.g. we are the only
owners of tdev/tfile in
ttm_object_device_release()/ttm_object_file_release()) or this code is already
terribly
broken. Anyways we can just move drm_ht_remove() out of
spin_lock()/spin_unlock() section.
Did I miss anything?
> Sure, we could change the API to return the object removed, and
> then force the caller to free the object that was removed from the hash
> table outside the lock it's holding, but that's a really inelegant API.
>
This won't be required if I'm right.
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
