kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Ivan Zahariev]

Hello,

Starting with kernel 5.15 for the past eight months we have a total of 
12 kernel panics at a fleet of 1000 KVM/Qemu machines which look the 
following way:

     kernel BUG at fs/ext4/inode.c:1914

Switching from kernel 4.14 to 5.15 almost immediately triggered the 
problem. Therefore we are very confident that userland activity is more 
or less the same and is not the root cause. The kernel function which 
triggers the BUG is __ext4_journalled_writepage(). In 5.15 the code for 
__ext4_journalled_writepage() in "fs/ext4/inode.c" is the same as the 
current kernel "master". The line where the BUG is triggered is:

     struct buffer_head *page_bufs = page_buffers(page)

The definition of "page_buffers(page)" in "include/linux/buffer_head.h" 
hasn't changed since 4.14, so no difference here. This is where the 
actual "kernel BUG" event is triggered:

     /* If we *know* page->private refers to buffer_heads */
     #define page_buffers(page) \
         ({ \
             BUG_ON(!PagePrivate(page)); \
             ((struct buffer_head *)page_private(page)); \
         })
     #define page_has_buffers(page) PagePrivate(page)

Initially I thought that the issue is already discussed here: 
https://lore.kernel.org/all/Yg0m6IjcNmfaSokM@google.com/
But this seems to be another (solved) problem and Theodore Ts'o already 
made a quick fix by simply reporting the rare occurrence and continuing 
forward. The commit is in 5.15 (and in the latest kernel), so it's not 
helping our case: 
https://github.com/torvalds/linux/commit/cc5095747edfb054ca2068d01af20be3fcc3634f

Back to the problem! 99% of the difference between 4.14 and the latest 
kernel for __ext4_journalled_writepage() in "fs/ext4/inode.c" comes from 
the following commit: 
https://github.com/torvalds/linux/commit/5c48a7df91499e371ef725895b2e2d21a126e227

Is it safe that we revert this patch on the latest 5.15 kernel, so that 
we can confirm if this resolves the issue for us?

Best regards.
--Ivan

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Ivan Zahariev]

Hello,

I forgot to mention that the ext4 file system is mounted with 
"data=journal" and the crash happens on servers which have more than 20 
GB RAM and are I/O busy.

> Back to the problem! 99% of the difference between 4.14 and the latest 
> kernel for __ext4_journalled_writepage() in "fs/ext4/inode.c" comes 
> from the following commit: 
> https://github.com/torvalds/linux/commit/5c48a7df91499e371ef725895b2e2d21a126e227
>
> Is it safe that we revert this patch on the latest 5.15 kernel, so 
> that we can confirm if this resolves the issue for us?

If we can't or if it doesn't make sense to revert the patch, is there 
anything else we can do to assist in the debug of this rare kernel crash?

The machines are Qemu/KVM guests but dumping the whole memory would take 
a couple of minutes, so it's not viable.

Are there any debug statements we could add in 
__ext4_journalled_writepage() in "fs/ext4/inode.c" that may give a hint 
where the problem is?

Best regards.
--Ivan

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Theodore Ts'o]

On Wed, Nov 23, 2022 at 04:48:01PM +0200, Ivan Zahariev wrote:
> Hello,
> 
> Starting with kernel 5.15 for the past eight months we have a total of 12
> kernel panics at a fleet of 1000 KVM/Qemu machines which look the following
> way:
> 
>     kernel BUG at fs/ext4/inode.c:1914
> 
> Switching from kernel 4.14 to 5.15 almost immediately triggered the problem.
> Therefore we are very confident that userland activity is more or less the
> same and is not the root cause. The kernel function which triggers the BUG
> is __ext4_journalled_writepage(). In 5.15 the code for
> __ext4_journalled_writepage() in "fs/ext4/inode.c" is the same as the
> current kernel "master". The line where the BUG is triggered is:
> 
>     struct buffer_head *page_bufs = page_buffers(page)
	...
> 
> Back to the problem! 99% of the difference between 4.14 and the latest
> kernel for __ext4_journalled_writepage() in "fs/ext4/inode.c" comes from the
> following commit: https://github.com/torvalds/linux/commit/5c48a7df91499e371ef725895b2e2d21a126e227
> 
> Is it safe that we revert this patch on the latest 5.15 kernel, so that we
> can confirm if this resolves the issue for us?

