linux-kernel - Re: [PATCH v4 3/4] mutex: Add ww tests to lib/locking-selftest.c. v4

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]

Date:	Tue, 28 May 2013 23:12:04 +0200
From:	Maarten Lankhorst <maarten.lankhorst@...onical.com>
To:	linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
	peterz@...radead.org, x86@...nel.org,
	dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org,
	robclark@...il.com, rostedt@...dmis.org, tglx@...utronix.de,
	mingo@...e.hu, linux-media@...r.kernel.org
Subject: Re: [PATCH v4 3/4] mutex: Add ww tests to lib/locking-selftest.c.
 v4

Op 28-05-13 21:18, Daniel Vetter schreef:
> On Tue, May 28, 2013 at 04:48:45PM +0200, Maarten Lankhorst wrote:
>> This stresses the lockdep code in some ways specifically useful to
>> ww_mutexes. It adds checks for most of the common locking errors.
>>
>> Changes since v1:
>>  - Add tests to verify reservation_id is untouched.
>>  - Use L() and U() macros where possible.
>>
>> Changes since v2:
>>  - Use the ww_mutex api directly.
>>  - Use macros for most of the code.
>> Changes since v3:
>>  - Rework tests for the api changes.
>>
>> <snip>
>>
>> +static void ww_test_normal(void)
>> +{
>> +	int ret;
>> +
>> +	WWAI(&t);
>> +
>> +	/*
>> +	 * test if ww_id is kept identical if not
>> +	 * called with any of the ww_* locking calls
>> +	 */
>> +
>> +	/* mutex_lock (and indirectly, mutex_lock_nested) */
>> +	o.ctx = (void *)~0UL;
>> +	mutex_lock(&o.base);
>> +	mutex_unlock(&o.base);
>> +	WARN_ON(o.ctx != (void *)~0UL);
>> +
>> +	/* mutex_lock_interruptible (and *_nested) */
>> +	o.ctx = (void *)~0UL;
>> +	ret = mutex_lock_interruptible(&o.base);
>> +	if (!ret)
>> +		mutex_unlock(&o.base);
>> +	else
>> +		WARN_ON(1);
>> +	WARN_ON(o.ctx != (void *)~0UL);
>> +
>> +	/* mutex_lock_killable (and *_nested) */
>> +	o.ctx = (void *)~0UL;
>> +	ret = mutex_lock_killable(&o.base);
>> +	if (!ret)
>> +		mutex_unlock(&o.base);
>> +	else
>> +		WARN_ON(1);
>> +	WARN_ON(o.ctx != (void *)~0UL);
>> +
>> +	/* trylock, succeeding */
>> +	o.ctx = (void *)~0UL;
>> +	ret = mutex_trylock(&o.base);
>> +	WARN_ON(!ret);
>> +	if (ret)
>> +		mutex_unlock(&o.base);
>> +	else
>> +		WARN_ON(1);
>> +	WARN_ON(o.ctx != (void *)~0UL);
>> +
>> +	/* trylock, failing */
>> +	o.ctx = (void *)~0UL;
>> +	mutex_lock(&o.base);
>> +	ret = mutex_trylock(&o.base);
>> +	WARN_ON(ret);
>> +	mutex_unlock(&o.base);
>> +	WARN_ON(o.ctx != (void *)~0UL);
>> +
>> +	/* nest_lock */
>> +	o.ctx = (void *)~0UL;
>> +	mutex_lock_nest_lock(&o.base, &t);
>> +	mutex_unlock(&o.base);
>> +	WARN_ON(o.ctx != (void *)~0UL);
>> +}
> Since we don't really allow this any more (instead allow ww_mutex_lock
> without context) do we need this test here really?
Yes. This test verifies all the normal locking paths are not affected by the ww_ctx changes.

>> +
>> +static void ww_test_two_contexts(void)
>> +{
>> +	WWAI(&t);
>> +	WWAI(&t2);
>> +}
>> +
>> +static void ww_test_context_unlock_twice(void)
>> +{
>> +	WWAI(&t);
>> +	WWAD(&t);
>> +	WWAF(&t);
>> +	WWAF(&t);
>> +}
>> +
>> +static void ww_test_object_unlock_twice(void)
>> +{
>> +	WWL1(&o);
>> +	WWU(&o);
>> +	WWU(&o);
>> +}
>> +
>> +static void ww_test_spin_nest_unlocked(void)
>> +{
>> +	raw_spin_lock_nest_lock(&lock_A, &o.base);
>> +	U(A);
>> +}
> I don't quite see the point of this one here ...
It's a lockdep test that was missing. o.base is not locked. So lock_A is being nested into an unlocked lock, resulting in a lockdep error.

>> +
>> +static void ww_test_unneeded_slow(void)
>> +{
>> +	int ret;
>> +
>> +	WWAI(&t);
>> +
>> +	ww_mutex_lock_slow(&o, &t);
>> +}
> I think checking the _slow debug stuff would be neat, i.e.
> - fail/success tests for properly unlocking all held locks
> - fail/success tests for lock_slow acquiring the right lock.
>
> Otherwise I didn't spot anything that seems missing in these self-tests
> here.
>
Yes it would be nice, doing so is left as an excercise for the reviewer, who failed to raise this point sooner. ;-)

~Maarten
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/