linux-kernel - Re: Undocumented and duplicated code

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 May 2008 15:14:07 +0200
From:	Rene Herman <rene.herman@...access.nl>
To:	Adrian Bunk <bunk@...nel.org>
CC:	Yinghai Lu <yhlu.kernel@...il.com>, Ingo Molnar <mingo@...e.hu>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	tglx@...utronix.de, hpa@...or.com, torvalds@...ux-foundation.org,
	akpm@...ux-foundation.org, Pavel Machek <pavel@...e.cz>
Subject: Re: Undocumented and duplicated code

On 07-05-08 14:46, Adrian Bunk wrote:

>> others like to to whitebox methods, ..., please try attach patch to
>> see if duron support PAT.
> 
> 
> There surely is documentation available covering this?
> 
> And why do we need this clear_cpu_cap(c, X86_FEATURE_PAT) and then 
> manual setting of X86_FEATURE_PAT at all?
> 
> There's no indication in the code, and as Rene already says there's even
> no description at all in commit 9307cacad0dfe3749f00303125c6f7f0523e5616
> 
> Such code really needs a comment explaining why we have to do this at all.
> 
> There must be some CPUs with the "pat" flag set but not being usable?
> Which?
> 
> According to the linux-kernel discussions there might not be any broken 
> CPU at all - but in this case the whitelist will not fill itself, and 
> expecting people to note that their flags changed and complaining is not 
> really a good approach.

I'd say it's an insane approach unless there's actually some indication of
buggy CPUs. Which there might be, I don't know, but this then definitely
wants a comment and changelog.

Just now sent out another reply as well saying much the same:

http://marc.info/?l=linux-kernel&m=121016531804511&w=2

but I see you tracked the people involved and added them to CC on this
thread-leaf...

> And that your commit added the same clear/set code in three different 
> places doesn't look good - this really deserved from the beginning being 
> factored out into an own function to avoid future problems when CPUs get 
> added (like what happens with your patch here - it touches only one 
> place, and since the same context is present in two places in the same 
> file "patch" might even choose freely where it gets applied...).
> 
> Pavel even made a similar comment on linux-kernel before the patch 
> got merged into Linus' tree. [1]
> 
> Guys, even if it compiles in all randconfig configurations and works on 
> all test machines this is exactly the kind of stuff that causes 
> headaches in the future.
> 
> And this patch (by the author of the code himself) is the first time 
> where it breaks.

Rene.
--
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/