No, it's not safe to revert this patch; this fixes a potential
use-after-free bug identified by Syzkaller (and use-after-frees can
sometimes be leveraged into security volunerabilities.

I have not tested this change yet, but looking at the code and the
change made in patch yet, this *might* be a possible fix:

	size = i_size_read(inode);
-	if (page->mapping != mapping || page_offset(page) > size) {
+	if (page->mapping != mapping || page_offset(page) >= size) {
		/* The page got truncated from under us */
		ext4_journal_stop(handle);
		ret = 0;
		goto out;
	}

Is it fair to say that your workload is using data=journaled and is
frequently truncating that might have been recently modified (hence
triggering the race between truncate and journalled writepages)?

I wonder if you could come up with a more reliable reproducer so we
can test a particular patch.

Thanks,

					- Ted

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Ivan Zahariev]

On 5.12.2022 г. 23:10, Theodore Ts'o wrote:
> Is it fair to say that your workload is using data=journaled and is
> frequently truncating that might have been recently modified (hence
> triggering the race between truncate and journalled writepages)?

The servers are hosting hundreds of users who run their own tasks and we 
have no control nor a way to closely observe their usage pattern. Unless 
you point us in a direction to debug this somehow.

"data=journaled" is definitely in place for all servers.

> I wonder if you could come up with a more reliable reproducer so we
> can test a particular patch.

We already tried different parallel combinations of mmap()'ed reading, 
direct and regular write(), drop_caches, sync(), etc. but we can't 
trigger the panic.

If you have any suggestions what we should try next as a reproducer, 
please share and we will try to implement and execute it.

Did I understand correctly that a possible reproducer would be a loop of 
heavy write() followed by truncate() of the same file? Should we 
randomly sync() and/or "echo 3 > /proc/sys/vm/drop_caches" to increase 
the chance of hitting the bug?

Best regards.
--Ivan

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 2815 bytes --]

