linux-kernel - Re: Converting dev->mutex into dev->spinlock ?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <827177aa-bb64-87a9-e1af-dfe070744045@I-love.SAKURA.ne.jp>
Date:   Mon, 6 Feb 2023 23:13:38 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        USB list <linux-usb@...r.kernel.org>,
        Hillf Danton <hdanton@...a.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: Converting dev->mutex into dev->spinlock ?

On 2023/02/05 10:23, Alan Stern wrote:
> I suppose we could create separate lockdep classes for every bus_type 
> and device_type combination, as well as for the different sorts of 
> devices -- treat things like class devices separately from normal 
> devices, and so on.  But even then there would be trouble.

Sorry, since I'm not familiar with devices, I can't interpret what you
are talking about in this response. But why don't you try test5() approach
in an example module shown below (i.e. treat all dev->mutex instances
independent to each other) ?

Sharing mutex_init() (like test2() approach) causes false positives,
but allocating a key on each dev->mutex (like test5() approach) should
avoid false positives.

----------
#include <linux/module.h>
#include <linux/slab.h>

static struct mutex *alloc_mutex(void)
{
	return kzalloc(sizeof(struct mutex), GFP_KERNEL | __GFP_NOFAIL);
}

static void free_mutex(struct mutex *m)
{
	kfree(m);
}

static void test1(void)
{
	struct mutex *parent_mutex;
	struct mutex *peer_mutex[4];
	int i;

	pr_info("Running %s\n", __func__);
	parent_mutex = alloc_mutex();
	mutex_init(parent_mutex);

	peer_mutex[0] = alloc_mutex();
	mutex_init(peer_mutex[0]);
	peer_mutex[1] = alloc_mutex();
	mutex_init(peer_mutex[1]);
	peer_mutex[2] = alloc_mutex();
	mutex_init(peer_mutex[2]);
	peer_mutex[3] = alloc_mutex();
	mutex_init(peer_mutex[3]);

	mutex_lock(parent_mutex);
	for (i = 0; i < 4; i++)
		mutex_lock(peer_mutex[i]);
	for (i = 0; i < 4; i++)
		mutex_unlock(peer_mutex[i]);
	mutex_unlock(parent_mutex);
	for (i = 0; i < 4; i++)
		free_mutex(peer_mutex[i]);
	free_mutex(parent_mutex);
}

static void test2(void)
{
	struct mutex *parent_mutex;
	struct mutex *peer_mutex[4];
	int i;

	pr_info("Running %s\n", __func__);
	parent_mutex = alloc_mutex();
	mutex_init(parent_mutex);
	for (i = 0; i < 4; i++) {
		peer_mutex[i] = alloc_mutex();
		mutex_init(peer_mutex[i]);
	}
	mutex_lock(parent_mutex);
	for (i = 0; i < 4; i++)
		mutex_lock(peer_mutex[i]);
	for (i = 0; i < 4; i++)
		mutex_unlock(peer_mutex[i]);
	mutex_unlock(parent_mutex);
	for (i = 0; i < 4; i++)
		free_mutex(peer_mutex[i]);
	free_mutex(parent_mutex);
}

static void test3(void)
{
	struct mutex *parent_mutex;
	struct mutex *peer_mutex[4];
	int i;

	pr_info("Running %s\n", __func__);
	parent_mutex = alloc_mutex();
	mutex_init(parent_mutex);
	for (i = 0; i < 4; i++) {
		peer_mutex[i] = alloc_mutex();
		switch (i) {
		case 0:
			mutex_init(peer_mutex[i]);
			break;
		case 1:
			mutex_init(peer_mutex[i]);
			break;
		case 2:
			mutex_init(peer_mutex[i]);
			break;
		case 3:
			mutex_init(peer_mutex[i]);
			break;
		}
	}
	mutex_lock(parent_mutex);
	for (i = 0; i < 4; i++)
		mutex_lock(peer_mutex[i]);
	for (i = 0; i < 4; i++)
		mutex_unlock(peer_mutex[i]);
	mutex_unlock(parent_mutex);
	for (i = 0; i < 4; i++)
		free_mutex(peer_mutex[i]);
	free_mutex(parent_mutex);
}

