linux-kernel - Re: [PATCH] IPMI: fix some RCU problems

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: <20061228195504.GC4501@linux.vnet.ibm.com>
Date:	Thu, 28 Dec 2006 11:55:04 -0800
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Christoph Hellwig <hch@...radead.org>,
	Corey Minyard <minyard@....org>, Andrew Morton <akpm@...l.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Carol Hebert <cah@...ibm.com>,
	OpenIPMI Developers <openipmi-developer@...ts.sourceforge.net>
Subject: Re: [PATCH] IPMI: fix some RCU problems

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@...ux.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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/