linux-kernel - Re: [PATCH 2/4] rwsem: Add missing ACQUIRE to read

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: <5279c745-f703-7037-766e-9874648a5de3@redhat.com>
Date:   Thu, 18 Jul 2019 13:13:26 -0400
From:   Waiman Long <longman@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>,
        Will Deacon <will@...nel.org>
Cc:     linux-kernel@...r.kernel.org, Jan Stancek <jstancek@...hat.com>,
        dbueso@...e.de, mingo@...hat.com, jade.alglave@....com,
        paulmck@...ux.vnet.ibm.com
Subject: Re: [PATCH 2/4] rwsem: Add missing ACQUIRE to read_slowpath sleep
 loop

On 7/18/19 9:49 AM, Peter Zijlstra wrote:
> While reviewing another read_slowpath patch, both Will and I noticed
> another missing ACQUIRE, namely:
>
>   X = 0;
>
>   CPU0			CPU1
>
>   rwsem_down_read()
>     for (;;) {
>       set_current_state(TASK_UNINTERRUPTIBLE);
>
>                         X = 1;
>                         rwsem_up_write();
>                           rwsem_mark_wake()
>                             atomic_long_add(adjustment, &sem->count);
>                             smp_store_release(&waiter->task, NULL);
>
>       if (!waiter.task)
>         break;
>
>       ...
>     }
>
>   r = X;
>
> Allows 'r == 0'.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> Reported-by: Will Deacon <will@...nel.org>
> Acked-by: Will Deacon <will@...nel.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
>  kernel/locking/rwsem.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> --- a/kernel/locking/rwsem.c
> +++ b/kernel/locking/rwsem.c
> @@ -1069,8 +1069,10 @@ rwsem_down_read_slowpath(struct rw_semap
>  	/* wait to be given the lock */
>  	while (true) {
>  		set_current_state(state);
> -		if (!waiter.task)
> +		if (!smp_load_acquire(&waiter.task)) {
> +			/* Orders against rwsem_mark_wake()'s smp_store_release() */
>  			break;
> +		}
>  		if (signal_pending_state(state, current)) {
>  			raw_spin_lock_irq(&sem->wait_lock);
>  			if (waiter.task)
>
>
Acked-by: Waiman Long <longman@...hat.com>

Thanks for fixing this old problem.

This problem does not apply to x86 and we do our testing mostly on x86.
That is why we seldom notice this kind of issue. Maybe we should be
doing more testing on ARM64 and PPC.

Cheers,
Longman