Commit f5ad3991 authored by Paul E. McKenney's avatar Paul E. McKenney
Browse files

srcu: Remove cleanup_srcu_struct_quiesced() 



The cleanup_srcu_struct_quiesced() function was added because NVME
used WQ_MEM_RECLAIM workqueues and SRCU did not, which meant that
NVME workqueues waiting on SRCU workqueues could result in deadlocks
during low-memory conditions.  However, SRCU now also has WQ_MEM_RECLAIM
workqueues, so there is no longer a potential for deadlock.  Furthermore,
it turns out to be extremely hard to use cleanup_srcu_struct_quiesced()
correctly due to the fact that SRCU callback invocation accesses the
srcu_struct structure's per-CPU data area just after callbacks are
invoked.  Therefore, the usual practice of using srcu_barrier() to wait
for callbacks to be invoked before invoking cleanup_srcu_struct_quiesced()
fails because SRCU's callback-invocation workqueue handler might be
delayed, which can result in cleanup_srcu_struct_quiesced() being invoked
(and thus freeing the per-CPU data) before the SRCU's callback-invocation
workqueue handler is finished using that per-CPU data.  Nor is this a
theoretical problem: KASAN emitted use-after-free warnings because of
this problem on actual runs.

In short, NVME can now safely invoke cleanup_srcu_struct(), which
avoids the use-after-free scenario.  And cleanup_srcu_struct_quiesced()
is quite difficult to use safely.  This commit therefore removes
cleanup_srcu_struct_quiesced(), switching its sole user back to
cleanup_srcu_struct().  This effectively reverts the following pair
of commits:

f7194ac3 ("srcu: Add cleanup_srcu_struct_quiesced()")
4317228a ("nvme: Avoid flush dependency in delete controller flow")

Reported-by: default avatarBart Van Assche <bvanassche@acm.org>
Signed-off-by: default avatarPaul E. McKenney <paulmck@linux.ibm.com>
Reviewed-by: default avatarBart Van Assche <bvanassche@acm.org>
Tested-by: default avatarBart Van Assche <bvanassche@acm.org>
parent 5cdfd174

drivers/nvme/host/core.c

+1 −1
Original line number Diff line number Diff line
@@ -388,7 +388,7 @@ static void nvme_free_ns_head(struct kref *ref) 
	nvme_mpath_remove_disk(head);

	ida_simple_remove(&head->subsys->ns_ida, head->instance);

	list_del_init(&head->entry);

	cleanup_srcu_struct_quiesced(&head->srcu);

	cleanup_srcu_struct(&head->srcu);

	nvme_put_subsystem(head->subsys);

	kfree(head);

}

include/linux/srcu.h

+1 −35
Original line number Diff line number Diff line
@@ -56,45 +56,11 @@ struct srcu_struct { }; 


void call_srcu(struct srcu_struct *ssp, struct rcu_head *head,

		void (*func)(struct rcu_head *head));

void _cleanup_srcu_struct(struct srcu_struct *ssp, bool quiesced);

void cleanup_srcu_struct(struct srcu_struct *ssp);

int __srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp);

void __srcu_read_unlock(struct srcu_struct *ssp, int idx) __releases(ssp);

void synchronize_srcu(struct srcu_struct *ssp);



/**

 * cleanup_srcu_struct - deconstruct a sleep-RCU structure

 * @ssp: structure to clean up.

 *

 * Must invoke this after you are finished using a given srcu_struct that

 * was initialized via init_srcu_struct(), else you leak memory.

 */

static inline void cleanup_srcu_struct(struct srcu_struct *ssp)

{

	_cleanup_srcu_struct(ssp, false);

}



/**

 * cleanup_srcu_struct_quiesced - deconstruct a quiesced sleep-RCU structure

 * @ssp: structure to clean up.

 *

 * Must invoke this after you are finished using a given srcu_struct that

 * was initialized via init_srcu_struct(), else you leak memory.  Also,

 * all grace-period processing must have completed.

 *

 * "Completed" means that the last synchronize_srcu() and

 * synchronize_srcu_expedited() calls must have returned before the call

 * to cleanup_srcu_struct_quiesced().  It also means that the callback

 * from the last call_srcu() must have been invoked before the call to

 * cleanup_srcu_struct_quiesced(), but you can use srcu_barrier() to help

 * with this last.  Violating these rules will get you a WARN_ON() splat

 * (with high probability, anyway), and will also cause the srcu_struct

 * to be leaked.

 */

