linux-kernel - Re: [PATCH v2 tip/core/rcu 04/39] srcu: Check for tardy grace-period activity in cleanup_srcu

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:   Mon, 17 Apr 2017 17:33:32 -0700
From:   Josh Triplett <josh@...htriplett.org>
To:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:     linux-kernel@...r.kernel.org, mingo@...nel.org,
        jiangshanlai@...il.com, dipankar@...ibm.com,
        akpm@...ux-foundation.org, mathieu.desnoyers@...icios.com,
        tglx@...utronix.de, peterz@...radead.org, rostedt@...dmis.org,
        dhowells@...hat.com, edumazet@...gle.com, fweisbec@...il.com,
        oleg@...hat.com, bobby.prani@...il.com
Subject: Re: [PATCH v2 tip/core/rcu 04/39] srcu: Check for tardy grace-period
 activity in cleanup_srcu_struct()

On Mon, Apr 17, 2017 at 04:44:51PM -0700, Paul E. McKenney wrote:
> Users of SRCU are obliged to complete all grace-period activity before
> invoking cleanup_srcu_struct().  This means that all calls to either
> synchronize_srcu() or synchronize_srcu_expedited() must have returned,
> and all calls to call_srcu() must have returned, and the last call to
> call_srcu() must have been followed by a call to srcu_barrier().
> Furthermore, the caller must have done something to prevent any
> further calls to synchronize_srcu(), synchronize_srcu_expedited(),
> and call_srcu().
> 
> Therefore, if there has ever been an invocation of call_srcu() on
> the srcu_struct in question, the sequence of events must be as
> follows:
> 
> 1.  Prevent any further calls to call_srcu().
> 2.  Wait for any pre-existing call_srcu() invocations to return.
> 3.  Invoke srcu_barrier().
> 4.  It is now safe to invoke cleanup_srcu_struct().
> 
> On the other hand, if there has ever been a call to synchronize_srcu()
> or synchronize_srcu_expedited(), the sequence of events must be as
> follows:
> 
> 1.  Prevent any further calls to synchronize_srcu() or
>     synchronize_srcu_expedited().
> 2.  Wait for any pre-existing synchronize_srcu() or
>     synchronize_srcu_expedited() invocations to return.
> 3.  It is now safe to invoke cleanup_srcu_struct().
> 
> If there have been calls to all both types of functions (call_srcu()
> and either of synchronize_srcu() and synchronize_srcu_expedited()), then
> the caller must do the first three steps of the call_srcu() procedure
> above and the first two steps of the synchronize_s*() procedure above,
> and only then invoke cleanup_srcu_struct().

This commit message clearly explains the correct sequence for the
client, but not which aspects of this the change now enforces.  Some of
the steps above remain the responsibility of the caller, while the
callee now checks more of them.  Could you add something at the end
explaining the change and what it enforces?

> Reported-by: Paolo Bonzini <pbonzini@...hat.com>
> Signed-off-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>

With the above change:
Reviewed-by: Josh Triplett <josh@...htriplett.org>

>  kernel/rcu/srcu.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/rcu/srcu.c b/kernel/rcu/srcu.c
> index ba41a5d04b49..6beeba7b0b67 100644
> --- a/kernel/rcu/srcu.c
> +++ b/kernel/rcu/srcu.c
> @@ -261,6 +261,11 @@ void cleanup_srcu_struct(struct srcu_struct *sp)
>  {
>  	if (WARN_ON(srcu_readers_active(sp)))
>  		return; /* Leakage unless caller handles error. */
> +	if (WARN_ON(!rcu_all_batches_empty(sp)))
> +		return; /* Leakage unless caller handles error. */
> +	flush_delayed_work(&sp->work);
> +	if (WARN_ON(sp->running))
> +		return; /* Caller forgot to stop doing call_srcu()? */
>  	free_percpu(sp->per_cpu_ref);
>  	sp->per_cpu_ref = NULL;
>  }
> -- 
> 2.5.2
>