linux-kernel - Re: [GIT PULL] Kmap conversions for 5.12

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]

Message-ID: <20210223192506.GY3014244@iweiny-DESK2.sc.intel.com>
Date:   Tue, 23 Feb 2021 11:25:06 -0800
From:   Ira Weiny <ira.weiny@...el.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     David Sterba <dsterba@...e.com>, David Sterba <dsterba@...e.cz>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>
Subject: Re: [GIT PULL] Kmap conversions for 5.12

On Tue, Feb 23, 2021 at 09:13:42AM -0800, Linus Torvalds wrote:
> On Tue, Feb 23, 2021 at 7:03 AM David Sterba <dsterba@...e.com> wrote:
> > Ira Weiny (8):
> >       iov_iter: Remove memzero_page() in favor of zero_user()
> 
> Ugh. I absolutely _detest_ this patch.

Sorry.

> 
> "zero_user()" is a completely horrendous function, and not at all the
> same as memzero_page().
> 
> Just look at it.
> 
> Yes, it's mis-used in a lot of places that really always wanted
> "memzero_page()", but this conversion is going exactly the wrong way
> around.

Originally I lifted memzero_page()[1] but was pointed to zero_user_segments()
which lead me astray to use zero_user().  I should have thought about it more
rather than blindly changing to zero_user().

> 
> Existing users of that zero_user() should have been converted to
> memzero_page(), rather than doing it this way.

I can do that.  No Problem.

> 
> The "user" naming should have given it away. It's a very very magical
> interface for user-mapped pages that have additional odd issues (ie
> look at the dcache flushing etc).

Agreed.

> 
> I'll think some more about this pull request, but honestly, this one
> broken is pretty much enough for me to say "No way in hell", because
> it shows a complete disregard for sanity.

Can we just drop the zero_user() patches?  Christoph and others would like to
see memcpy_[to|from]_page() lifted to the core for other work which is pending.
Would you agree to those?

> 
> The last commit in the series:
> 
> >       btrfs: convert to zero_user()
> 
> is also very mixed up about whether it actually wants the dcache
> flushing or not (and thus zero_user() or memzero_page()).

Drop this patch too?

> 
> Honestly, I suspect all the dcache flushing is totally pointless,
> because any architecture with virtual caches that does kmap needs to
> flush at kunmap anyway, afaik. Some of it is probably just voodoo
> programming and copying a pattern.
> 
> But in any case, zero_user() is not the same thing as memzero_page(),
> and even if they *were* the same thing, zero_user() is objectively the
> horribly much worse name.

Sorry.  I will change it.
Ira

> 
>               Linus

[1] https://lore.kernel.org/lkml/20201124141941.GB4327@casper.infradead.org/#t