linux-kernel - Re: [PATCH 1/6] locking: add ww_mutex_(un)lock_for

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:   Fri, 14 Jun 2019 15:04:13 +0200
From:   Christian König <ckoenig.leichtzumerken@...il.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     daniel@...ll.ch, l.stach@...gutronix.de,
        linux+etnaviv@...linux.org.uk, christian.gmeiner@...il.com,
        yuq825@...il.com, eric@...olt.net, thellstrom@...are.com,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        etnaviv@...ts.freedesktop.org, lima@...ts.freedesktop.org
Subject: Re: [PATCH 1/6] locking: add ww_mutex_(un)lock_for_each helpers

Am 14.06.19 um 14:56 schrieb Peter Zijlstra:
> On Fri, Jun 14, 2019 at 02:41:20PM +0200, Christian König wrote:
>> The ww_mutex implementation allows for detection deadlocks when multiple
>> threads try to acquire the same set of locks in different order.
>>
>> The problem is that handling those deadlocks was the burden of the user of
>> the ww_mutex implementation and at least some users didn't got that right
>> on the first try.
>>
>> I'm not a big fan of macros, but it still better then implementing the same
>> logic at least halve a dozen times. So this patch adds two macros to lock
>> and unlock multiple ww_mutex instances with the necessary deadlock handling.
>>
>> Signed-off-by: Christian König <christian.koenig@....com>
>> ---
>>   include/linux/ww_mutex.h | 75 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 75 insertions(+)
>>
>> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
>> index 3af7c0e03be5..a0d893b5b114 100644
>> --- a/include/linux/ww_mutex.h
>> +++ b/include/linux/ww_mutex.h
>> @@ -369,4 +369,79 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>>   	return mutex_is_locked(&lock->base);
>>   }
>>   
>> +/**
>> + * ww_mutex_unlock_for_each - cleanup after error or contention
>> + * @loop: for loop code fragment iterating over all the locks
>> + * @pos: code fragment returning the currently handled lock
>> + * @contended: the last contended ww_mutex or NULL or ERR_PTR
>> + *
>> + * Helper to make cleanup after error or lock contention easier.
>> + * First unlock the last contended lock and then all other locked ones.
>> + */
>> +#define ww_mutex_unlock_for_each(loop, pos, contended)	\
>> +	if (!IS_ERR(contended)) {			\
>> +		if (contended)				\
>> +			ww_mutex_unlock(contended);	\
>> +		contended = (pos);			\
>> +		loop {					\
>> +			if (contended == (pos))		\
>> +				break;			\
>> +			ww_mutex_unlock(pos);		\
>> +		}					\
>> +	}
>> +
>> +/**
>> + * ww_mutex_lock_for_each - implement ww_mutex deadlock handling
>> + * @loop: for loop code fragment iterating over all the locks
>> + * @pos: code fragment returning the currently handled lock
>> + * @contended: ww_mutex pointer variable for state handling
>> + * @ret: int variable to store the return value of ww_mutex_lock()
>> + * @intr: If true ww_mutex_lock_interruptible() is used
>> + * @ctx: ww_acquire_ctx pointer for the locking
>> + *
>> + * This macro implements the necessary logic to lock multiple ww_mutex
>> + * instances. Lock contention with backoff and re-locking is handled by the
>> + * macro so that the loop body only need to handle other errors and
>> + * successfully acquired locks.
>> + *
>> + * With the @loop and @pos code fragment it is possible to apply this logic
>> + * to all kind of containers (array, list, tree, etc...) holding multiple
>> + * ww_mutex instances.
>> + *
>> + * @contended is used to hold the current state we are in. -ENOENT is used to
>> + * signal that we are just starting the handling. -EDEADLK means that the
>> + * current position is contended and we need to restart the loop after locking
>> + * it. NULL means that there is no contention to be handled. Any other value is
>> + * the last contended ww_mutex.
>> + */
>> +#define ww_mutex_lock_for_each(loop, pos, contended, ret, intr, ctx)	\
>> +	for (contended = ERR_PTR(-ENOENT); ({				\
>> +		__label__ relock, next;					\
>> +		ret = -ENOENT;						\
>> +		if (contended == ERR_PTR(-ENOENT))			\
>> +			contended = NULL;				\
>> +		else if (contended == ERR_PTR(-EDEADLK))		\
>> +			contended = (pos);				\
>> +		else							\
>> +			goto next;					\
>> +		loop {							\
>> +			if (contended == (pos))	{			\
>> +				contended = NULL;			\
>> +				continue;				\
>> +			}						\
>> +relock:									\
>> +			ret = !(intr) ? ww_mutex_lock(pos, ctx) :	\
>> +				ww_mutex_lock_interruptible(pos, ctx);	\
>> +			if (ret == -EDEADLK) {				\
>> +				ww_mutex_unlock_for_each(loop, pos,	\
>> +							 contended);	\
>> +				contended = ERR_PTR(-EDEADLK);		\
>> +				goto relock;				\
>> +			}						\
>> +			break;						\
>> +next:									\
>> +			continue;					\
>> +		}							\
>> +	}), ret != -ENOENT;)
>> +
> Yea gawds, that's eyebleeding bad, even for macros :/

Yeah, I actually don't like it that much either.

But we have at least 8 implementations of this and I'm going to add 
number 9 rather soon.

> It also breaks with pretty much all other *for_each*() macros in
> existence by not actually including the loop itself but farming that out
> to an argument statement (@loop), suggesting these really should not be
> called for_each.

How about ww_mutex_lock_multiple? If not feel free to suggest anything.

Also I'm open to suggestions how to approach that differently, but my 
other tries (callback functions mostly) just somehow suck badly as well.

Thanks,
Christian.