linux-kernel - Re: [RFC PATCH v2 00/19] virtual-bus

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: <200906060025.57961.rusty@rustcorp.com.au>
Date:	Sat, 6 Jun 2009 00:25:57 +0930
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	paulmck@...ux.vnet.ibm.com
Cc:	Gregory Haskins <gregory.haskins@...il.com>,
	"Michael S. Tsirkin" <mst@...hat.com>, Avi Kivity <avi@...hat.com>,
	Gregory Haskins <ghaskins@...ell.com>,
	linux-kernel@...r.kernel.org, agraf@...e.de, pmullaney@...ell.com,
	pmorreale@...ell.com, anthony@...emonkey.ws,
	netdev@...r.kernel.org, kvm@...r.kernel.org,
	bhutchings@...arflare.com, andi@...stfloor.org, gregkh@...e.de,
	herber@...dor.apana.org.au, chrisw@...s-sol.org,
	shemminger@...tta.com
Subject: Re: [RFC PATCH v2 00/19] virtual-bus

On Fri, 5 Jun 2009 03:00:10 pm Paul E. McKenney wrote:
> On Fri, Jun 05, 2009 at 02:25:01PM +0930, Rusty Russell wrote:
> > +	/* lg->eventfds is RCU-protected */
> > +	preempt_disable();
>
> Suggest changing to rcu_read_lock() to match the synchronize_rcu().

Ah yes, much better.  As I was implementing it I warred with myself since
lguest aims for simplicity above all else.  But since we only ever add things
to the array, RCU probably is simpler.

> > +	for (i = 0; i < cpu->lg->num_eventfds; i++) {
> > +		if (cpu->lg->eventfds[i].addr == cpu->pending_notify) {
> > +			eventfd_signal(cpu->lg->eventfds[i].event, 1);
>
> Shouldn't this be something like the following?
>
> 		p = rcu_dereference(cpu->lg->eventfds);
> 		if (p[i].addr == cpu->pending_notify) {
> 			eventfd_signal(p[i].event, 1);

Hmm, need to read num_eventfds first, too.  It doesn't matter if we get the old
->num_eventfds and the new ->eventfds, but the other way around would be bad.

Here's the inter-diff:

diff --git a/drivers/lguest/lguest_user.c b/drivers/lguest/lguest_user.c
--- a/drivers/lguest/lguest_user.c
+++ b/drivers/lguest/lguest_user.c
@@ -39,18 +39,24 @@ static int break_guest_out(struct lg_cpu
 
 bool send_notify_to_eventfd(struct lg_cpu *cpu)
 {
-	unsigned int i;
+	unsigned int i, num;
+	struct lg_eventfds *eventfds;
+
+	/* Make sure we grab the total number before accessing the array. */
+	cpu->lg->num_eventfds = num;
+	rmb();
 
 	/* lg->eventfds is RCU-protected */
 	rcu_read_lock();
-	for (i = 0; i < cpu->lg->num_eventfds; i++) {
-		if (cpu->lg->eventfds[i].addr == cpu->pending_notify) {
-			eventfd_signal(cpu->lg->eventfds[i].event, 1);
+	eventfds = rcu_dereference(cpu->lg->eventfds);
+	for (i = 0; i < num; i++) {
+		if (eventfds[i].addr == cpu->pending_notify) {
+			eventfd_signal(eventfds[i].event, 1);
 			cpu->pending_notify = 0;
 			break;
 		}
 	}
-	preempt_enable();
+	rcu_read_unlock();
 	return cpu->pending_notify == 0;
 }
 
Thanks!
Rusty.
--
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/