On Mon 05-12-22 16:10:47, Theodore Ts'o wrote:
> On Wed, Nov 23, 2022 at 04:48:01PM +0200, Ivan Zahariev wrote:
> > Hello,
> > 
> > Starting with kernel 5.15 for the past eight months we have a total of 12
> > kernel panics at a fleet of 1000 KVM/Qemu machines which look the following
> > way:
> > 
> >     kernel BUG at fs/ext4/inode.c:1914
> > 
> > Switching from kernel 4.14 to 5.15 almost immediately triggered the problem.
> > Therefore we are very confident that userland activity is more or less the
> > same and is not the root cause. The kernel function which triggers the BUG
> > is __ext4_journalled_writepage(). In 5.15 the code for
> > __ext4_journalled_writepage() in "fs/ext4/inode.c" is the same as the
> > current kernel "master". The line where the BUG is triggered is:
> > 
> >     struct buffer_head *page_bufs = page_buffers(page)
> 	...
> > 
> > Back to the problem! 99% of the difference between 4.14 and the latest
> > kernel for __ext4_journalled_writepage() in "fs/ext4/inode.c" comes from the
> > following commit: https://github.com/torvalds/linux/commit/5c48a7df91499e371ef725895b2e2d21a126e227
> > 
> > Is it safe that we revert this patch on the latest 5.15 kernel, so that we
> > can confirm if this resolves the issue for us?
> 
> No, it's not safe to revert this patch; this fixes a potential
> use-after-free bug identified by Syzkaller (and use-after-frees can
> sometimes be leveraged into security volunerabilities.
> 
> I have not tested this change yet, but looking at the code and the
> change made in patch yet, this *might* be a possible fix:
> 
> 	size = i_size_read(inode);
> -	if (page->mapping != mapping || page_offset(page) > size) {
> +	if (page->mapping != mapping || page_offset(page) >= size) {
> 		/* The page got truncated from under us */
> 		ext4_journal_stop(handle);
> 		ret = 0;
> 		goto out;
> 	}
> 
> Is it fair to say that your workload is using data=journaled and is
> frequently truncating that might have been recently modified (hence
> triggering the race between truncate and journalled writepages)?
> 
> I wonder if you could come up with a more reliable reproducer so we
> can test a particular patch.

So after a bit of thought I agree that the commit 5c48a7df91499 ("ext4: fix
an use-after-free issue about data=journal writeback mode") is broken. The
problem is when we unlock the page in __ext4_journalled_writepage() anybody
else can come, writeout the page, and reclaim page buffers (due to memory
pressure). Previously, bh references were preventing the buffer reclaim to
happen but now there's nothing to prevent it.

My rewrite of data=journal writeback path fixes this problem as a
side-effect but perhaps we need a quickfix for stable kernels? Something
like attached patch?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-ext4-Fix-crash-in-__ext4_journalled_writepage.patch --]
[-- Type: text/x-patch, Size: 3221 bytes --]

From 17ec3d08a7878625c08ab37c45a8dc3c619db7fb Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Thu, 12 Jan 2023 14:46:12 +0100
Subject: [PATCH] ext4: Fix crash in __ext4_journalled_writepage()

When __ext4_journalled_writepage() unlocks the page, there's nothing
that prevents another process from finding the page and reclaiming
buffers from it (because we have cleaned the page dirty bit and buffers
needn't have the dirty bit set). When that happens we crash in
__ext4_journalled_writepage() when trying to get the page buffers. Fix
the problem by redirtying the page before unlocking it (so that reclaim
and other users know the page isn't written yet) and by checking the
page is still dirty after reacquiring the page lock. This should also
make sure the page still has buffers.

Fixes: 5c48a7df9149 ("ext4: fix an use-after-free issue about data=journal writeback mode")
CC: stable@vger.kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 9d9f414f99fe..9c8866777430 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -136,7 +136,6 @@ static inline int ext4_begin_ordered_truncate(struct inode *inode,
 						   new_size);
 }
 
-static int __ext4_journalled_writepage(struct page *page, unsigned int len);
 static int ext4_meta_trans_blocks(struct inode *inode, int lblocks,
 				  int pextents);
 
@@ -1887,8 +1886,8 @@ int ext4_da_get_block_prep(struct inode *inode, sector_t iblock,
 	return 0;
 }
 
-static int __ext4_journalled_writepage(struct page *page,
-				       unsigned int len)
+static int __ext4_journalled_writepage(struct writeback_control *wbc,
+				       struct page *page, unsigned int len)
 {
 	struct address_space *mapping = page->mapping;
 	struct inode *inode = mapping->host;
@@ -1898,8 +1897,6 @@ static int __ext4_journalled_writepage(struct page *page,
 	struct buffer_head *inode_bh = NULL;
 	loff_t size;
 
-	ClearPageChecked(page);
-
 	if (inline_data) {
 		BUG_ON(page->index != 0);
 		BUG_ON(len > ext4_get_max_inline_size(inode));
@@ -1913,6 +1910,7 @@ static int __ext4_journalled_writepage(struct page *page,
 	 * out from under us.
 	 */
 	get_page(page);
+	redirty_page_for_writepage(wbc, page);
 	unlock_page(page);
 
 	handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE,
@@ -1926,8 +1924,10 @@ static int __ext4_journalled_writepage(struct page *page,
 
 	lock_page(page);
 	put_page(page);
+	ClearPageChecked(page);
 	size = i_size_read(inode);
-	if (page->mapping != mapping || page_offset(page) > size) {
+	if (page->mapping != mapping || page_offset(page) >= size ||
+	    !clear_page_dirty_for_io(page)) {
 		/* The page got truncated from under us */
 		ext4_journal_stop(handle);
 		ret = 0;
@@ -2083,7 +2083,7 @@ static int ext4_writepage(struct page *page,
 		 * It's mmapped pagecache.  Add buffers and journal it.  There
 		 * doesn't seem much point in redirtying the page here.
 		 */
-		return __ext4_journalled_writepage(page, len);
+		return __ext4_journalled_writepage(wbc, page, len);
 
 	ext4_io_submit_init(&io_submit, wbc);
 	io_submit.io_end = ext4_init_io_end(inode, GFP_NOFS);
-- 
2.35.3

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Ivan Zahariev]

On 12.1.2023 г. 17:07, Jan Kara wrote:
> So after a bit of thought I agree that the commit 5c48a7df91499 ("ext4: fix
> an use-after-free issue about data=journal writeback mode") is broken. The
> problem is when we unlock the page in __ext4_journalled_writepage() anybody
> else can come, writeout the page, and reclaim page buffers (due to memory
> pressure). Previously, bh references were preventing the buffer reclaim to
> happen but now there's nothing to prevent it.
>
> My rewrite of data=journal writeback path fixes this problem as a
> side-effect but perhaps we need a quickfix for stable kernels? Something
> like attached patch?
>
> 								Honza

Do you consider this patch production ready?

Should we test it on real production machines with a peace of mind that 
nothing can go wrong in regards to data loss or corruption?

--Ivan

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Jan Kara]

On Wed 15-03-23 13:27:11, Ivan Zahariev wrote:
> On 12.1.2023 г. 17:07, Jan Kara wrote:
> > So after a bit of thought I agree that the commit 5c48a7df91499 ("ext4: fix
> > an use-after-free issue about data=journal writeback mode") is broken. The
> > problem is when we unlock the page in __ext4_journalled_writepage() anybody
> > else can come, writeout the page, and reclaim page buffers (due to memory
> > pressure). Previously, bh references were preventing the buffer reclaim to
> > happen but now there's nothing to prevent it.
> > 
> > My rewrite of data=journal writeback path fixes this problem as a
> > side-effect but perhaps we need a quickfix for stable kernels? Something
> > like attached patch?
> > 
> > 								Honza
> 
> Do you consider this patch production ready?

Ah, the patch has likely fallen through the cracks because I waited for
some reply and then forgot about it and Ted likely missed it inside the
thread. But yes I consider the patch safe to test on production machines -
at least it has passed testing with fstests on my test VM without any
visible issues.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Theodore Ts'o]

On Wed, Mar 15, 2023 at 06:32:17PM +0100, Jan Kara wrote:
> On Wed 15-03-23 13:27:11, Ivan Zahariev wrote:
> > On 12.1.2023 г. 17:07, Jan Kara wrote:
> > > So after a bit of thought I agree that the commit 5c48a7df91499 ("ext4: fix
> > > an use-after-free issue about data=journal writeback mode") is broken. The
> > > problem is when we unlock the page in __ext4_journalled_writepage() anybody
> > > else can come, writeout the page, and reclaim page buffers (due to memory
> > > pressure). Previously, bh references were preventing the buffer reclaim to
> > > happen but now there's nothing to prevent it.
> > > 
> > > My rewrite of data=journal writeback path fixes this problem as a
> > > side-effect but perhaps we need a quickfix for stable kernels? Something
> > > like attached patch?
> > > 
> > > 								Honza
> > 
> > Do you consider this patch production ready?
> 
> Ah, the patch has likely fallen through the cracks because I waited for
> some reply and then forgot about it and Ted likely missed it inside the
> thread. But yes I consider the patch safe to test on production machines -
> at least it has passed testing with fstests on my test VM without any
> visible issues.

Yeah, sorry, I didn't see it since it was in an attachment as opposed
to with an explicit [PATCH] subject line.

And at this point, the data=journal writeback patches have landed in
the ext4/dev tree, and while we could try to see if we could land this
before the next merge window, I'm worried about merge or semantic
conflicts of having both patches in a tree at one time.

I guess we could send it to Linus, let it get backported into stable,
and then revert it during the merge window, ahead of applying the
data=journal cleanup patch series.  But that seems a bit ugly.  Or we
could ask for an exception from the stable kernel folks, after I do a
full set of xfstests runs on it.  (Of course, I don't think anyone has
been able to create a reliable reproducer, so all we can do is to test
for regression failures.)

Jan, Greg, what do you think?

					- Ted

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Marcus Hoffmann]

Hi,

> On Wed, Mar 15, 2023 at 18:57, Theodore Ts'o wrote:
>
> Yeah, sorry, I didn't see it since it was in an attachment as opposed
> to with an explicit [PATCH] subject line.
>
> And at this point, the data=journal writeback patches have landed in
> the ext4/dev tree, and while we could try to see if we could land this
> before the next merge window, I'm worried about merge or semantic
> conflicts of having both patches in a tree at one time.
>
> I guess we could send it to Linus, let it get backported into stable,
> and then revert it during the merge window, ahead of applying the
> data=journal cleanup patch series.  But that seems a bit ugly.  Or we
> could ask for an exception from the stable kernel folks, after I do a
> full set of xfstests runs on it.  (Of course, I don't think anyone has
> been able to create a reliable reproducer, so all we can do is to test
> for regression failures.)
>
> Jan, Greg, what do you think?

We've noticed this appearing for us as well now (on 5.15 with
data=journaled) and I wanted to ask what the status here is. Did any fix
here make it into a stable kernel yet? If not, I suppose I can still
apply the patch posted above as a quick-fix until this (or another
solution) makes it into the stable tree?

Best,
Marcus
________________________________

othermo GmbH | Sitz der Gesellschaft: Alzenau | Amtsgericht Aschaffenburg: HRB 14783 | USt-IdNr.: DE319977978 | Geschäftsführung: Dr. Dennis Metz.

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Greg KH]

On Thu, May 11, 2023 at 11:21:27AM +0200, Marcus Hoffmann wrote:
> Hi,
> 
> > On Wed, Mar 15, 2023 at 18:57, Theodore Ts'o wrote:
> > 
> > Yeah, sorry, I didn't see it since it was in an attachment as opposed
> > to with an explicit [PATCH] subject line.
> > 
> > And at this point, the data=journal writeback patches have landed in
> > the ext4/dev tree, and while we could try to see if we could land this
> > before the next merge window, I'm worried about merge or semantic
> > conflicts of having both patches in a tree at one time.
> > 
> > I guess we could send it to Linus, let it get backported into stable,
> > and then revert it during the merge window, ahead of applying the
> > data=journal cleanup patch series.  But that seems a bit ugly.  Or we
> > could ask for an exception from the stable kernel folks, after I do a
> > full set of xfstests runs on it.  (Of course, I don't think anyone has
> > been able to create a reliable reproducer, so all we can do is to test
> > for regression failures.)
> > 
> > Jan, Greg, what do you think?
> 
> We've noticed this appearing for us as well now (on 5.15 with
> data=journaled) and I wanted to ask what the status here is. Did any fix
> here make it into a stable kernel yet? If not, I suppose I can still
> apply the patch posted above as a quick-fix until this (or another
> solution) makes it into the stable tree?

Any reason you can't just move to 6.1.y instead?  What prevents that?

thanks,

greg k-h

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Marcus Hoffmann]

On 12.05.23 14:19, Greg KH wrote:
> On Thu, May 11, 2023 at 11:21:27AM +0200, Marcus Hoffmann wrote:
>> Hi,
>>
>>> On Wed, Mar 15, 2023 at 18:57, Theodore Ts'o wrote:
>>>
>>> Yeah, sorry, I didn't see it since it was in an attachment as opposed
>>> to with an explicit [PATCH] subject line.
>>>
>>> And at this point, the data=journal writeback patches have landed in
>>> the ext4/dev tree, and while we could try to see if we could land this
>>> before the next merge window, I'm worried about merge or semantic
>>> conflicts of having both patches in a tree at one time.
>>>
>>> I guess we could send it to Linus, let it get backported into stable,
>>> and then revert it during the merge window, ahead of applying the
>>> data=journal cleanup patch series.  But that seems a bit ugly.  Or we
>>> could ask for an exception from the stable kernel folks, after I do a
>>> full set of xfstests runs on it.  (Of course, I don't think anyone has
>>> been able to create a reliable reproducer, so all we can do is to test
>>> for regression failures.)
>>>
>>> Jan, Greg, what do you think?
>>
>> We've noticed this appearing for us as well now (on 5.15 with
>> data=journaled) and I wanted to ask what the status here is. Did any fix
>> here make it into a stable kernel yet? If not, I suppose I can still
>> apply the patch posted above as a quick-fix until this (or another
>> solution) makes it into the stable tree?
>
> Any reason you can't just move to 6.1.y instead?  What prevents that?
>

We will move to 6.1.y soon-ish (we are downstream from the rpi kernel tree)
Is this problem fixed there though? I couldn't really find anything
related to that in the tree?

Best,
Marcus
________________________________

othermo GmbH | Sitz der Gesellschaft: Alzenau | Amtsgericht Aschaffenburg: HRB 14783 | USt-IdNr.: DE319977978 | Geschäftsführung: Dr. Dennis Metz.

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Greg KH]

On Fri, May 12, 2023 at 04:24:30PM +0200, Marcus Hoffmann wrote:
> On 12.05.23 14:19, Greg KH wrote:
> > On Thu, May 11, 2023 at 11:21:27AM +0200, Marcus Hoffmann wrote:
> > > Hi,
> > > 
> > > > On Wed, Mar 15, 2023 at 18:57, Theodore Ts'o wrote:
> > > > 
> > > > Yeah, sorry, I didn't see it since it was in an attachment as opposed
> > > > to with an explicit [PATCH] subject line.
> > > > 
> > > > And at this point, the data=journal writeback patches have landed in
> > > > the ext4/dev tree, and while we could try to see if we could land this
> > > > before the next merge window, I'm worried about merge or semantic
> > > > conflicts of having both patches in a tree at one time.
> > > > 
> > > > I guess we could send it to Linus, let it get backported into stable,
> > > > and then revert it during the merge window, ahead of applying the
> > > > data=journal cleanup patch series.  But that seems a bit ugly.  Or we
> > > > could ask for an exception from the stable kernel folks, after I do a
> > > > full set of xfstests runs on it.  (Of course, I don't think anyone has
> > > > been able to create a reliable reproducer, so all we can do is to test
> > > > for regression failures.)
> > > > 
> > > > Jan, Greg, what do you think?
> > > 
> > > We've noticed this appearing for us as well now (on 5.15 with
> > > data=journaled) and I wanted to ask what the status here is. Did any fix
> > > here make it into a stable kernel yet? If not, I suppose I can still
> > > apply the patch posted above as a quick-fix until this (or another
> > > solution) makes it into the stable tree?
> > 
> > Any reason you can't just move to 6.1.y instead?  What prevents that?
> > 
> 
> We will move to 6.1.y soon-ish (we are downstream from the rpi kernel tree)
> Is this problem fixed there though? I couldn't really find anything
> related to that in the tree?

Test it and see!

And if you are downstream from the RPI kernel tree, my sympathies,
that's a tough place to be given the speed of it updating (i.e. not at
all...)

good luck!

greg k-h

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Jan Kara]

On Fri 12-05-23 16:24:30, Marcus Hoffmann wrote:
> On 12.05.23 14:19, Greg KH wrote:
> > On Thu, May 11, 2023 at 11:21:27AM +0200, Marcus Hoffmann wrote:
> > > Hi,
> > > 
> > > > On Wed, Mar 15, 2023 at 18:57, Theodore Ts'o wrote:
> > > > 
> > > > Yeah, sorry, I didn't see it since it was in an attachment as opposed
> > > > to with an explicit [PATCH] subject line.
> > > > 
> > > > And at this point, the data=journal writeback patches have landed in
> > > > the ext4/dev tree, and while we could try to see if we could land this
> > > > before the next merge window, I'm worried about merge or semantic
> > > > conflicts of having both patches in a tree at one time.
> > > > 
> > > > I guess we could send it to Linus, let it get backported into stable,
> > > > and then revert it during the merge window, ahead of applying the
> > > > data=journal cleanup patch series.  But that seems a bit ugly.  Or we
> > > > could ask for an exception from the stable kernel folks, after I do a
> > > > full set of xfstests runs on it.  (Of course, I don't think anyone has
> > > > been able to create a reliable reproducer, so all we can do is to test
> > > > for regression failures.)
> > > > 
> > > > Jan, Greg, what do you think?
> > > 
> > > We've noticed this appearing for us as well now (on 5.15 with
> > > data=journaled) and I wanted to ask what the status here is. Did any fix
> > > here make it into a stable kernel yet? If not, I suppose I can still
> > > apply the patch posted above as a quick-fix until this (or another
> > > solution) makes it into the stable tree?
> > 
> > Any reason you can't just move to 6.1.y instead?  What prevents that?
> > 
> 
> We will move to 6.1.y soon-ish (we are downstream from the rpi kernel tree)
> Is this problem fixed there though? I couldn't really find anything
> related to that in the tree?

Yeah. Due to various delays the rewrite of data=journal mode got merged
only into 6.4-rc1. So updating to 6.1.y isn't going to fix the problem.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Mathieu Othacehe]

Hello,

I also encountered that specific issue while migrating from 4.14 to 5.15
with data=journal. The proposed patch fixes the issue for me. The 6.1
branch seems to be affected as well. Would it be an option to have that
patch applied to stable branches?

Thanks,

Mathieu

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Jan Kara]

