linux-kernel - Re: [RFC][PATCH 0/5] Optimize percpu-rwsem

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: <20150526191343.GA5794@redhat.com>
Date:	Tue, 26 May 2015 21:13:43 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	Paul McKenney <paulmck@...ux.vnet.ibm.com>,
	Tejun Heo <tj@...nel.org>, Ingo Molnar <mingo@...hat.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	der.herr@...r.at, Davidlohr Bueso <dave@...olabs.net>
Subject: Re: [RFC][PATCH 0/5] Optimize percpu-rwsem

On 05/26, Oleg Nesterov wrote:
>
> On 05/26, Linus Torvalds wrote:
> >
> >
> > We literally have one single percpu-rwsem IN THE WHOLE KERNEL TREE.
> >
> > One.
>
> Well. IIRC Tejun is going to turn signal_struct->group_rwsem into
> percpu-rwsem.
>
> And it can have more users. Say, __sb_start_write/etc  does something
> similar, and last time I checked this code it looked buggy to me.

I have found my old email, see below. Perhaps this code was changed
since 2013 when I sent this email, I didn't verify... but in any
case this logic doesn't look simple, imo it would be nice to rely
on the generic helpers from kernel/locking.

Oleg.

------------------------------------------------------------------------------
When I look at __sb_start_write/etc I am not sure this locking
is correct. OK, __sb_start_write() does:

	percpu_counter_inc();

	mb();

	if (sb->s_writers.frozen)
		abort_and_retry;

freeze_super() does STORE + mb + LOAD in reverse order so either
__sb_start_write() must see SB_FREEZE_WRITE or freeze_super() must
see the change in ->s_writers.counter. This is correct.

Still I am not sure sb_wait_write() can trust percpu_counter_sum(),
because it can also see _other_ changes.

To simplify the discussion, suppose that percpu_counter doesn't have
lock/count/batch/whatever and  inc/dec/sum only uses "__percpu *counters".
Lets denote sb->s_writers.counter[level] as CTR[cpu].

Suppose that some thread did __sb_start_write() on CPU_1 and sleeps
"forever". CTR[0] == 0, CTR_[1] == 1, freezer_super() should block.

Now:

	1. freeze_super() sets SB_FREEZE_WRITE, does mb(), and
	   starts sb_wait_write()->percpu_counter_sum().

	2. __percpu_counter_sum() does for_each_online_cpu(),
	   reads CTR[0] == 0. ret = 0.

	3. Another thread comes, calls __sb_start_write() on CPU_0,
	   increments CTR[0].

	   Then it notices sb->s_writers.frozen >= level and starts
	   __sb_end_write() before retry.

	   Then it migrates to CPU_1. And decrements CTR[1] before
	   __percpu_counter_sum() reads it.

	   So CTR[0] == 1, CTR[1] == 0. Everything is fine except
	   sb_wait_write() has already read CTR[0].

	 4. __percpu_counter_sum() continues, reads CTR[1] == 0
	    and returns ret == 0.

sb_wait_write() returns while it should not?

--
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/