[PATCH] IPMI: fix some RCU problems

[PATCH] IPMI: fix some RCU problems

[Corey Minyard]

Fix some RCU problem pointed out by Paul McKenney of IBM.  These are:

The wholesale move of the command receivers list into a new list was
not safe because the list will point to the new tail during a
traversal, so the traversal will never end on a reader if this happens
during a read.

Memory barriers were needed to handle proper ordering of the setting
of the IPMI interface as valid.  Readers might not see proper ordering
of data otherwise.

In ipmi_smi_watcher_register(), the use of the _rcu suffix on the list
is unnecessary.

Signed-off-by: Corey Minyard <minyard@acm.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Index: linux-2.6.19/drivers/char/ipmi/ipmi_msghandler.c
===================================================================
--- linux-2.6.19.orig/drivers/char/ipmi/ipmi_msghandler.c
+++ linux-2.6.19/drivers/char/ipmi/ipmi_msghandler.c
@@ -406,13 +406,31 @@ static void clean_up_interface_data(ipmi
 	free_smi_msg_list(&intf->waiting_msgs);
 	free_recv_msg_list(&intf->waiting_events);
 
-	/* Wholesale remove all the entries from the list in the
-	 * interface and wait for RCU to know that none are in use. */
+	/*
+	 * Wholesale remove all the entries from the list in the
+	 * interface and wait for RCU to know that none are in use.
+	 */
 	mutex_lock(&intf->cmd_rcvrs_mutex);
-	list_add_rcu(&list, &intf->cmd_rcvrs);
-	list_del_rcu(&intf->cmd_rcvrs);
+	if (list_empty(&intf->cmd_rcvrs))
+		INIT_LIST_HEAD(&list);
+	else {
+		list.next = intf->cmd_rcvrs.next;
+		list.prev = intf->cmd_rcvrs.prev;
+		INIT_LIST_HEAD(&intf->cmd_rcvrs);
+
+		/*
+		 * At this point the list body still points to
+		 * intf->cmd_rcvrs.  Wait for any readers to finish
+		 * using the list before we switch the list body over
+		 * to the new list.
+		 */
+		synchronize_rcu();
+
+		/* Ready the list for use. */
+		list.next->prev = &list;
+		list.prev->next = &list;
+	}
 	mutex_unlock(&intf->cmd_rcvrs_mutex);
-	synchronize_rcu();
 
 	list_for_each_entry_safe(rcvr, rcvr2, &list, link)
 		kfree(rcvr);
@@ -451,7 +469,7 @@ int ipmi_smi_watcher_register(struct ipm
 	mutex_lock(&ipmi_interfaces_mutex);
 
 	/* Build a list of things to deliver. */
-	list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
+	list_for_each_entry(intf, &ipmi_interfaces, link) {
 		if (intf->intf_num == -1)
 			continue;
 		e = kmalloc(sizeof(*e), GFP_KERNEL);
@@ -838,6 +856,7 @@ int ipmi_create_user(unsigned int       
 	goto out_kfree;
 
  found:
+	smp_rmb();
 	/* Note that each existing user holds a refcount to the interface. */
 	kref_get(&intf->refcount);
 
@@ -2761,6 +2780,7 @@ int ipmi_register_smi(struct ipmi_smi_ha
 		kref_put(&intf->refcount, intf_free);
 	} else {
 		/* After this point the interface is legal to use. */
+		smp_wmb(); /* Keep memory order straight for RCU readers. */
 		intf->intf_num = i;
 		mutex_unlock(&ipmi_interfaces_mutex);
 		call_smi_watchers(i, intf->si_dev);
@@ -3924,6 +3944,8 @@ static void send_panic_events(char *str)
 			/* Interface was not ready yet. */
 			continue;
 
+		smp_rmb();
+
 		/* First job here is to figure out where to send the
 		   OEM events.  There's no way in IPMI to send OEM
 		   events using an event send command, so we have to

Re: [PATCH] IPMI: fix some RCU problems

[Christoph Hellwig]

> +	if (list_empty(&intf->cmd_rcvrs))
> +		INIT_LIST_HEAD(&list);
> +	else {
> +		list.next = intf->cmd_rcvrs.next;
> +		list.prev = intf->cmd_rcvrs.prev;
> +		INIT_LIST_HEAD(&intf->cmd_rcvrs);
> +
> +		/*
> +		 * At this point the list body still points to
> +		 * intf->cmd_rcvrs.  Wait for any readers to finish
> +		 * using the list before we switch the list body over
> +		 * to the new list.
> +		 */
> +		synchronize_rcu();
> +
> +		/* Ready the list for use. */
> +		list.next->prev = &list;
> +		list.prev->next = &list;
> +	}

This kind of thing must not be opencoded in drivers.  Please add
a new list_splice_rcu helper to list.h

Re: [PATCH] IPMI: fix some RCU problems

[Paul E. McKenney]

On Thu, Dec 28, 2006 at 06:31:01PM +0000, Christoph Hellwig wrote:
> > +	if (list_empty(&intf->cmd_rcvrs))
> > +		INIT_LIST_HEAD(&list);
> > +	else {
> > +		list.next = intf->cmd_rcvrs.next;
> > +		list.prev = intf->cmd_rcvrs.prev;
> > +		INIT_LIST_HEAD(&intf->cmd_rcvrs);
> > +
> > +		/*
> > +		 * At this point the list body still points to
> > +		 * intf->cmd_rcvrs.  Wait for any readers to finish
> > +		 * using the list before we switch the list body over
> > +		 * to the new list.
> > +		 */
> > +		synchronize_rcu();
> > +
> > +		/* Ready the list for use. */
> > +		list.next->prev = &list;
> > +		list.prev->next = &list;
> > +	}
> 
> This kind of thing must not be opencoded in drivers.  Please add
> a new list_splice_rcu helper to list.h

I must admit that this sounds better than list_privatize_rcu().  But since
the source list gets initialized, so how about list_splice_init_rcu()?
Patch below, untested, probably does not compile.  This takes the sync
function as an argument, so one would do something like:

	INIT_LIST_HEAD(&list);
	list_splice_init_rcu(&intf->cmdrcvrs, &list, synchronize_rcu);

The idea being to keep the RCU API proliferation down to a dull roar.

Thoughts?

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---

 list.h |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff -urpNa -X dontdiff linux-2.6.19/include/linux/list.h linux-2.6.19-lpr/include/linux/list.h
--- linux-2.6.19/include/linux/list.h	2006-11-29 13:57:37.000000000 -0800
+++ linux-2.6.19-lpr/include/linux/list.h	2006-12-28 11:48:31.000000000 -0800
@@ -360,6 +360,64 @@ static inline void list_splice_init(stru
 }
 
 /**
+ * list_splice_init_rcu - splice an RCU-protected list into an existing list.
+ * @list	the RCU-protected list to splice
+ * @head	the place in the list to splice the first list into
+ * @sync	function to sync: synchronize_rcu(), synchronize_sched(), ...
+ *
+ * @head can be RCU-read traversed concurrently with this function.
+ *
+ * Note that this function blocks.
+ *
+ * Important note: the caller must take whatever action is necessary to
+ *	prevent any other updates to @head.  In principle, it is possible
+ *	to modify the list as soon as sync() begins execution.
+ *	If this sort of thing becomes necessary, an alternative version
+ *	based on call_rcu() could be created.  But only if -really-
+ *	needed -- there is no shortage of RCU API members.
+ */
+static inline void list_splice_init_rcu(struct list_head *list,
+					struct list_head *head,
+					void (*sync)(void))
+{
+	struct list_head *first = list->next;
+	struct list_head *last = list->prev;
+	struct list_head *at = head->next;
+
+	might_sleep();
+	if (list_empty(head)) {
+		return;
+	}
+
+	/* "first" and "last" tracking list, so initialize it. */
+
+	INIT_LIST_HEAD(list);
+
+	/*
+	 * At this point, the list body still points to the source list.
+	 * Wait for any readers to finish using the list before splicing
+	 * the list body into the new list.  Any new readers will see
+	 * an empty list.
+	 */
+
+	sync();
+
+	/*
+	 * Readers are finished with the source list, so perform splice.
+	 * The order is important if the new list is global and accessible
+	 * to concurrent RCU readers.  Note that RCU readers are not
+	 * permitted to traverse the prev pointers without excluding
+	 * this function.
+	 */
+
+	last->next = at;
+	smp_wmb();
+	head->next = first;
+	first->prev = head;
+	at->prev = last;
+}
+
+/**
  * list_entry - get the struct for this entry
  * @ptr:	the &struct list_head pointer.
  * @type:	the type of the struct this is embedded in.

Re: [PATCH] IPMI: fix some RCU problems

[Randy Dunlap]

On Thu, 28 Dec 2006 11:55:04 -0800 Paul E. McKenney wrote:

>  list.h |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff -urpNa -X dontdiff linux-2.6.19/include/linux/list.h linux-2.6.19-lpr/include/linux/list.h
> --- linux-2.6.19/include/linux/list.h	2006-11-29 13:57:37.000000000 -0800
> +++ linux-2.6.19-lpr/include/linux/list.h	2006-12-28 11:48:31.000000000 -0800
> @@ -360,6 +360,64 @@ static inline void list_splice_init(stru
>  }
>  
>  /**
> + * list_splice_init_rcu - splice an RCU-protected list into an existing list.
> + * @list	the RCU-protected list to splice
> + * @head	the place in the list to splice the first list into
> + * @sync	function to sync: synchronize_rcu(), synchronize_sched(), ...

@parameter: is kernel-doc syntax.
I.e., please add a colon after each one of those.

---
~Randy

Re: [PATCH] IPMI: fix some RCU problems

[Paul E. McKenney]

On Thu, Dec 28, 2006 at 12:24:22PM -0800, Randy Dunlap wrote:
> On Thu, 28 Dec 2006 11:55:04 -0800 Paul E. McKenney wrote:
> 
> >  list.h |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> >
> > diff -urpNa -X dontdiff linux-2.6.19/include/linux/list.h linux-2.6.19-lpr/include/linux/list.h
> > --- linux-2.6.19/include/linux/list.h	2006-11-29 13:57:37.000000000 -0800
> > +++ linux-2.6.19-lpr/include/linux/list.h	2006-12-28 11:48:31.000000000 -0800
> > @@ -360,6 +360,64 @@ static inline void list_splice_init(stru
> >  }
> >
> >  /**
> > + * list_splice_init_rcu - splice an RCU-protected list into an existing list.
> > + * @list	the RCU-protected list to splice
> > + * @head	the place in the list to splice the first list into
> > + * @sync	function to sync: synchronize_rcu(), synchronize_sched(), ...
> 
> @parameter: is kernel-doc syntax.
> I.e., please add a colon after each one of those.

Good point!  :-/

Fixed below.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---

 list.h |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff -urpNa -X dontdiff linux-2.6.19/include/linux/list.h linux-2.6.19-lpr/include/linux/list.h
--- linux-2.6.19/include/linux/list.h	2006-11-29 13:57:37.000000000 -0800
+++ linux-2.6.19-lpr/include/linux/list.h	2006-12-28 14:21:05.000000000 -0800
@@ -360,6 +360,64 @@ static inline void list_splice_init(stru
 }
 
 /**
+ * list_splice_init_rcu - splice an RCU-protected list into an existing list.
+ * @list:	the RCU-protected list to splice
+ * @head:	the place in the list to splice the first list into
+ * @sync:	function to sync: synchronize_rcu(), synchronize_sched(), ...
+ *
+ * @head can be RCU-read traversed concurrently with this function.
+ *
+ * Note that this function blocks.
+ *
+ * Important note: the caller must take whatever action is necessary to
+ *	prevent any other updates to @head.  In principle, it is possible
+ *	to modify the list as soon as sync() begins execution.
+ *	If this sort of thing becomes necessary, an alternative version
+ *	based on call_rcu() could be created.  But only if -really-
+ *	needed -- there is no shortage of RCU API members.
+ */
+static inline void list_splice_init_rcu(struct list_head *list,
+					struct list_head *head,
+					void (*sync)(void))
+{
+	struct list_head *first = list->next;
+	struct list_head *last = list->prev;
+	struct list_head *at = head->next;
+
+	might_sleep();
+	if (list_empty(head)) {
+		return;
+	}
+
+	/* "first" and "last" tracking list, so initialize it. */
+
+	INIT_LIST_HEAD(list);
+
+	/*
+	 * At this point, the list body still points to the source list.
+	 * Wait for any readers to finish using the list before splicing
+	 * the list body into the new list.  Any new readers will see
+	 * an empty list.
+	 */
+
+	sync();
+
+	/*
+	 * Readers are finished with the source list, so perform splice.
+	 * The order is important if the new list is global and accessible
+	 * to concurrent RCU readers.  Note that RCU readers are not
+	 * permitted to traverse the prev pointers without excluding
+	 * this function.
+	 */
+
+	last->next = at;
+	smp_wmb();
+	head->next = first;
+	first->prev = head;
+	at->prev = last;
+}
+
+/**
  * list_entry - get the struct for this entry
  * @ptr:	the &struct list_head pointer.
  * @type:	the type of the struct this is embedded in.

[PATCH] IPMI: Fix some RCU problems

[Corey Minyard]

Fix some RCU problem pointed out by Paul McKenney of IBM.  These are:

The wholesale move of the command receivers list into a new list was
not safe because the list will point to the new tail during a
traversal, so the traversal will never end on a reader if this happens
during a read.

Memory barriers were needed to handle proper ordering of the setting
of the IPMI interface as valid.  Readers might not see proper ordering
of data otherwise.

In ipmi_smi_watcher_register(), the use of the _rcu suffix on the list
is unnecessary.

This require the list_splice_init_rcu() patch previously posted.

Signed-off-by: Corey Minyard <minyard@acm.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Index: linux-2.6.19/drivers/char/ipmi/ipmi_msghandler.c
===================================================================
--- linux-2.6.19.orig/drivers/char/ipmi/ipmi_msghandler.c	2006-12-30 12:41:15.000000000 -0600
+++ linux-2.6.19/drivers/char/ipmi/ipmi_msghandler.c	2006-12-30 12:43:50.000000000 -0600
@@ -406,13 +406,14 @@
 	free_smi_msg_list(&intf->waiting_msgs);
 	free_recv_msg_list(&intf->waiting_events);
 
-	/* Wholesale remove all the entries from the list in the
-	 * interface and wait for RCU to know that none are in use. */
+	/*
+	 * Wholesale remove all the entries from the list in the
+	 * interface and wait for RCU to know that none are in use.
+	 */
 	mutex_lock(&intf->cmd_rcvrs_mutex);
-	list_add_rcu(&list, &intf->cmd_rcvrs);
-	list_del_rcu(&intf->cmd_rcvrs);
+	INIT_LIST_HEAD(&list);
+	list_splice_init_rcu(&intf->cmd_rcvrs, &list, synchronize_rcu);
 	mutex_unlock(&intf->cmd_rcvrs_mutex);
-	synchronize_rcu();
 
 	list_for_each_entry_safe(rcvr, rcvr2, &list, link)
 		kfree(rcvr);
@@ -451,7 +452,7 @@
 	mutex_lock(&ipmi_interfaces_mutex);
 
 	/* Build a list of things to deliver. */
-	list_for_each_entry_rcu(intf, &ipmi_interfaces, link) {
+	list_for_each_entry(intf, &ipmi_interfaces, link) {
 		if (intf->intf_num == -1)
 			continue;
 		e = kmalloc(sizeof(*e), GFP_KERNEL);
@@ -838,6 +839,7 @@
 	goto out_kfree;
 
  found:
+	smp_rmb();
 	/* Note that each existing user holds a refcount to the interface. */
 	kref_get(&intf->refcount);
 
@@ -2761,6 +2763,7 @@
 		kref_put(&intf->refcount, intf_free);
 	} else {
 		/* After this point the interface is legal to use. */
+		smp_wmb(); /* Keep memory order straight for RCU readers. */
 		intf->intf_num = i;
 		mutex_unlock(&ipmi_interfaces_mutex);
 		call_smi_watchers(i, intf->si_dev);
@@ -3924,6 +3927,8 @@
 			/* Interface was not ready yet. */
 			continue;
 
+		smp_rmb();
+
 		/* First job here is to figure out where to send the
 		   OEM events.  There's no way in IPMI to send OEM
 		   events using an event send command, so we have to

Re: [PATCH] IPMI: Fix some RCU problems

[Andrew Morton]

On Wed, 3 Jan 2007 09:31:30 -0600
Corey Minyard <minyard@acm.org> wrote:

>   found:
> +	smp_rmb();
>  	/* Note that each existing user holds a refcount to the interface. */
>  	kref_get(&intf->refcount);
>  
> @@ -2761,6 +2763,7 @@
>  		kref_put(&intf->refcount, intf_free);
>  	} else {
>  		/* After this point the interface is legal to use. */
> +		smp_wmb(); /* Keep memory order straight for RCU readers. */
>  		intf->intf_num = i;
>  		mutex_unlock(&ipmi_interfaces_mutex);
>  		call_smi_watchers(i, intf->si_dev);
> @@ -3924,6 +3927,8 @@
>  			/* Interface was not ready yet. */
>  			continue;
>  
> +		smp_rmb();
> +

It's nice to always have a comment explaining the use of open-coded
barriers.  Because often the reader is left wondered what on earth it's
barriering against what on earth else.