From: Jan Kara <jack@suse.cz>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Cody P Schafer <cody@linux.vnet.ibm.com>,
EXT4 <linux-ext4@vger.kernel.org>, Jan Kara <jack@suse.cz>,
rostedt@goodmis.org, Seth Jennings <sjenning@linux.vnet.ibm.com>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 01/11] rbtree: Fix rbtree_postorder_for_each_entry_safe() iterator
Date: Thu, 7 Nov 2013 23:14:46 +0100 [thread overview]
Message-ID: <20131107221446.GA2054@quack.suse.cz> (raw)
In-Reply-To: <20131107133800.c779b2f2b2ec73c91cd25f47@linux-foundation.org>
On Thu 07-11-13 13:38:00, Andrew Morton wrote:
> On Wed, 6 Nov 2013 17:42:30 -0800 Cody P Schafer <cody@linux.vnet.ibm.com> wrote:
>
> > The iterator rbtree_postorder_for_each_entry_safe() relies on pointer
> > underflow behavior when testing for loop termination. In particular
> > it expects that
> > &rb_entry(NULL, type, field)->field
> > is NULL. But the result of this expression is not defined by a C standard
> > and some gcc versions (e.g. 4.3.4) assume the above expression can never
> > be equal to NULL. The net result is an oops because the iteration is not
> > properly terminated.
> >
> > Fix the problem by modifying the iterator to avoid pointer underflows.
>
> So the sole caller is in zswap.c. Is that code actually generating oopses?
Oh, I didn't know there is any user of that iterator already in tree. Let
me check... Umm, looking at the disassembly of
zswap_frontswap_invalidate_aread:
0xffffffff8112c9a5 <+37>: mov %r13,%rdi
0xffffffff8112c9a8 <+40>: callq 0xffffffff81227620 <rb_first_postorder>
0xffffffff8112c9ad <+45>: mov %rax,%rdi
0xffffffff8112c9b0 <+48>: mov %rax,%rbx
0xffffffff8112c9b3 <+51>: callq 0xffffffff812275d0 <rb_next_postorder>
0xffffffff8112c9b8 <+56>: mov %rax,%r12
0xffffffff8112c9bb <+59>: nopl 0x0(%rax,%rax,1)
0xffffffff8112c9c0 <+64>: mov 0x28(%rbx),%rsi
0xffffffff8112c9c4 <+68>: mov 0x40(%r13),%rdi
0xffffffff8112c9c8 <+72>: callq 0xffffffff811352b0 <zbud_free>
0xffffffff8112c9cd <+77>: mov 0x1105504(%rip),%rdi
0xffffffff8112c9d4 <+84>: mov %rbx,%rsi
0xffffffff8112c9d7 <+87>: callq 0xffffffff81130b80 <kmem_cache_free>
0xffffffff8112c9dc <+92>: lock decl 0x110539d(%rip)
0xffffffff8112c9e3 <+99>: mov %r12,%rdi
0xffffffff8112c9e6 <+102>: mov %r12,%rbx
0xffffffff8112c9e9 <+105>: callq 0xffffffff812275d0 <rb_next_postorder>
0xffffffff8112c9ee <+110>: mov %rax,%r12
0xffffffff8112c9f1 <+113>: jmp 0xffffffff8112c9c0 <zswap_frontswap_invalidate_area+64>
So my gcc helpfully compiled that iterator into an endless loop as well,
although now it is a perfectly valid C code. According to our gcc guys
that was a bug in some gcc versions which is already fixed. But anyway
pushing my patch to 3.12 or anything that actually uses that iterator will
probably save us some bug reports.
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
next prev parent reply other threads:[~2013-11-07 22:14 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-07 1:42 [PATCH v2 00/11] rbtree: postorder iteration: fix, add tests, and use in various places Cody P Schafer
2013-11-07 1:42 ` [PATCH v2 01/11] rbtree: Fix rbtree_postorder_for_each_entry_safe() iterator Cody P Schafer
2013-11-07 11:51 ` Michel Lespinasse
2013-11-07 18:59 ` Cody P Schafer
2013-11-07 21:38 ` Andrew Morton
2013-11-07 21:58 ` Cody P Schafer
2013-11-07 22:14 ` Jan Kara [this message]
2013-11-07 1:42 ` [PATCH v2 02/11] rbtree/test: move rb_node to the middle of the test struct Cody P Schafer
2013-11-07 11:52 ` Michel Lespinasse
2013-11-07 1:42 ` [PATCH v2 03/11] rbtree/test: test rbtree_postorder_for_each_entry_safe() Cody P Schafer
2013-11-07 11:54 ` Michel Lespinasse
2013-11-07 1:42 ` [PATCH v2 04/11] net ipset: use rbtree postorder iteration instead of opencoding Cody P Schafer
2013-11-07 1:42 ` [PATCH v2 05/11] trace/trace_stat: use rbtree postorder iteration helper " Cody P Schafer
2013-11-07 1:42 ` [PATCH v2 06/11] fs/ubifs: " Cody P Schafer
2013-11-07 1:42 ` Cody P Schafer
2013-11-07 1:42 ` Cody P Schafer
2013-11-07 1:42 ` [PATCH v2 07/11] fs/ext4: " Cody P Schafer
2013-11-07 9:28 ` Jan Kara
2013-11-07 1:42 ` [PATCH v2 08/11] fs/jffs2: " Cody P Schafer
2013-11-07 1:42 ` Cody P Schafer
2013-11-07 1:42 ` Cody P Schafer
2013-11-07 1:42 ` [PATCH v2 09/11] fs/ext3: " Cody P Schafer
2013-11-07 8:17 ` Jan Kara
2013-11-07 1:42 ` [PATCH v2 10/11] mtd/ubi: " Cody P Schafer
2013-11-07 1:42 ` Cody P Schafer
2013-11-07 1:42 ` Cody P Schafer
2013-11-07 1:42 ` [PATCH v2 11/11] sh/dwarf: use rbtree postorder iteration helper instead of solution using repeated Cody P Schafer
2013-11-07 1:42 ` [PATCH v2 11/11] sh/dwarf: use rbtree postorder iteration helper instead of solution using repeated rb_erase() Cody P Schafer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131107221446.GA2054@quack.suse.cz \
--to=jack@suse.cz \
--cc=akpm@linux-foundation.org \
--cc=cody@linux.vnet.ibm.com \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=sjenning@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.