linux-kernel - Re: [PATCH v6 3/6] x86/e820: Refactor range_update and range

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:   Tue, 8 Feb 2022 10:40:24 +0200
From:   Mike Rapoport <rppt@...nel.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     Martin Fernandez <martin.fernandez@...ypsium.com>,
        linux-kernel@...r.kernel.org, linux-efi@...r.kernel.org,
        platform-driver-x86@...r.kernel.org, linux-mm@...ck.org,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
        ardb@...nel.org, dvhart@...radead.org, andy@...radead.org,
        gregkh@...uxfoundation.org, rafael@...nel.org,
        akpm@...ux-foundation.org, daniel.gutson@...ypsium.com,
        hughsient@...il.com, alex.bazhaniuk@...ypsium.com,
        alison.schofield@...el.com
Subject: Re: [PATCH v6 3/6] x86/e820: Refactor range_update and range_remove

On Mon, Feb 07, 2022 at 01:45:40PM -0800, Kees Cook wrote:
> On Thu, Feb 03, 2022 at 01:43:25PM -0300, Martin Fernandez wrote:
> > __e820__range_update and e820__range_remove had a very similar
> > implementation with a few lines different from each other, the lines
> > that actually perform the modification over the e820_table. The
> > similiraties were found in the checks for the different cases on how
> > each entry intersects with the given range (if it does at all). These
> > checks were very presice and error prone so it was not a good idea to
> > have them in both places.
> 
> Yay removing copy/paste code! :)

Removing copy/paste is nice but diffstat of

 arch/x86/kernel/e820.c | 383 ++++++++++++++++++++++++++++++-----------
 1 file changed, 283 insertions(+), 100 deletions(-)

does not look nice even accounting for lots of comments :(

I didn't look closely, but diffstat clues that the refactoring making
things much more complex.
 
> > 
> > I propose a refactor of those functions, given that I need to create a
> > similar one for this patchset.
> 
> The diff here is pretty hard (for me) to review; I'll need more time
> to check it. What might make review easier (at least for me), is to
> incrementally change these routines. i.e. separate patches to:
> 
> - add the new infrastructure
> - replace e820__range_remove
> - replace __e820__range_update
> 
> If that's not actually useful, no worries. I'll just stare at it a bit
> more. :)

-- 
Sincerely yours,
Mike.