[Bug,13012] 2.6.28.9 causes init to segfault on Debian etch; 2.6.28.8 OK

Commit Message

Dave Jones July 25, 2009, 3:23 a.m. UTC

On Sun, Jul 12, 2009 at 10:29:50PM -0700, Ian Lance Taylor wrote:

 > (The gcc 4.2 and later option -Wstrict-overflow=N can help find the
 > cases where a program relies on defined signed overflow, but only if
 > somebody is patient enough to wade through all the false positives.)
 
I got curious and wondered just how many warnings this would trigger,
so I did a build with -Wstrict-overflow=5.  I was pleasantly surprised
to see that it isn't that many from an allmodconfig build..

crypto/algboss.c:173: warning: assuming signed overflow does not occur when simplifying conditional to constant
net/ipv4/igmp.c:773: warning: assuming signed overflow does not occur when simplifying conditional to constant
drivers/ata/libata-eh.c:3076: warning: assuming signed overflow does not occur when simplifying conditional to constant
sound/core/seq/seq_queue.c:195: warning: assuming signed overflow does not occur when simplifying conditional to constant
drivers/block/ub.c:2293: warning: assuming signed overflow does not occur when simplifying conditional to constant
sound/pci/es1968.c:1545: warning: assuming signed overflow does not occur when simplifying conditional to constant
sound/pci/es1968.c:1582: warning: assuming signed overflow does not occur when simplifying conditional to constant
fs/ext3/inode.c:658: warning: assuming signed overflow does not occur when simplifying conditional to constant
fs/ext4/inode.c:797: warning: assuming signed overflow does not occur when simplifying conditional to constant
drivers/edac/amd64_edac.c:726: warning: assuming signed overflow does not occur when simplifying conditional to constant
sound/soc/codecs/wm8988.c:665: warning: assuming signed overflow does not occur when simplifying conditional to constant
net/bluetooth/cmtp/core.c:234: warning: assuming signed overflow does not occur when simplifying conditional to constant
net/mac80211/rc80211_minstrel.c:193: warning: assuming signed overflow does not occur when simplifying conditional to constant
drivers/isdn/hardware/eicon/debug.c:419: warning: assuming signed overflow does not occur when simplifying conditional to constant
drivers/uio/uio.c:707: warning: assuming signed overflow does not occur when simplifying conditional to constant
drivers/uio/uio.c:671: warning: assuming signed overflow does not occur when simplifying conditional to constant
drivers/uio/uio.c:642: warning: assuming signed overflow does not occur when simplifying conditional to constant
drivers/net/usb/hso.c:2308: warning: assuming signed overflow does not occur when simplifying conditional to constant
drivers/video/matrox/matroxfb_g450.c:214: warning: assuming signed overflow does not occur when simplifying conditional to constant
drivers/video/matrox/matroxfb_g450.c:160: warning: assuming signed overflow does not occur when simplifying conditional to constant
drivers/video/matrox/matroxfb_maven.c:1125: warning: assuming signed overflow does not occur when simplifying conditional to constant
drivers/video/matrox/matroxfb_maven.c:1044: warning: assuming signed overflow does not occur when simplifying conditional to constant
drivers/net/wireless/zd1211rw/zd_rf_uw2453.c:417: warning: assuming signed overflow does not occur when simplifying conditional to constant

Some of these appear to be obvious cases where we don't care about
the sign (loop counters for eg)

Is it worth fixing these up, with diffs like the below ?

	Dave


--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Linus Torvalds July 25, 2009, 4:49 p.m. UTC | #1

On Fri, 24 Jul 2009, Dave Jones wrote:
> 
> Is it worth fixing these up, with diffs like the below ?

Historically, when we fix up bugs like this, it causes more bugs than it 
fixes.

It needs _really_ careful people, and people who really understand how C 
type rules work. Stuff that looks obvious often is not, and basing a large 
part of the patch on a compiler warning is a _very_ weak reason for 
something that is more than just syntactic.

And the problem is, nobody can judge the patch from the diff. So it gets 
absolutely zero review, until the day when somebody notices that the 
unsigned version is a bug.

I'd suggest that the rule should be:

 - if you can use -U30 and show all uses, so that people can actually look 
   at the patch and see what it _causes_ (ie not just the "we change 'i' 
   to 'unsigned'", but also the "this is where 'i' gets used, and 
   'unsigned' is right"), then we can apply it.

 - none of the patches go through a 'trivial' tree, or come from newbies 
   that think this is a good way to get involved.

Is it worth it at that point? I dunno.

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
index 5f51fed..3bb95de 100644
--- a/fs/ext3/inode.c
+++ b/fs/ext3/inode.c
@@ -595,7 +595,7 @@  static int ext3_alloc_branch(handle_t *handle, struct inode *inode,
 			int *offsets, Indirect *branch)
 {
 	int blocksize = inode->i_sb->s_blocksize;
-	int i, n = 0;
+	unsigned int i, n = 0;
 	int err = 0;
 	struct buffer_head *bh;
 	int num;