linux-kernel - Re: [3.11-rc1] CONFIG_DEBUG_MUTEXES=y using gcc 3.x makes unbootable kernel.

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: <201309081624.EBJ90176.OJStQOFLHVFFOM@I-love.SAKURA.ne.jp>
Date:	Sun, 8 Sep 2013 16:24:34 +0900
From:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:	imirkin@...m.mit.edu, maarten.lankhorst@...onical.com
Cc:	linux-kernel@...r.kernel.org, daniel.vetter@...ll.ch,
	robdclark@...il.com, a.p.zijlstra@...llo.nl, mingo@...nel.org
Subject: Re: [3.11-rc1] CONFIG_DEBUG_MUTEXES=y using gcc 3.x makes unbootable kernel.

Hello.

Ilia Mirkin wrote:
> > Commit 040a0a37 "mutex: Add support for wound/wait style locks" used
> > "!__builtin_constant_p(p == NULL)" which I guess the author meant that
> > "__builtin_constant_p(p) && p", but gcc 3.x cannot handle such expression
> > correctly, leading to boot failure when built with CONFIG_DEBUG_MUTEXES=y.
> 
> I think that !__builtin_constant_p(p == NULL) is basically saying "I
> am unable to conclude that p == NULL at build time", which would
> translate to something along the lines of
> 
> (__builtin_constant_p(p) && p) || !__builtin_constant_p(p)
> 

I think

  (__builtin_constant_p(p) && p) && p->acquired > 0

is safe but

  (!__builtin_constant_p(p)) && p->acquired > 0

is not safe, for "p != NULL" check is required for avoiding NULL pointer
dereference.

It seems to me that

  (!__builtin_constant_p(p == NULL))

need to be translated to something along the lines of

  (__builtin_constant_p(p) && p) || (!__builtin_constant_p(p) && p)

which can be simplified as

  (p)

.

> Or perhaps it's just equivalent to !__builtin_constant_p(p), since the
> compiler's ability to conclude whether it is NULL at build-time should
> be unaffected by whether it actually is NULL or not.

Likewise, it seems to me that

  (!__builtin_constant_p(p == NULL))

need to be translated to something along the lines of

  (!__builtin_constant_p(p) && p)

. Well this change as well can fix "boot failure on gcc 3.x" and avoid "locking
selftests failure on gcc 3.x / 4.x". OK, let's wait for answer from the author.

Can I add "Signed-off-by: Ilia Mirkin <imirkin@...m.mit.edu>" to below patch?

---------- good patch start ----------
>>From a8bbf6b3c2d44cb90d63820f146aaff119d871c9 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Date: Sun, 8 Sep 2013 16:09:27 +0900
Subject: [PATCH] mutex: Avoid gcc version dependent __builtin_constant_p() usage.

Commit 040a0a37 "mutex: Add support for wound/wait style locks" used
"!__builtin_constant_p(p == NULL)" but gcc 3.x cannot handle such expression
correctly, leading to boot failure when built with CONFIG_DEBUG_MUTEXES=y.

Fix it by changing from "!__builtin_constant_p(p == NULL)" to
"!__builtin_constant_p(p) && p".

Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc: <stable@...nel.org> [3.11+]
---
 kernel/mutex.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/mutex.c b/kernel/mutex.c
index a52ee7bb..ef02003 100644
--- a/kernel/mutex.c
+++ b/kernel/mutex.c
@@ -448,7 +448,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 		struct task_struct *owner;
 		struct mspin_node  node;
 
-		if (!__builtin_constant_p(ww_ctx == NULL) && ww_ctx->acquired > 0) {
+		if (!__builtin_constant_p(ww_ctx) && ww_ctx && ww_ctx->acquired > 0) {
 			struct ww_mutex *ww;
 
 			ww = container_of(lock, struct ww_mutex, base);
@@ -478,7 +478,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 		if ((atomic_read(&lock->count) == 1) &&
 		    (atomic_cmpxchg(&lock->count, 1, 0) == 1)) {
 			lock_acquired(&lock->dep_map, ip);
-			if (!__builtin_constant_p(ww_ctx == NULL)) {
+			if (!__builtin_constant_p(ww_ctx) && ww_ctx) {
 				struct ww_mutex *ww;
 				ww = container_of(lock, struct ww_mutex, base);
 
@@ -548,7 +548,7 @@ slowpath:
 			goto err;
 		}
 
-		if (!__builtin_constant_p(ww_ctx == NULL) && ww_ctx->acquired > 0) {
+		if (!__builtin_constant_p(ww_ctx) && ww_ctx && ww_ctx->acquired > 0) {
 			ret = __mutex_lock_check_stamp(lock, ww_ctx);
 			if (ret)
 				goto err;
@@ -568,7 +568,7 @@ done:
 	mutex_remove_waiter(lock, &waiter, current_thread_info());
 	mutex_set_owner(lock);
 
-	if (!__builtin_constant_p(ww_ctx == NULL)) {
+	if (!__builtin_constant_p(ww_ctx) && ww_ctx) {
 		struct ww_mutex *ww = container_of(lock,
 						      struct ww_mutex,
 						      base);
-- 
1.7.8
---------- good patch end ----------
--
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/