Hello,

On Wed 20-09-23 11:40:24, Mathieu Othacehe wrote:
> I also encountered that specific issue while migrating from 4.14 to 5.15
> with data=journal. The proposed patch fixes the issue for me. The 6.1
> branch seems to be affected as well. Would it be an option to have that
> patch applied to stable branches?

Well, Greg is rather reluctant to merge into stable tree patches that were
not included upstream. But with a good reason he could do that. I guess the
easiest is to try - take the backported fix and officially submit it to
stable@vger.kernel.org with the explanation why you are submitting
non-upstream patch - in particular that upstream solution is some 14
patch series.

Alternative solution is to backport and test the upstream solution:
bd159398a2d2 ("jdb2: Don't refuse invalidation of already invalidated buffers")
d84c9ebdac1e ("ext4: Mark pages with journalled data dirty")
265e72efa99f ("ext4: Keep pages with journalled data dirty")
5e1bdea6391d ("ext4: Clear dirty bit from pages without data to write")
1f1a55f0bf06 ("ext4: Commit transaction before writing back pages in data=journal mode")
e360c6ed7274 ("ext4: Drop special handling of journalled data from ext4_sync_file()")
c000dfec7e88 ("ext4: Drop special handling of journalled data from extent shifting operations")
783ae448b7a2 ("ext4: Fix special handling of journalled data from extent zeroing")
56c2a0e3d90d ("ext4: Drop special handling of journalled data from ext4_evict_inode()")
7c375870fdc5 ("ext4: Drop special handling of journalled data from ext4_quota_on()")
951cafa6b80e ("ext4: Simplify handling of journalled data in ext4_bmap()")
ab382539adcb ("ext4: Update comment in mpage_prepare_extent_to_map()")
d0ab8368c175 ("Revert "ext4: Fix warnings when freezing filesystem with journaled data"")
1077b2d53ef5 ("ext4: fix fsync for non-directories")

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Jan Kara]

