[+CC drm folks, see the following threads: http://lkml.kernel.org/r/201703232349.BGB95898.QHLVFFOMtFOOJS@xxxxxxxxxxxxxxxxxxx http://lkml.kernel.org/r/1490352808-7187-1-git-send-email-penguin-kernel@xxxxxxxxxxxxxxxxxxx ] 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 dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
