linux-kernel - Re: llist code relies on undefined behaviour, upsets llvm/clang

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] [day] [month] [year] [list]

Date:   Mon, 16 Jan 2017 17:25:04 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     'Anton Blanchard' <anton@...ba.org>,
        "behanw@...verseincode.com" <behanw@...verseincode.com>,
        "ying.huang@...el.com" <ying.huang@...el.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "oleg@...hat.com" <oleg@...hat.com>,
        Segher Boessenkool <segher@...nel.crashing.org>,
        "mingo@...e.hu" <mingo@...e.hu>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: llist code relies on undefined behaviour, upsets llvm/clang

On Mon, Jan 16, 2017 at 02:34:43PM +0000, David Laight wrote:
> From: Anton Blanchard
> > Sent: 15 January 2017 21:36
> > I was debugging a hang on a ppc64le kernel built with clang, and it
> > looks to be undefined behaviour with pointer wrapping in the llist code.
> > 
> > A test case is below. llist_for_each_entry() does container_of() on a
> > NULL pointer, which wraps our pointer negative, then adds the same
> > offset back in and expects to get back to NULL. Unfortunately clang
> > decides that this can never be NULL and optimises it into an infinite
> > loop.
> ...
> > #define llist_for_each_entry(pos, node, member)                         \
> >         for ((pos) = llist_entry((node), typeof(*(pos)), member);       \
> >              &(pos)->member != NULL;                                    \
> >              (pos) = llist_entry((pos)->member.next, typeof(*(pos)), member))
> 
> Maybe the above could be rewritten as (untested):
> 		for ((pos) = NULL; (!(pos) ? (node) : ((pos)->member.next) || (pos) = 0) && \
> 			(((pos) = !(pos) ? llist_entry((node), typeof(*(pos)), member) \
> 					: llist_entry((pos)->member.next, typeof(*(pos)), member)),1); )
> Provided the compiler assumes that the loop body is never executed with 'pos == 0'
> it should generate the same code.

That's far uglier code and to what point? The compiler should simply not
assume silly things.