Hello!

On Thu 05-10-23 09:08:50, Mathieu Othacehe wrote:
> > Backporting the series would be ideal. Is this only for the 5.15 kernel?
> 
> OK. I spotted it on a 5.15 but as far as I understand, this affects all
> stables with 5c48a7df9149, i.e all stables. Is that correct Jan?

Yes, that is correct. Also I have realized that before patches I've already
mentioned are applicable, you will also need to pick up:

9462f770eda8 ("ext4: Update stale comment about write constraints")
c8e8e16dbbf0 ("ext4: Use nr_to_write directly in mpage_prepare_extent_to_map()")
3f5d30636d2a ("ext4: Mark page for delayed dirtying only if it is pinned")
f1496362e9d7 ("ext4: Don't unlock page in ext4_bio_write_page()")
eaf2ca10ca4b ("ext4: Move page unlocking out of mpage_submit_page()")
d8be7607de03 ("ext4: Move mpage_page_done() calls after error handling")
3f079114bf52 ("ext4: Convert data=journal writeback to use ext4_writepages()")
e6c28a26b799 ("ext4: Fix warnings when freezing filesystem with journaled data")
  This commit actually gets reverted in the series of patches I have
already mentioned.

So sadly the backport is even larger than what I originally thought.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Mathieu Othacehe]