static inline void cleanup_srcu_struct_quiesced(struct srcu_struct *ssp)

{

	_cleanup_srcu_struct(ssp, true);

}



#ifdef CONFIG_DEBUG_LOCK_ALLOC



/**

kernel/rcu/rcutorture.c

+1 −6
Original line number Diff line number Diff line
@@ -592,12 +592,7 @@ static void srcu_torture_init(void) 


static void srcu_torture_cleanup(void)

{

	static DEFINE_TORTURE_RANDOM(rand);



	if (torture_random(&rand) & 0x800)

	cleanup_srcu_struct(&srcu_ctld);

	else

		cleanup_srcu_struct_quiesced(&srcu_ctld);

	srcu_ctlp = &srcu_ctl; /* In case of a later rcutorture run. */

}

kernel/rcu/srcutiny.c

+3 −6
Original line number Diff line number Diff line
@@ -76,19 +76,16 @@ EXPORT_SYMBOL_GPL(init_srcu_struct); 
 * Must invoke this after you are finished using a given srcu_struct that

 * was initialized via init_srcu_struct(), else you leak memory.

 */

void _cleanup_srcu_struct(struct srcu_struct *ssp, bool quiesced)

void cleanup_srcu_struct(struct srcu_struct *ssp)

{

	WARN_ON(ssp->srcu_lock_nesting[0] || ssp->srcu_lock_nesting[1]);

	if (quiesced)

		WARN_ON(work_pending(&ssp->srcu_work));

	else

	flush_work(&ssp->srcu_work);

	WARN_ON(ssp->srcu_gp_running);

	WARN_ON(ssp->srcu_gp_waiting);

	WARN_ON(ssp->srcu_cb_head);

	WARN_ON(&ssp->srcu_cb_head != ssp->srcu_cb_tail);

}

EXPORT_SYMBOL_GPL(_cleanup_srcu_struct);

EXPORT_SYMBOL_GPL(cleanup_srcu_struct);



/*

 * Removes the count for the old reader from the appropriate element of

kernel/rcu/srcutree.c

+12 −18
Original line number Diff line number Diff line
@@ -360,8 +360,14 @@ static unsigned long srcu_get_delay(struct srcu_struct *ssp) 
	return SRCU_INTERVAL;

}



/* Helper for cleanup_srcu_struct() and cleanup_srcu_struct_quiesced(). */

void _cleanup_srcu_struct(struct srcu_struct *ssp, bool quiesced)

/**

 * cleanup_srcu_struct - deconstruct a sleep-RCU structure

 * @ssp: structure to clean up.

 *

 * Must invoke this after you are finished using a given srcu_struct that

 * was initialized via init_srcu_struct(), else you leak memory.

 */

void cleanup_srcu_struct(struct srcu_struct *ssp)

{

	int cpu;
@@ -369,24 +375,12 @@ void _cleanup_srcu_struct(struct srcu_struct *ssp, bool quiesced) 
		return; /* Just leak it! */

	if (WARN_ON(srcu_readers_active(ssp)))

		return; /* Just leak it! */

	if (quiesced) {

		if (WARN_ON(delayed_work_pending(&ssp->work)))

			return; /* Just leak it! */

	} else {

	flush_delayed_work(&ssp->work);

	}

	for_each_possible_cpu(cpu) {

		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);



		if (quiesced) {

			if (WARN_ON(timer_pending(&sdp->delay_work)))

				return; /* Just leak it! */

			if (WARN_ON(work_pending(&sdp->work)))

				return; /* Just leak it! */

		} else {

		del_timer_sync(&sdp->delay_work);

		flush_work(&sdp->work);

		}

		if (WARN_ON(rcu_segcblist_n_cbs(&sdp->srcu_cblist)))

			return; /* Forgot srcu_barrier(), so just leak it! */

	}
@@ -399,7 +393,7 @@ void _cleanup_srcu_struct(struct srcu_struct *ssp, bool quiesced) 
	free_percpu(ssp->sda);

	ssp->sda = NULL;

}

EXPORT_SYMBOL_GPL(_cleanup_srcu_struct);

EXPORT_SYMBOL_GPL(cleanup_srcu_struct);



/*

 * Counts the new reader in the appropriate per-CPU element of the

CRA Git | Maintained and supported by SUSTech CRA and CCSE