| 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
| ||
|
Date: Wed, 19 May 2021 12:12:51 -0400
From: Waiman Long <llong@...hat.com>
To: Peter Zijlstra <peterz@...radead.org>, qiang.zhang@...driver.com
Cc: mingo@...hat.com, will@...nel.org, boqun.feng@...il.com,
linux-kernel@...r.kernel.org, maarten.lankhorst@...onical.com
Subject: Re: [PATCH v3] locking/mutex: clear MUTEX_FLAGS if wait_list is empty
due to signal
On 5/18/21 6:49 AM, Peter Zijlstra wrote:
> On Mon, May 17, 2021 at 11:40:05AM +0800, qiang.zhang@...driver.com wrote:
>> From: Zqiang <qiang.zhang@...driver.com>
>>
>> When a interruptible mutex locker is interrupted by a signal
>> without acquiring this lock and removed from the wait queue.
>> if the mutex isn't contended enough to have a waiter
>> put into the wait queue again, the setting of the WAITER
>> bit will force mutex locker to go into the slowpath to
>> acquire the lock every time, so if the wait queue is empty,
>> the WAITER bit need to be clear.
> I'm still interestd in knowing how you found this. Did you have an
> actual problem, or were you just reading the code?
>
> AFAICT, this needs:
>
> Fixes: 040a0a371005 ("mutex: Add support for wound/wait style locks")
>
>> Suggested-by: Peter Zijlstra <peterz@...radead.org>
>> Signed-off-by: Zqiang <qiang.zhang@...driver.com>
> Thanks!
>
> Updated patch below.
>
> ---
> Subject: locking/mutex: clear MUTEX_FLAGS if wait_list is empty due to signal
> From: Zqiang <qiang.zhang@...driver.com>
> Date: Mon, 17 May 2021 11:40:05 +0800
>
> From: Zqiang <qiang.zhang@...driver.com>
>
> When a interruptible mutex locker is interrupted by a signal
> without acquiring this lock and removed from the wait queue.
> if the mutex isn't contended enough to have a waiter
> put into the wait queue again, the setting of the WAITER
> bit will force mutex locker to go into the slowpath to
> acquire the lock every time, so if the wait queue is empty,
> the WAITER bit need to be clear.
>
> Fixes: 040a0a371005 ("mutex: Add support for wound/wait style locks")
> Suggested-by: Peter Zijlstra <peterz@...radead.org>
> Signed-off-by: Zqiang <qiang.zhang@...driver.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> Link: https://lkml.kernel.org/r/20210517034005.30828-1-qiang.zhang@windriver.com
> ---
> kernel/locking/mutex-debug.c | 4 ++--
> kernel/locking/mutex-debug.h | 2 +-
> kernel/locking/mutex.c | 18 +++++++++++++-----
> kernel/locking/mutex.h | 4 +---
> 4 files changed, 17 insertions(+), 11 deletions(-)
>
> --- a/kernel/locking/mutex-debug.c
> +++ b/kernel/locking/mutex-debug.c
> @@ -57,7 +57,7 @@ void debug_mutex_add_waiter(struct mutex
> task->blocked_on = waiter;
> }
>
> -void mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
> +void debug_mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
> struct task_struct *task)
> {
> DEBUG_LOCKS_WARN_ON(list_empty(&waiter->list));
> @@ -65,7 +65,7 @@ void mutex_remove_waiter(struct mutex *l
> DEBUG_LOCKS_WARN_ON(task->blocked_on != waiter);
> task->blocked_on = NULL;
>
> - list_del_init(&waiter->list);
> + INIT_LIST_HEAD(&waiter->list);
> waiter->task = NULL;
> }
>
> --- a/kernel/locking/mutex-debug.h
> +++ b/kernel/locking/mutex-debug.h
> @@ -22,7 +22,7 @@ extern void debug_mutex_free_waiter(stru
> extern void debug_mutex_add_waiter(struct mutex *lock,
> struct mutex_waiter *waiter,
> struct task_struct *task);
> -extern void mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
> +extern void debug_mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter,
> struct task_struct *task);
> extern void debug_mutex_unlock(struct mutex *lock);
> extern void debug_mutex_init(struct mutex *lock, const char *name,
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -194,7 +194,7 @@ static inline bool __mutex_waiter_is_fir
> * Add @waiter to a given location in the lock wait_list and set the
> * FLAG_WAITERS flag if it's the first waiter.
> */
> -static void __sched
> +static void
> __mutex_add_waiter(struct mutex *lock, struct mutex_waiter *waiter,
> struct list_head *list)
> {
> @@ -205,6 +205,16 @@ __mutex_add_waiter(struct mutex *lock, s
> __mutex_set_flag(lock, MUTEX_FLAG_WAITERS);
> }
>
> +static void
> +__mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter)
> +{
> + list_del(&waiter->list);
> + if (likely(list_empty(&lock->wait_list)))
> + __mutex_clear_flag(lock, MUTEX_FLAGS);
> +
> + debug_mutex_remove_waiter(lock, waiter, current);
> +}
> +
> /*
> * Give up ownership to a specific task, when @task = NULL, this is equivalent
> * to a regular unlock. Sets PICKUP on a handoff, clears HANDOFF, preserves
> @@ -1061,9 +1071,7 @@ __mutex_lock_common(struct mutex *lock,
> __ww_mutex_check_waiters(lock, ww_ctx);
> }
>
> - mutex_remove_waiter(lock, &waiter, current);
> - if (likely(list_empty(&lock->wait_list)))
> - __mutex_clear_flag(lock, MUTEX_FLAGS);
> + __mutex_remove_waiter(lock, &waiter);
>
> debug_mutex_free_waiter(&waiter);
>
> @@ -1080,7 +1088,7 @@ __mutex_lock_common(struct mutex *lock,
>
> err:
> __set_current_state(TASK_RUNNING);
> - mutex_remove_waiter(lock, &waiter, current);
> + __mutex_remove_waiter(lock, &waiter);
> err_early_kill:
> spin_unlock(&lock->wait_lock);
> debug_mutex_free_waiter(&waiter);
> --- a/kernel/locking/mutex.h
> +++ b/kernel/locking/mutex.h
> @@ -10,12 +10,10 @@
> * !CONFIG_DEBUG_MUTEXES case. Most of them are NOPs:
> */
>
> -#define mutex_remove_waiter(lock, waiter, task) \
> - __list_del((waiter)->list.prev, (waiter)->list.next)
> -
> #define debug_mutex_wake_waiter(lock, waiter) do { } while (0)
> #define debug_mutex_free_waiter(waiter) do { } while (0)
> #define debug_mutex_add_waiter(lock, waiter, ti) do { } while (0)
> +#define debug_mutex_remove_waiter(lock, waiter, ti) do { } while (0)
> #define debug_mutex_unlock(lock) do { } while (0)
> #define debug_mutex_init(lock, name, key) do { } while (0)
>
>
Reviewed-by: Waiman Long <longman@...hat.com>
Powered by blists - more mailing lists