Hello,

> Backporting the series would be ideal. Is this only for the 5.15 kernel?

OK. I spotted it on a 5.15 but as far as I understand, this affects all
stables with 5c48a7df9149, i.e all stables. Is that correct Jan?

Mathieu

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Theodore Ts'o]

On Wed, Oct 04, 2023 at 06:10:16AM -0400, Sasha Levin wrote:
> On Wed, Oct 04, 2023 at 11:37:22AM +0200, Mathieu Othacehe wrote:
> > 
> > bd159398a2d2 ("jdb2: Don't refuse invalidation of already invalidated buffers")
> > d84c9ebdac1e ("ext4: Mark pages with journalled data dirty")
> > 265e72efa99f ("ext4: Keep pages with journalled data dirty")
> > 5e1bdea6391d ("ext4: Clear dirty bit from pages without data to write")
> > 1f1a55f0bf06 ("ext4: Commit transaction before writing back pages in data=journal mode")
> > e360c6ed7274 ("ext4: Drop special handling of journalled data from ext4_sync_file()")
> > c000dfec7e88 ("ext4: Drop special handling of journalled data from extent shifting operations")
> > 783ae448b7a2 ("ext4: Fix special handling of journalled data from extent zeroing")
> > 56c2a0e3d90d ("ext4: Drop special handling of journalled data from ext4_evict_inode()")
> > 7c375870fdc5 ("ext4: Drop special handling of journalled data from ext4_quota_on()")
> > 951cafa6b80e ("ext4: Simplify handling of journalled data in ext4_bmap()")
> > ab382539adcb ("ext4: Update comment in mpage_prepare_extent_to_map()")
> > d0ab8368c175 ("Revert "ext4: Fix warnings when freezing filesystem with journaled data"")
> > 1077b2d53ef5 ("ext4: fix fsync for non-directories")
> > 
> > Or apply the proposed, attached patch. Do you think that would be an
> > option?
> 
> Backporting the series would be ideal. Is this only for the 5.15 kernel?

