linux-kernel - Re: [PATCH] rwsem: fix missed wakeup due to reordering of load

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] [day] [month] [year] [list]

Message-ID: <20170810104406.zgpatji2ex2tgre6@hirez.programming.kicks-ass.net>
Date:   Thu, 10 Aug 2017 12:44:06 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Prateek Sood <prsood@...eaurora.org>
Cc:     mingo@...hat.com, sramana@...eaurora.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rwsem: fix missed wakeup due to reordering of load

On Thu, Jul 27, 2017 at 01:47:52AM +0530, Prateek Sood wrote:
> diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
> index 34e727f..21c111a 100644
> --- a/kernel/locking/rwsem-xadd.c
> +++ b/kernel/locking/rwsem-xadd.c
> @@ -585,6 +585,40 @@ struct rw_semaphore *rwsem_wake(struct rw_semaphore *sem)
>  	unsigned long flags;
>  	DEFINE_WAKE_Q(wake_q);
>  
> +        /*
> +         * If a spinner is present, there is a chance that the load of
> +         * rwsem_has_spinner() in rwsem_wake() can be reordered with
> +         * respect to decrement of rwsem count in __up_write() leading
> +         * to wakeup being missed.
> +         *
> +         * spinning writer                  up_write caller
> +         * ---------------                  -----------------------
> +         * [S] osq_unlock()                 [L] osq
> +         *  spin_lock(wait_lock)
> +         *  sem->count=0xFFFFFFFF00000001
> +         *            +0xFFFFFFFF00000000
> +         *  count=sem->count
> +         *  MB
> +         *                                   sem->count=0xFFFFFFFE00000001
> +         *                                             -0xFFFFFFFF00000001
> +         *                                   spin_trylock(wait_lock)
> +         *                                   return
> +         * rwsem_try_write_lock(count)
> +         * spin_unlock(wait_lock)
> +         * schedule()
> +         *
> +         * Reordering of atomic_long_sub_return_release() in __up_write()
> +         * and rwsem_has_spinner() in rwsem_wake() can cause missing of
> +         * wakeup in up_write() context. In spinning writer, sem->count
> +         * and local variable count is 0XFFFFFFFE00000001. It would result
> +         * in rwsem_try_write_lock() failing to acquire rwsem and spinning
> +         * writer going to sleep in rwsem_down_write_failed().
> +         *
> +         * The smp_rmb() here is to make sure that the spinner state is
> +         * consulted after sem->count is updated in up_write context.

I feel that comment can use help.. for example the RMB you add below is
not present at all.

> +         */
> +        smp_rmb();
> +
>  	/*
>  	 * If a spinner is present, it is not necessary to do the wakeup.
>  	 * Try to do wakeup only if the trylock succeeds to minimize

Your patch is whitespace damaged, all the indentation on the + lines is
with spaces. Please resend with \t.