Re: [PATCH] mm: Remove pointless might_sleep() in remove_vm_area().

[Andrey Ryabinin <aryabinin@virtuozzo.com>]

[+CC drm folks, see the following threads:
	http://lkml.kernel.org/r/201703232349.BGB95898.QHLVFFOMtFOOJS@I-love.SAKURA.ne.jp
	http://lkml.kernel.org/r/1490352808-7187-1-git-send-email-penguin-kernel@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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>