If we're going to backport all of these patches, I'd really would like
to see a full regression test run, using something like:

   gce-xfstests ltm -c ext4/all -g auto

before and after applying all of these patches, to make sure there are
no regression.

(or you can "kvm-xfstests -c ext4/all -g auto" but be prepared for it
to take over 24 hours of run time.  With gce-xfstesets we start a
dozen VM's in parallel so it finishes in about 2.5 hours.  See
https://thunk.org/gce-xfstests for more information.)

If you someone who does the backports can send me a pointer to a git
branch, I can run the tests for you, if that would be helpful.

Thanks!!

				       - Ted

Re: kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Sasha Levin]

On Wed, Oct 04, 2023 at 11:37:22AM +0200, Mathieu Othacehe wrote:
>
>Hello,
>
>I have been experimenting this issue:
>https://www.spinics.net/lists/linux-ext4/msg86259.html, on a 5.15
>kernel.
>
>This issue caused by 5c48a7df9149 ("ext4: fix an use-after-free issue
>about data=journal writeback mode") is affecting ext4 users with
>data=journal on all stable kernels.
>
>Jan proposed a fix here
>https://www.spinics.net/lists/linux-ext4/msg87054.html which solves the
>situation for me.
>
>Now this fix is not upstream because the data journaling support has
>been rewritten. As suggested by Jan, that would mean that we could
>either backport the following patches from upstream:
>
>bd159398a2d2 ("jdb2: Don't refuse invalidation of already invalidated buffers")
>d84c9ebdac1e ("ext4: Mark pages with journalled data dirty")
>265e72efa99f ("ext4: Keep pages with journalled data dirty")
>5e1bdea6391d ("ext4: Clear dirty bit from pages without data to write")
>1f1a55f0bf06 ("ext4: Commit transaction before writing back pages in data=journal mode")
>e360c6ed7274 ("ext4: Drop special handling of journalled data from ext4_sync_file()")
>c000dfec7e88 ("ext4: Drop special handling of journalled data from extent shifting operations")
>783ae448b7a2 ("ext4: Fix special handling of journalled data from extent zeroing")
>56c2a0e3d90d ("ext4: Drop special handling of journalled data from ext4_evict_inode()")
>7c375870fdc5 ("ext4: Drop special handling of journalled data from ext4_quota_on()")
>951cafa6b80e ("ext4: Simplify handling of journalled data in ext4_bmap()")
>ab382539adcb ("ext4: Update comment in mpage_prepare_extent_to_map()")
>d0ab8368c175 ("Revert "ext4: Fix warnings when freezing filesystem with journaled data"")
>1077b2d53ef5 ("ext4: fix fsync for non-directories")
>
>Or apply the proposed, attached patch. Do you think that would be an
>option?

Backporting the series would be ideal. Is this only for the 5.15 kernel?

-- 
Thanks,
Sasha

kernel BUG at fs/ext4/inode.c:1914 - page_buffers()

[Mathieu Othacehe]

[-- Attachment #1: Type: text/plain, Size: 1786 bytes --]


Hello,

I have been experimenting this issue:
https://www.spinics.net/lists/linux-ext4/msg86259.html, on a 5.15
kernel.

This issue caused by 5c48a7df9149 ("ext4: fix an use-after-free issue
about data=journal writeback mode") is affecting ext4 users with
data=journal on all stable kernels.

Jan proposed a fix here
https://www.spinics.net/lists/linux-ext4/msg87054.html which solves the
situation for me.

Now this fix is not upstream because the data journaling support has
been rewritten. As suggested by Jan, that would mean that we could
either backport the following patches from upstream:

bd159398a2d2 ("jdb2: Don't refuse invalidation of already invalidated buffers")
d84c9ebdac1e ("ext4: Mark pages with journalled data dirty")
265e72efa99f ("ext4: Keep pages with journalled data dirty")
5e1bdea6391d ("ext4: Clear dirty bit from pages without data to write")
1f1a55f0bf06 ("ext4: Commit transaction before writing back pages in data=journal mode")
e360c6ed7274 ("ext4: Drop special handling of journalled data from ext4_sync_file()")
c000dfec7e88 ("ext4: Drop special handling of journalled data from extent shifting operations")
783ae448b7a2 ("ext4: Fix special handling of journalled data from extent zeroing")
56c2a0e3d90d ("ext4: Drop special handling of journalled data from ext4_evict_inode()")
7c375870fdc5 ("ext4: Drop special handling of journalled data from ext4_quota_on()")
951cafa6b80e ("ext4: Simplify handling of journalled data in ext4_bmap()")
ab382539adcb ("ext4: Update comment in mpage_prepare_extent_to_map()")
d0ab8368c175 ("Revert "ext4: Fix warnings when freezing filesystem with journaled data"")
1077b2d53ef5 ("ext4: fix fsync for non-directories")

Or apply the proposed, attached patch. Do you think that would be an
option?

Thanks,

Mathieu


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-fix-ext4-journalled-crash.patch --]
[-- Type: text/x-patch, Size: 3228 bytes --]

From 17ec3d08a7878625c08ab37c45a8dc3c619db7fb Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@xxxxxxx>
Date: Thu, 12 Jan 2023 14:46:12 +0100
Subject: [PATCH] ext4: Fix crash in __ext4_journalled_writepage()

When __ext4_journalled_writepage() unlocks the page, there's nothing
that prevents another process from finding the page and reclaiming
buffers from it (because we have cleaned the page dirty bit and buffers
needn't have the dirty bit set). When that happens we crash in
__ext4_journalled_writepage() when trying to get the page buffers. Fix
the problem by redirtying the page before unlocking it (so that reclaim
and other users know the page isn't written yet) and by checking the
page is still dirty after reacquiring the page lock. This should also
make sure the page still has buffers.

Fixes: 5c48a7df9149 ("ext4: fix an use-after-free issue about data=journal writeback mode")
CC: stable@xxxxxxxxxxxxxxx
Signed-off-by: Jan Kara <jack@xxxxxxx>
---
 fs/ext4/inode.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 64a783f22105..b9f1fd05cec6 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -138,7 +138,6 @@ static inline int ext4_begin_ordered_truncate(struct inode *inode,
 
 static void ext4_invalidatepage(struct page *page, unsigned int offset,
 				unsigned int length);
-static int __ext4_journalled_writepage(struct page *page, unsigned int len);
 static int ext4_meta_trans_blocks(struct inode *inode, int lblocks,
 				  int pextents);
 
@@ -1858,7 +1857,8 @@ int ext4_da_get_block_prep(struct inode *inode, sector_t iblock,
 	return 0;
 }
 
-static int __ext4_journalled_writepage(struct page *page,
+static int __ext4_journalled_writepage(struct writeback_control *wbc,
+				       struct page *page,
 				       unsigned int len)
 {
 	struct address_space *mapping = page->mapping;
@@ -1869,8 +1869,6 @@ static int __ext4_journalled_writepage(struct page *page,
 	struct buffer_head *inode_bh = NULL;
 	loff_t size;
 
-	ClearPageChecked(page);
-
 	if (inline_data) {
 		BUG_ON(page->index != 0);
 		BUG_ON(len > ext4_get_max_inline_size(inode));
@@ -1884,6 +1882,7 @@ static int __ext4_journalled_writepage(struct page *page,
 	 * out from under us.
 	 */
 	get_page(page);
+	redirty_page_for_writepage(wbc, page);
 	unlock_page(page);
 
 	handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE,
@@ -1897,8 +1896,10 @@ static int __ext4_journalled_writepage(struct page *page,
 
 	lock_page(page);
 	put_page(page);
+	ClearPageChecked(page);
 	size = i_size_read(inode);
-	if (page->mapping != mapping || page_offset(page) > size) {
+	if (page->mapping != mapping || page_offset(page) >= size ||
+	    !clear_page_dirty_for_io(page)) {
 		/* The page got truncated from under us */
 		ext4_journal_stop(handle);
 		ret = 0;
@@ -2055,7 +2056,7 @@ static int ext4_writepage(struct page *page,
 		 * It's mmapped pagecache.  Add buffers and journal it.  There
 		 * doesn't seem much point in redirtying the page here.
 		 */
-		return __ext4_journalled_writepage(page, len);
+		return __ext4_journalled_writepage(wbc, page, len);
 
 	ext4_io_submit_init(&io_submit, wbc);
 	io_submit.io_end = ext4_init_io_end(inode, GFP_NOFS);