linux-kernel - x86 rwsem: up_read() does not check active count in fast path

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100503225105.GA18897@google.com>
Date:	Mon, 3 May 2010 15:51:05 -0700
From:	Michel Lespinasse <walken@...gle.com>
To:	David Howells <dhowells@...hat.com>,
	Andrew Morton <akpm@...gle.com>
Cc:	LKML <linux-kernel@...r.kernel.org>
Subject: x86 rwsem: up_read() does not check active count in fast path

Hi,

Looking at the x86 rwsem code, I have been wondering about the up_read() path.
__rwsem_do_wake() comment mentions that one should check the active count on
the way there; however I could not find that check when coming from up_read().

If there are multiple read locks active on a given semaphore, and there is
a write lock queued, each read lock will go:
__up_read
-> notice sem value is <0
-> rwsem_wake
-> take spinlock
-> add 1 in __rwsem_do_wake
-> notice there were other active lockers
-> substract 1 again under 'undo'
-> release spinlock

This could be avoided by adding a check for the active count in __up_write(),
as in the following untested patch:

diff --git a/arch/x86/include/asm/rwsem.h b/arch/x86/include/asm/rwsem.h
index 606ede1..915941f 100644
--- a/arch/x86/include/asm/rwsem.h
+++ b/arch/x86/include/asm/rwsem.h
@@ -200,11 +200,18 @@ static inline void __up_read(struct rw_semaphore *sem)
 		     LOCK_PREFIX "  xadd      %1,(%2)\n\t"
 		     /* subtracts 1, returns the old value */
 		     "  jns        1f\n\t"
+#ifdef CONFIG_X86_64
+		     "  dec        %%edx\n\t" /* %edx = low 32 bits of %1 */
+#else
+		     "  dec        %1\n\t"
+		     "  and        %4,%1\n\t"
+#endif
+		     "  jnz        1f\n\t"
 		     "  call call_rwsem_wake\n"
 		     "1:\n"
 		     "# ending __up_read\n"
 		     : "+m" (sem->count), "=d" (tmp)
-		     : "a" (sem), "1" (tmp)
+		     : "a" (sem), "1" (tmp), "i" (RWSEM_ACTIVE_MASK)
 		     : "memory", "cc");
 }
 
Is there any reason not to do this ? I have to mention I don't have any
specific workload in mind that might benefit from this; I'm only sending this
because the code contradicts the __rwsem_do_wake() comment.

Thanks,

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
--
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/