linux-kernel - Re: [PATCH] Fix get

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]

Date:	Wed, 07 Mar 2007 11:43:06 +0000
From:	David Howells <dhowells@...hat.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	akpm@...ux-foundation.org, benh@...nel.crashing.org,
	linux-kernel@...r.kernel.org, hpa@...or.com,
	johannes@...solutions.net
Subject: Re: [PATCH] Fix get_order() 

Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> > +#define ilog2_up(n) ((n) == 1 ? 0 : ilog2((n) - 1) + 1)
> 
> This is wrong. It uses "n" twice, which makes it unsafe as a macro.

Damn.  I missed that.

> Or it could use a "__builtin_constant_p()" (which gcc defines to not have 
> side effects) to allow the multiple use for constant data.

I should have, yes.

> Or we could require that "ilog2(0)" returns -1, and then we could just say
> 
> 	#define ilog2_up(n) (ilog2((n)-1)+1)

I'd rather not do that as the inline assembly variants then have to special
case ilog2(0) rather than just having an undefined result.

> The whole "get_order()" macro also has some serious lack of parenthesis. 
> In general, commit 39d61db0edb34d60b83c5e0d62d0e906578cc707 just was 
> pretty damn bad!

Unfortunately, I can't disagree.

> I'm becoming a bit disgruntled about this whole thing, I have to admit. 
> I'm just not sure the bugs here are worth it. Especially considering that 
> __get_order() has apparently never even tested these things to begin with, 

It was tested...  I've just re-examined my test program and I've realised I've
only tested power-of-2 parameters.  Sigh.

> since nobody but FRV has ever #defined the ARCH_HAS_ILOG2_U?? macros.

Well, that should be CONFIG_ARCH_HAS_ILOG2_U?? macros, and powerpc defines
those too.

>  - buggy

True, for N being a non-power-of-two, unfortunately; and also where evaluating
N has side-effects.

>  - untested

Not true, just that my userspace test program isn't sufficiently exhaustive.

>  - has untrue comments

Unfortunately so.

>  - makes no real sense

Not true.

Various archs (including i386, x86_64, powerpc and frv) have instructions that
can be used to calculate integer log2(N).  The fallback position is to use a
loop:

	size = (size - 1) >> (PAGE_SHIFT - 1);
	order = -1;
	do {
		size >>= 1;
		order++;
	} while (size);

> and I'm inclined to just revert 39d61db0 instead of adding more and more 
> breakage to it, since it's simply not going to help with the fundamental 
> problems!

Probably a good idea.  I'll work on it some more and improve my test program
(which is actually quite simple to do).

David
-
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/