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]

Message-ID: <20190614125641.GO3436@hirez.programming.kicks-ass.net>
Date:   Fri, 14 Jun 2019 14:56:41 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Christian König 
        <ckoenig.leichtzumerken@...il.com>
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

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 :/

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.