| 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
| ||
|
Message-ID: <522C312A.7050705@canonical.com>
Date: Sun, 08 Sep 2013 10:11:22 +0200
From: Maarten Lankhorst <maarten.lankhorst@...onical.com>
To: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
CC: imirkin@...m.mit.edu, 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.
Op 08-09-13 09:24, Tetsuo Handa schreef:
> 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);
Wrong. Wrong. Wrong.
The builtin_constant_p was explicitly added to NOT do a null pointer check in the ww_ctx case, and now you re-introduce it for ALL
compilers. Gcc will still think ww_ctx may be NULL in the ww_mutex_lock case.
__builtin_constant_p(ww_ctx) always evaluates to false, and is not equivalent to __builtin_constant_p(ww_ctx != NULL) in gcc 4.6 at least,
I have no idea why gcc treats pointers differently like that. Explicitly testing against NULL fixes it.
__builtin_constant_p(ww_ctx == NULL) should return the same value as __builtin_constant_p(ww_ctx != NULL), but I did the == NULL check for clarity,
if it's broken for your compiler, please add a bool use_ww_ctx or something to __mutex_lock_common that's set directly instead, the __builtin_constant_p trick
might be too gcc version specific.
~Maarten
--
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/
Powered by blists - more mailing lists