linux-kernel - Re: [PATCH 3/3] x86,pci,acpi: Handle invalid

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 21 Apr 2010 16:10:07 -0700
From:	"H. Peter Anvin" <hpa@...or.com>
To:	Bjorn Helgaas <bjorn.helgaas@...com>
CC:	Yinghai Lu <yinghai.lu@...cle.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	Jesse Barnes <jbarnes@...tuousgeek.org>,
	Andy Isaacson <adi@...apodia.org>, guenter.roeck@...csson.com,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"x86@...nel.org" <x86@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Thomas Renninger <trenn@...e.de>
Subject: Re: [PATCH 3/3] x86,pci,acpi: Handle invalid _CRS

On 04/21/2010 04:04 PM, Bjorn Helgaas wrote:
> On Wednesday 21 April 2010 04:33:28 pm H. Peter Anvin wrote:
>> Do you have opinions on patches 1-2 of the series?
>>
>> I'm getting concerned about how the size of the patchset has grown, and
>> we're past -rc5 already... but it is a regression so we can't just defer
>> it to .35.
> 
> Part 1: the essential part of this seems to be the trim_bios_range()
> change, and that part is not too big.  In v4, Yinghai also removed
> probe_roms_32.c.  That sounds like the right thing to do, but I'd
> rather have that in separate patch so it doesn't obfuscate the other
> change, and I don't know whether it *has* to be done for .34; maybe
> it could be deferred.

I would agree with that.

> Part 2: IMHO, we're putting way too much crap in kernel/resource.c.
> A name like "reserve_region_with_split_check_child()" is a pretty
> good clue that we've lost our way somewhere.  But that's mostly a
> cosmetic thing, and the end result does seem to be something that
> fixes the current regression.

It's not just a good clue we have lost our way, it's also completely
impossible for anyone but Yinghai to divine what the intended semantics
are supposed to be.  This *greatly* concerns me, especially given
previous track record.

Even the checkin comment is almost unparsable, which makes it very
likely that someone is going to trip up on some of this in the future.
I really would like to get a better description.

The use of a string match in:

+       if (check_child && !conflict->child && strstr(conflict->name,
"PCI Bus"))
^^^^^^^^^

... screams "wrong! ugly! bad!" in my opinion.  I utterly fail to see
how that could be acceptable under any circumstances.  I thought that
had been flagged earlier in the conversation, but it is apparently still
there.

> So I guess that in spite of my issues with the implementation, I'm
> still OK with the concept.

OK, but what is "the implementation" and what is "the concept" here?

	-hap

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