linux-kernel - Re: [GIT PULL] Folio fixes for 5.19

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: <YqO6FaO0/I9Ateze@casper.infradead.org>
Date:   Fri, 10 Jun 2022 22:39:33 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] Folio fixes for 5.19

On Fri, Jun 10, 2022 at 12:56:48PM -0700, Linus Torvalds wrote:
> On Fri, Jun 10, 2022 at 12:22 PM Matthew Wilcox <willy@...radead.org> wrote:
> >
> >  - Don't release a folio while it's still locked
> 
> Ugh.
> 
> I *hate* this patch. It's just incredibly broken.
> 
> Yes, I've pulled this, but I have looked at that readahead_folio()
> function before, and I have despised it before, but this patch really
> drove home how incredibly broken that function is.
> 
> Honestly, readahead_folio() returns a folio *AFTER* it has dropped the
> ref to that folio.

OK, you caught me.

I realised (a little too late) that the rules around refcounts in
->readpage and ->readahead are different, and that creates pain for
people writing filesystems.  For ->readahead, I stuck with the refcount
model that was in ->readpages (there is an extra refcount on the folio
and the filesystem must put it before it returns).

But I don't want to change the refcounting rules on a method without
changing something else about the method, because trying to find a
missing refcount change is misery.  Anyway, my cunning thought was
that if I bundle the change to the refcount rule with the change
from readahead_page() to readahead_folio(), once all filesystems
are converted to readahead_folio(), I can pull the refcount game out
of readahead_folio() and do it in the caller where it belongs, all
transparent to the filesystems.

I think it's worth doing, because it's two fewer atomic ops per folio
that we read from a file.  But I didn't think through the transition
process clearly enough, and right now it's a mess.  How would you like
me to proceed?

(I don't think the erofs code has a bug because it doesn't remove
the folio from the pagecache while holding the lock -- the folio lock
prevents anyone _else_ from removing the folio from the pagecache,
so there must be a reference on the folio up until erofs calls
folio_unlock()).