linux-kernel - Re: [PATCH v2] ext4: check folio uptodate state in ext4_page

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: <CADhLXY7pSghxkjw5g2pzbuB3KM7Ms7HByrn-wRRbjxrUxHwV_g@mail.gmail.com>
Date: Wed, 3 Dec 2025 22:34:10 +0530
From: Deepanshu Kartikey <kartikey406@...il.com>
To: Theodore Tso <tytso@....edu>
Cc: Zhang Yi <yi.zhang@...weicloud.com>, linux-ext4@...r.kernel.org, 
	linux-kernel@...r.kernel.org, 
	syzbot+b0a0670332b6b3230a0a@...kaller.appspotmail.com, 
	adilger.kernel@...ger.ca, djwong@...nel.org, 
	Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH v2] ext4: check folio uptodate state in ext4_page_mkwrite()

On Wed, Dec 3, 2025 at 9:18 PM Theodore Tso <tytso@....edu> wrote:
>
> My main concern with your patch is folio_lock() is *incredibly*
> heavyweight and is going to be a real scalability concern if we need
> to take it every single time we need to make a page writeable.
>
> So could we perhaps do something like this?  So the first question is
> do we need to take the lock at all?  I'm not sure we need to worry
> about the case where the page is not uptodate because we're racing
> with the page being brought into memory; if we that could happen under
> normal circumstances we would be triggering the warning even without
> these situations such as a delayed allocaiton write failing due to a
> corrupted file system image.   So can we just do this?
>
>         if (!folio_test_uptodate(folio)) {
>                 ret = VM_FAULT_SIGBUS;
>                 goto out;
>         }
>
> If it is legitmate that ext4_page_mkwrite() could be called while the
> page is still being read in (and again, I don't think it is), then we
> could do something like this:
>
>         if (!folio_test_uptodate(folio)) {
>                 folio_lock(folio);
>                 if (!folio_test_uptodate(folio)) {
>                         folio_unlock(folio);
>                         ret = VM_FAULT_SIGBUS;
>                         goto out;
>                 }
>                 folio_unlock(folio);
>         }
>
> Matthew, as the page cache maintainer, do we actually need this extra
> rigamarole.  Or can we just skip taking the lock before checking to
> see if the folio is uptodate in ext4_page_mkwrite()?
>
>                                                         - Ted


Hi Ted,

Thank you for the feedback and the performance concern!

You're absolutely right that folio_lock() is heavyweight. I included
it because I was being overly cautious about potential races, but I
agree with your analysis that under normal circumstances,
ext4_page_mkwrite() should never be called with a non-uptodate folio.

The non-uptodate state only occurs in this specific error case where:
1. Delayed allocation fails due to corruption
2. mpage_release_unused_pages() invalidates the folio
3. A subsequent operation triggers the fault

In this error path, the folio is already in an inconsistent state, so
checking folio_test_uptodate() without the lock should be sufficient
to catch it.

I'll wait for Matthew's input on the locking question, and then send
v3 with the appropriate changes.

Thank you for the guidance!

Best regards,
Deepanshu