static void test4(void)
{
	struct mutex *parent_mutex;
	struct mutex *peer_mutex[4];
	int i;

	pr_info("Running %s\n", __func__);
	parent_mutex = alloc_mutex();
	mutex_init(parent_mutex);
	for (i = 0; i < 4; i++) {
		peer_mutex[i] = alloc_mutex();
		switch (i) {
		case 0:
			mutex_init(peer_mutex[i]);
			break;
		case 1:
			mutex_init(peer_mutex[i]);
			break;
		case 2:
			mutex_init(peer_mutex[i]);
			break;
		case 3:
			mutex_init(peer_mutex[i]);
			break;
		}
	}
	mutex_lock(parent_mutex);
	for (i = 0; i < 4; i++)
		mutex_lock(peer_mutex[i]);
	for (i = 0; i < 4; i++)
		mutex_unlock(peer_mutex[i]);
	for (i = 3; i >= 0; i--)
		mutex_lock(peer_mutex[i]);
	for (i = 3; i >= 0; i--)
		mutex_unlock(peer_mutex[i]);
	mutex_unlock(parent_mutex);
	for (i = 0; i < 4; i++)
		free_mutex(peer_mutex[i]);
	free_mutex(parent_mutex);
}

struct mutex2 {
	struct mutex mutex;
	struct lock_class_key key;
};

static struct mutex2 *alloc_mutex2(void)
{
	struct mutex2 *m = kzalloc(sizeof(struct mutex2), GFP_KERNEL | __GFP_NOFAIL);

	lockdep_register_key(&m->key);
	mutex_init(&m->mutex);
	lockdep_set_class(&m->mutex, &m->key);
	return m;
}

static void free_mutex2(struct mutex2 *m)
{
	mutex_destroy(&m->mutex);
	lockdep_unregister_key(&m->key);
	kfree(m);
}

static void test5(void)
{
	struct mutex2 *parent_mutex;
	struct mutex2 *peer_mutex[4];
	int i;

	pr_info("Running %s\n", __func__);
	parent_mutex = alloc_mutex2();
	for (i = 0; i < 4; i++)
		peer_mutex[i] = alloc_mutex2();
	mutex_lock(&parent_mutex->mutex);
	for (i = 0; i < 4; i++)
		mutex_lock(&peer_mutex[i]->mutex);
	for (i = 0; i < 4; i++)
		mutex_unlock(&peer_mutex[i]->mutex);
	mutex_unlock(&parent_mutex->mutex);
	for (i = 0; i < 4; i++)
		free_mutex2(peer_mutex[i]);
	free_mutex2(parent_mutex);
}

static void test6(void)
{
	struct mutex2 *parent_mutex;
	struct mutex2 *peer_mutex[4];
	int i;

	pr_info("Running %s\n", __func__);
	parent_mutex = alloc_mutex2();
	for (i = 0; i < 4; i++)
		peer_mutex[i] = alloc_mutex2();
	mutex_lock(&parent_mutex->mutex);
	for (i = 0; i < 4; i++)
		mutex_lock(&peer_mutex[i]->mutex);
	for (i = 0; i < 4; i++)
		mutex_unlock(&peer_mutex[i]->mutex);
	for (i = 3; i >= 0; i--)
		mutex_lock(&peer_mutex[i]->mutex);
	for (i = 3; i >= 0; i--)
		mutex_unlock(&peer_mutex[i]->mutex);
	mutex_unlock(&parent_mutex->mutex);
	for (i = 0; i < 4; i++)
		free_mutex2(peer_mutex[i]);
	free_mutex2(parent_mutex);
}

static int __init lockdep_test_init(void)
{
	if (1)
		test1(); // safe and lockdep does not complain
	if (0)
		test2(); // safe but lockdep complains
	if (1)
		test3(); // safe and lockdep does not complain
	if (0)
		test4(); // unsafe and lockdep complains
	if (1)
		test5(); // safe and lockdep does not complain
	if (0)
		test6(); // unsafe and lockdep complains
        return -EINVAL;
}

module_init(lockdep_test_init);
MODULE_LICENSE("GPL");
----------