[PATCH] xfs: clear PF_MEMALLOC before exiting xfsaild thread

[PATCH] xfs: clear PF_MEMALLOC before exiting xfsaild thread

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Leaving PF_MEMALLOC set when exiting a kthread causes it to remain set
during do_exit().  That can confuse things.  For example, if BSD process
accounting is enabled, then it's possible for do_exit() to end up
calling ext4_write_inode().  That triggers the
WARN_ON_ONCE(current->flags & PF_MEMALLOC) there, as it assumes
(appropriately) that inodes aren't written when allocating memory.

This case was reported by syzbot at
https://lkml.kernel.org/r/0000000000000e7156059f751d7b@google.com.

Fix this in xfsaild() by using the helper functions to save and restore
PF_MEMALLOC.

Reported-by: syzbot+1f9dc49e8de2582d90c2@syzkaller.appspotmail.com
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/xfs/xfs_trans_ail.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 00cc5b8734be..3bc570c90ad9 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -529,8 +529,9 @@ xfsaild(
 {
 	struct xfs_ail	*ailp = data;
 	long		tout = 0;	/* milliseconds */
+	unsigned int	noreclaim_flag;
 
-	current->flags |= PF_MEMALLOC;
+	noreclaim_flag = memalloc_noreclaim_save();
 	set_freezable();
 
 	while (1) {
@@ -601,6 +602,7 @@ xfsaild(
 		tout = xfsaild_push(ailp);
 	}
 
+	memalloc_noreclaim_restore(noreclaim_flag);
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH] xfs: clear PF_MEMALLOC before exiting xfsaild thread

[Dave Chinner]

On Sat, Mar 07, 2020 at 08:35:40PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Leaving PF_MEMALLOC set when exiting a kthread causes it to remain set
> during do_exit().  That can confuse things.  For example, if BSD process
> accounting is enabled, then it's possible for do_exit() to end up
> calling ext4_write_inode().  That triggers the
> WARN_ON_ONCE(current->flags & PF_MEMALLOC) there, as it assumes
> (appropriately) that inodes aren't written when allocating memory.

And just how the hell does and XFS kernel thread end up calling
ext4_write_inode()? That's kinda a key factor in all this, and
it's not explained here.

> This case was reported by syzbot at
> https://lkml.kernel.org/r/0000000000000e7156059f751d7b@google.com.

Which doesn't really explain it, either.

What is the configuration conditions under which this triggers? It
looks like some weird combination of a no-journal ext4 root
filesystem and the audit subsystem being configured with O_SYNC
files?

People trying to decide if this is something that needs to be
backported to stable kernels need to be able to unerstand how this
bug is actually triggered so they can make sane decisions about
it...

/me tracks the PF_MEMALLOC flag back to commit 43ff2122e649 ("xfs:
on-stack delayed write buffer lists") where is was inherited here
from the buffer flush daemon that xfsaild took over from. Which also
never cleared the PF_MEMALLOC flag. That goes back to 2002:

commit d676c94914eb97d72061aff69c99406df4f395e9
Author: Steve Lord <lord@sgi.com>
Date:   Fri Jan 11 23:31:51 2002 +0000

    Merge pagebuf module into XFS

So this issue of calling do_exit() with PF_MEMALLOC set has been
around for 18+ years without anyone noticing it.

I also note that cifs_demultiplex_thread() has the same problem -
can you please do a complete audit of all the users of PF_MEMALLOC
and fix all of them?

> Fix this in xfsaild() by using the helper functions to save and restore
> PF_MEMALLOC.
> 
> Reported-by: syzbot+1f9dc49e8de2582d90c2@syzkaller.appspotmail.com
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/xfs/xfs_trans_ail.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 00cc5b8734be..3bc570c90ad9 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -529,8 +529,9 @@ xfsaild(
>  {
>  	struct xfs_ail	*ailp = data;
>  	long		tout = 0;	/* milliseconds */
> +	unsigned int	noreclaim_flag;
>  
> -	current->flags |= PF_MEMALLOC;
> +	noreclaim_flag = memalloc_noreclaim_save();
>  	set_freezable();
>  
>  	while (1) {
> @@ -601,6 +602,7 @@ xfsaild(
>  		tout = xfsaild_push(ailp);
>  	}
>  
> +	memalloc_noreclaim_restore(noreclaim_flag);
>  	return 0;
>  }

The code looks fine - I considered doing this a couple of weeks ago
just for cleaniness reasons - but the commit message needs work to
explain the context of the bug...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: clear PF_MEMALLOC before exiting xfsaild thread

[Eric Biggers]

On Mon, Mar 09, 2020 at 10:03:07AM +1100, Dave Chinner wrote:
> On Sat, Mar 07, 2020 at 08:35:40PM -0800, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Leaving PF_MEMALLOC set when exiting a kthread causes it to remain set
> > during do_exit().  That can confuse things.  For example, if BSD process
> > accounting is enabled, then it's possible for do_exit() to end up
> > calling ext4_write_inode().  That triggers the
> > WARN_ON_ONCE(current->flags & PF_MEMALLOC) there, as it assumes
> > (appropriately) that inodes aren't written when allocating memory.
> 
> And just how the hell does and XFS kernel thread end up calling
> ext4_write_inode()? That's kinda a key factor in all this, and
> it's not explained here.
> 
> > This case was reported by syzbot at
> > https://lkml.kernel.org/r/0000000000000e7156059f751d7b@google.com.
> 
> Which doesn't really explain it, either.
> 
> What is the configuration conditions under which this triggers? It
> looks like some weird combination of a no-journal ext4 root
> filesystem and the audit subsystem being configured with O_SYNC
> files?
> 
> People trying to decide if this is something that needs to be
> backported to stable kernels need to be able to unerstand how this
> bug is actually triggered so they can make sane decisions about
> it...

My guess is that syzbot enabled BSD process accounting to a file with FS_SYNC_FL
on an ext4 nojournal fs.  It didn't provide a reproducer itself.  Sure, I'll try
to write a reproducer and include it in the commit message.  I felt it wasn't
quite as important for this one compared to most of the other syzbot bugs, since
BSD process accounting can only be enabled by root, and once we're talking about
do_exit() being able to write an arbitrary file, it's not hard to see why
*something* could get tripped up by PF_MEMALLOC.  There are probably other ways
it could cause problems besides this specific one.  But sure, I'll try.

> 
> I also note that cifs_demultiplex_thread() has the same problem -
> can you please do a complete audit of all the users of PF_MEMALLOC
> and fix all of them?

I already did, that's why at the same time I sent out this patch, I also sent
out one to fix cifs_demultiplex_thread()
(https://lkml.kernel.org/linux-cifs/20200308043645.1034870-1-ebiggers@kernel.org/T/#t).
These were the only two; I didn't find any others that needed to be fixed.

- Eric

[PATCH v2] xfs: clear PF_MEMALLOC before exiting xfsaild thread

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Leaving PF_MEMALLOC set when exiting a kthread causes it to remain set
during do_exit().  That can confuse things.  For example, if BSD process
accounting is enabled and the accounting file has FS_SYNC_FL set and is
located on an ext4 filesystem without a journal, then do_exit() ends up
calling ext4_write_inode().  That triggers the
WARN_ON_ONCE(current->flags & PF_MEMALLOC) there, as it assumes
(appropriately) that inodes aren't written when allocating memory.

Fix this in xfsaild() by using the helper functions to save and restore
PF_MEMALLOC.

This can be reproduced as follows in the kvm-xfstests test appliance
modified to add the 'acct' Debian package, and with kvm-xfstests's
recommended kconfig modified to add CONFIG_BSD_PROCESS_ACCT=y:

	mkfs.ext2 -F /dev/vdb
	mount /vdb -t ext4
	touch /vdb/file
	chattr +S /vdb/file
	accton /vdb/file
	mkfs.xfs -f /dev/vdc
	mount /vdc
	umount /vdc

It causes:
	WARNING: CPU: 0 PID: 332 at fs/ext4/inode.c:5097 ext4_write_inode+0x140/0x1a0
	CPU: 0 PID: 332 Comm: xfsaild/vdc Not tainted 5.6.0-rc5 #5
	[...]
	RIP: 0010:ext4_write_inode+0x140/0x1a0 fs/ext4/inode.c:5097
	[...]
	Call Trace:
	 write_inode fs/fs-writeback.c:1312 [inline]
	 __writeback_single_inode+0x465/0x5f0 fs/fs-writeback.c:1511
	 writeback_single_inode+0xad/0x120 fs/fs-writeback.c:1565
	 sync_inode fs/fs-writeback.c:2602 [inline]
	 sync_inode_metadata+0x3d/0x57 fs/fs-writeback.c:2622
	 ext4_fsync_nojournal fs/ext4/fsync.c:94 [inline]
	 ext4_sync_file+0x243/0x4b0 fs/ext4/fsync.c:172
	 generic_write_sync include/linux/fs.h:2867 [inline]
	 ext4_buffered_write_iter+0xe1/0x130 fs/ext4/file.c:277
	 call_write_iter include/linux/fs.h:1901 [inline]
	 new_sync_write+0x130/0x1d0 fs/read_write.c:483
	 __kernel_write+0x54/0xe0 fs/read_write.c:515
	 do_acct_process+0x122/0x170 kernel/acct.c:522
	 slow_acct_process kernel/acct.c:581 [inline]
	 acct_process+0x1d4/0x27c kernel/acct.c:607
	 do_exit+0x83d/0xbc0 kernel/exit.c:791
	 kthread+0xf1/0x140 kernel/kthread.c:257
	 ret_from_fork+0x27/0x50 arch/x86/entry/entry_64.S:352

This case was originally reported by syzbot at
https://lore.kernel.org/r/0000000000000e7156059f751d7b@google.com.

Reported-by: syzbot+1f9dc49e8de2582d90c2@syzkaller.appspotmail.com
Signed-off-by: Eric Biggers <ebiggers@google.com>
---

v2: include more details in the commit message.

 fs/xfs/xfs_trans_ail.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 00cc5b8734be8..3bc570c90ad97 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -529,8 +529,9 @@ xfsaild(
 {
 	struct xfs_ail	*ailp = data;
 	long		tout = 0;	/* milliseconds */
+	unsigned int	noreclaim_flag;
 
-	current->flags |= PF_MEMALLOC;
+	noreclaim_flag = memalloc_noreclaim_save();
 	set_freezable();
 
 	while (1) {
@@ -601,6 +602,7 @@ xfsaild(
 		tout = xfsaild_push(ailp);
 	}
 
+	memalloc_noreclaim_restore(noreclaim_flag);
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH v2] xfs: clear PF_MEMALLOC before exiting xfsaild thread

[Brian Foster]

On Sun, Mar 08, 2020 at 09:34:30PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Leaving PF_MEMALLOC set when exiting a kthread causes it to remain set
> during do_exit().  That can confuse things.  For example, if BSD process
> accounting is enabled and the accounting file has FS_SYNC_FL set and is
> located on an ext4 filesystem without a journal, then do_exit() ends up
> calling ext4_write_inode().  That triggers the
> WARN_ON_ONCE(current->flags & PF_MEMALLOC) there, as it assumes
> (appropriately) that inodes aren't written when allocating memory.
> 
> Fix this in xfsaild() by using the helper functions to save and restore
> PF_MEMALLOC.
> 
> This can be reproduced as follows in the kvm-xfstests test appliance
> modified to add the 'acct' Debian package, and with kvm-xfstests's
> recommended kconfig modified to add CONFIG_BSD_PROCESS_ACCT=y:
> 
> 	mkfs.ext2 -F /dev/vdb
> 	mount /vdb -t ext4
> 	touch /vdb/file
> 	chattr +S /vdb/file
> 	accton /vdb/file
> 	mkfs.xfs -f /dev/vdc
> 	mount /vdc
> 	umount /vdc
> 
> It causes:
> 	WARNING: CPU: 0 PID: 332 at fs/ext4/inode.c:5097 ext4_write_inode+0x140/0x1a0
> 	CPU: 0 PID: 332 Comm: xfsaild/vdc Not tainted 5.6.0-rc5 #5
> 	[...]
> 	RIP: 0010:ext4_write_inode+0x140/0x1a0 fs/ext4/inode.c:5097
> 	[...]
> 	Call Trace:
> 	 write_inode fs/fs-writeback.c:1312 [inline]
> 	 __writeback_single_inode+0x465/0x5f0 fs/fs-writeback.c:1511
> 	 writeback_single_inode+0xad/0x120 fs/fs-writeback.c:1565
> 	 sync_inode fs/fs-writeback.c:2602 [inline]
> 	 sync_inode_metadata+0x3d/0x57 fs/fs-writeback.c:2622
> 	 ext4_fsync_nojournal fs/ext4/fsync.c:94 [inline]
> 	 ext4_sync_file+0x243/0x4b0 fs/ext4/fsync.c:172
> 	 generic_write_sync include/linux/fs.h:2867 [inline]
> 	 ext4_buffered_write_iter+0xe1/0x130 fs/ext4/file.c:277
> 	 call_write_iter include/linux/fs.h:1901 [inline]
> 	 new_sync_write+0x130/0x1d0 fs/read_write.c:483
> 	 __kernel_write+0x54/0xe0 fs/read_write.c:515
> 	 do_acct_process+0x122/0x170 kernel/acct.c:522
> 	 slow_acct_process kernel/acct.c:581 [inline]
> 	 acct_process+0x1d4/0x27c kernel/acct.c:607
> 	 do_exit+0x83d/0xbc0 kernel/exit.c:791
> 	 kthread+0xf1/0x140 kernel/kthread.c:257
> 	 ret_from_fork+0x27/0x50 arch/x86/entry/entry_64.S:352
> 
> This case was originally reported by syzbot at
> https://lore.kernel.org/r/0000000000000e7156059f751d7b@google.com.
> 
> Reported-by: syzbot+1f9dc49e8de2582d90c2@syzkaller.appspotmail.com
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

> 
> v2: include more details in the commit message.
> 
>  fs/xfs/xfs_trans_ail.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 00cc5b8734be8..3bc570c90ad97 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -529,8 +529,9 @@ xfsaild(
>  {
>  	struct xfs_ail	*ailp = data;
>  	long		tout = 0;	/* milliseconds */
> +	unsigned int	noreclaim_flag;
>  
> -	current->flags |= PF_MEMALLOC;
> +	noreclaim_flag = memalloc_noreclaim_save();
>  	set_freezable();
>  
>  	while (1) {
> @@ -601,6 +602,7 @@ xfsaild(
>  		tout = xfsaild_push(ailp);
>  	}
>  
> +	memalloc_noreclaim_restore(noreclaim_flag);
>  	return 0;
>  }
>  
> -- 
> 2.25.1
> 

Re: [PATCH v2] xfs: clear PF_MEMALLOC before exiting xfsaild thread

[Darrick J. Wong]

On Sun, Mar 08, 2020 at 09:34:30PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Leaving PF_MEMALLOC set when exiting a kthread causes it to remain set
> during do_exit().  That can confuse things.  For example, if BSD process
> accounting is enabled and the accounting file has FS_SYNC_FL set and is
> located on an ext4 filesystem without a journal, then do_exit() ends up
> calling ext4_write_inode().  That triggers the
> WARN_ON_ONCE(current->flags & PF_MEMALLOC) there, as it assumes
> (appropriately) that inodes aren't written when allocating memory.
> 
> Fix this in xfsaild() by using the helper functions to save and restore
> PF_MEMALLOC.
> 
> This can be reproduced as follows in the kvm-xfstests test appliance
> modified to add the 'acct' Debian package, and with kvm-xfstests's
> recommended kconfig modified to add CONFIG_BSD_PROCESS_ACCT=y:
> 
> 	mkfs.ext2 -F /dev/vdb
> 	mount /vdb -t ext4
> 	touch /vdb/file
> 	chattr +S /vdb/file

Does this trip if the process accounting file is also on an xfs
filesystem?

> 	accton /vdb/file
> 	mkfs.xfs -f /dev/vdc
> 	mount /vdc
> 	umount /vdc

...and if so, can this be turned into an fstests case, please?


> 
> It causes:
> 	WARNING: CPU: 0 PID: 332 at fs/ext4/inode.c:5097 ext4_write_inode+0x140/0x1a0
> 	CPU: 0 PID: 332 Comm: xfsaild/vdc Not tainted 5.6.0-rc5 #5
> 	[...]
> 	RIP: 0010:ext4_write_inode+0x140/0x1a0 fs/ext4/inode.c:5097
> 	[...]
> 	Call Trace:
> 	 write_inode fs/fs-writeback.c:1312 [inline]
> 	 __writeback_single_inode+0x465/0x5f0 fs/fs-writeback.c:1511
> 	 writeback_single_inode+0xad/0x120 fs/fs-writeback.c:1565
> 	 sync_inode fs/fs-writeback.c:2602 [inline]
> 	 sync_inode_metadata+0x3d/0x57 fs/fs-writeback.c:2622
> 	 ext4_fsync_nojournal fs/ext4/fsync.c:94 [inline]
> 	 ext4_sync_file+0x243/0x4b0 fs/ext4/fsync.c:172
> 	 generic_write_sync include/linux/fs.h:2867 [inline]
> 	 ext4_buffered_write_iter+0xe1/0x130 fs/ext4/file.c:277
> 	 call_write_iter include/linux/fs.h:1901 [inline]
> 	 new_sync_write+0x130/0x1d0 fs/read_write.c:483
> 	 __kernel_write+0x54/0xe0 fs/read_write.c:515
> 	 do_acct_process+0x122/0x170 kernel/acct.c:522
> 	 slow_acct_process kernel/acct.c:581 [inline]
> 	 acct_process+0x1d4/0x27c kernel/acct.c:607
> 	 do_exit+0x83d/0xbc0 kernel/exit.c:791
> 	 kthread+0xf1/0x140 kernel/kthread.c:257
> 	 ret_from_fork+0x27/0x50 arch/x86/entry/entry_64.S:352
> 
> This case was originally reported by syzbot at
> https://lore.kernel.org/r/0000000000000e7156059f751d7b@google.com.
> 
> Reported-by: syzbot+1f9dc49e8de2582d90c2@syzkaller.appspotmail.com
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> v2: include more details in the commit message.
> 
>  fs/xfs/xfs_trans_ail.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 00cc5b8734be8..3bc570c90ad97 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -529,8 +529,9 @@ xfsaild(
>  {
>  	struct xfs_ail	*ailp = data;
>  	long		tout = 0;	/* milliseconds */
> +	unsigned int	noreclaim_flag;
>  
> -	current->flags |= PF_MEMALLOC;
> +	noreclaim_flag = memalloc_noreclaim_save();
>  	set_freezable();
>  
>  	while (1) {
> @@ -601,6 +602,7 @@ xfsaild(
>  		tout = xfsaild_push(ailp);
>  	}
>  
> +	memalloc_noreclaim_restore(noreclaim_flag);
>  	return 0;
>  }
>  
> -- 
> 2.25.1
> 

Re: [PATCH v2] xfs: clear PF_MEMALLOC before exiting xfsaild thread

[Eric Biggers]

On Mon, Mar 09, 2020 at 09:24:39AM -0700, Darrick J. Wong wrote:
> On Sun, Mar 08, 2020 at 09:34:30PM -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Leaving PF_MEMALLOC set when exiting a kthread causes it to remain set
> > during do_exit().  That can confuse things.  For example, if BSD process
> > accounting is enabled and the accounting file has FS_SYNC_FL set and is
> > located on an ext4 filesystem without a journal, then do_exit() ends up
> > calling ext4_write_inode().  That triggers the
> > WARN_ON_ONCE(current->flags & PF_MEMALLOC) there, as it assumes
> > (appropriately) that inodes aren't written when allocating memory.
> > 
> > Fix this in xfsaild() by using the helper functions to save and restore
> > PF_MEMALLOC.
> > 
> > This can be reproduced as follows in the kvm-xfstests test appliance
> > modified to add the 'acct' Debian package, and with kvm-xfstests's
> > recommended kconfig modified to add CONFIG_BSD_PROCESS_ACCT=y:
> > 
> > 	mkfs.ext2 -F /dev/vdb
> > 	mount /vdb -t ext4
> > 	touch /vdb/file
> > 	chattr +S /vdb/file
> 
> Does this trip if the process accounting file is also on an xfs
> filesystem?
> 
> > 	accton /vdb/file
> > 	mkfs.xfs -f /dev/vdc
> > 	mount /vdc
> > 	umount /vdc
> 
> ...and if so, can this be turned into an fstests case, please?

I wasn't expecting it, but it turns out it does actually trip a similar warning
in iomap_do_writepage():

        mkfs.xfs -f /dev/vdb
        mount /vdb
        touch /vdb/file
        chattr +S /vdb/file
        accton /vdb/file
        mkfs.xfs -f /dev/vdc
        mount /vdc
        umount /vdc

causes...

	WARNING: CPU: 1 PID: 336 at fs/iomap/buffered-io.c:1534
	CPU: 1 PID: 336 Comm: xfsaild/vdc Not tainted 5.6.0-rc5 #3
	Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
	RIP: 0010:iomap_do_writepage+0x16b/0x1f0 fs/iomap/buffered-io.c:1534
	[...]
	Call Trace:
	 write_cache_pages+0x189/0x4d0 mm/page-writeback.c:2238
	 iomap_writepages+0x1c/0x33 fs/iomap/buffered-io.c:1642
	 xfs_vm_writepages+0x65/0x90 fs/xfs/xfs_aops.c:578
	 do_writepages+0x41/0xe0 mm/page-writeback.c:2344
	 __filemap_fdatawrite_range+0xd2/0x120 mm/filemap.c:421
	 file_write_and_wait_range+0x71/0xc0 mm/filemap.c:760
	 xfs_file_fsync+0x7a/0x2b0 fs/xfs/xfs_file.c:114
	 generic_write_sync include/linux/fs.h:2867 [inline]
	 xfs_file_buffered_aio_write+0x379/0x3b0 fs/xfs/xfs_file.c:691
	 call_write_iter include/linux/fs.h:1901 [inline]
	 new_sync_write+0x130/0x1d0 fs/read_write.c:483
	 __kernel_write+0x54/0xe0 fs/read_write.c:515
	 do_acct_process+0x122/0x170 kernel/acct.c:522
	 slow_acct_process kernel/acct.c:581 [inline]
	 acct_process+0x1d4/0x27c kernel/acct.c:607
	 do_exit+0x83d/0xbc0 kernel/exit.c:791
	 kthread+0xf1/0x140 kernel/kthread.c:257
	 ret_from_fork+0x27/0x50 arch/x86/entry/entry_64.S:352

So sure, since it's not necessarily a multi-filesystem thing, I can try to turn
it into an xfstest.  There's currently no way to enable BSD process accounting
in xfstests though, so we'll either need to make the test depend on the 'acct'
program or add a helper test program.

Also, do you want me to update the commit message again, to mention the above
case?

- Eric

Re: [PATCH v2] xfs: clear PF_MEMALLOC before exiting xfsaild thread

[Darrick J. Wong]

On Mon, Mar 09, 2020 at 11:04:52AM -0700, Eric Biggers wrote:
> On Mon, Mar 09, 2020 at 09:24:39AM -0700, Darrick J. Wong wrote:
> > On Sun, Mar 08, 2020 at 09:34:30PM -0700, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > Leaving PF_MEMALLOC set when exiting a kthread causes it to remain set
> > > during do_exit().  That can confuse things.  For example, if BSD process
> > > accounting is enabled and the accounting file has FS_SYNC_FL set and is
> > > located on an ext4 filesystem without a journal, then do_exit() ends up
> > > calling ext4_write_inode().  That triggers the
> > > WARN_ON_ONCE(current->flags & PF_MEMALLOC) there, as it assumes
> > > (appropriately) that inodes aren't written when allocating memory.
> > > 
> > > Fix this in xfsaild() by using the helper functions to save and restore
> > > PF_MEMALLOC.
> > > 
> > > This can be reproduced as follows in the kvm-xfstests test appliance
> > > modified to add the 'acct' Debian package, and with kvm-xfstests's
> > > recommended kconfig modified to add CONFIG_BSD_PROCESS_ACCT=y:
> > > 
> > > 	mkfs.ext2 -F /dev/vdb
> > > 	mount /vdb -t ext4
> > > 	touch /vdb/file
> > > 	chattr +S /vdb/file
> > 
> > Does this trip if the process accounting file is also on an xfs
> > filesystem?
> > 
> > > 	accton /vdb/file
> > > 	mkfs.xfs -f /dev/vdc
> > > 	mount /vdc
> > > 	umount /vdc
> > 
> > ...and if so, can this be turned into an fstests case, please?
> 
> I wasn't expecting it, but it turns out it does actually trip a similar warning
> in iomap_do_writepage():
> 
>         mkfs.xfs -f /dev/vdb
>         mount /vdb
>         touch /vdb/file
>         chattr +S /vdb/file
>         accton /vdb/file
>         mkfs.xfs -f /dev/vdc
>         mount /vdc
>         umount /vdc
> 
> causes...
> 
> 	WARNING: CPU: 1 PID: 336 at fs/iomap/buffered-io.c:1534
> 	CPU: 1 PID: 336 Comm: xfsaild/vdc Not tainted 5.6.0-rc5 #3
> 	Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
> 	RIP: 0010:iomap_do_writepage+0x16b/0x1f0 fs/iomap/buffered-io.c:1534
> 	[...]
> 	Call Trace:
> 	 write_cache_pages+0x189/0x4d0 mm/page-writeback.c:2238
> 	 iomap_writepages+0x1c/0x33 fs/iomap/buffered-io.c:1642
> 	 xfs_vm_writepages+0x65/0x90 fs/xfs/xfs_aops.c:578
> 	 do_writepages+0x41/0xe0 mm/page-writeback.c:2344
> 	 __filemap_fdatawrite_range+0xd2/0x120 mm/filemap.c:421
> 	 file_write_and_wait_range+0x71/0xc0 mm/filemap.c:760
> 	 xfs_file_fsync+0x7a/0x2b0 fs/xfs/xfs_file.c:114
> 	 generic_write_sync include/linux/fs.h:2867 [inline]
> 	 xfs_file_buffered_aio_write+0x379/0x3b0 fs/xfs/xfs_file.c:691
> 	 call_write_iter include/linux/fs.h:1901 [inline]
> 	 new_sync_write+0x130/0x1d0 fs/read_write.c:483
> 	 __kernel_write+0x54/0xe0 fs/read_write.c:515
> 	 do_acct_process+0x122/0x170 kernel/acct.c:522
> 	 slow_acct_process kernel/acct.c:581 [inline]
> 	 acct_process+0x1d4/0x27c kernel/acct.c:607
> 	 do_exit+0x83d/0xbc0 kernel/exit.c:791
> 	 kthread+0xf1/0x140 kernel/kthread.c:257
> 	 ret_from_fork+0x27/0x50 arch/x86/entry/entry_64.S:352
> 
> So sure, since it's not necessarily a multi-filesystem thing, I can try to turn
> it into an xfstest.  There's currently no way to enable BSD process accounting
> in xfstests though, so we'll either need to make the test depend on the 'acct'
> program or add a helper test program.
> 
> Also, do you want me to update the commit message again, to mention the above
> case?

I think it's worth mentioning that this is a general problem that
applies any time the process accounting file has that sync flag set,
since this problem isn't specific to ext4 + xfs.

(and now I wonder how many other places in the kernel suffer from these
kinds of file write surprises...)

--D

> - Eric

[PATCH v3] xfs: clear PF_MEMALLOC before exiting xfsaild thread

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Leaving PF_MEMALLOC set when exiting a kthread causes it to remain set
during do_exit().  That can confuse things.  In particular, if BSD
process accounting is enabled, then do_exit() writes data to an
accounting file.  If that file has FS_SYNC_FL set, then this write
occurs synchronously and can misbehave if PF_MEMALLOC is set.

For example, if the accounting file is located on an XFS filesystem,
then a WARN_ON_ONCE() in iomap_do_writepage() is triggered and the data
doesn't get written when it should.  Or if the accounting file is
located on an ext4 filesystem without a journal, then a WARN_ON_ONCE()
in ext4_write_inode() is triggered and the inode doesn't get written.

Fix this in xfsaild() by using the helper functions to save and restore
PF_MEMALLOC.

This can be reproduced as follows in the kvm-xfstests test appliance
modified to add the 'acct' Debian package, and with kvm-xfstests's
recommended kconfig modified to add CONFIG_BSD_PROCESS_ACCT=y:

        mkfs.xfs -f /dev/vdb
        mount /vdb
        touch /vdb/file
        chattr +S /vdb/file
        accton /vdb/file
        mkfs.xfs -f /dev/vdc
        mount /vdc
        umount /vdc

It causes:
	WARNING: CPU: 1 PID: 336 at fs/iomap/buffered-io.c:1534
	CPU: 1 PID: 336 Comm: xfsaild/vdc Not tainted 5.6.0-rc5 #3
	Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
	RIP: 0010:iomap_do_writepage+0x16b/0x1f0 fs/iomap/buffered-io.c:1534
	[...]
	Call Trace:
	 write_cache_pages+0x189/0x4d0 mm/page-writeback.c:2238
	 iomap_writepages+0x1c/0x33 fs/iomap/buffered-io.c:1642
	 xfs_vm_writepages+0x65/0x90 fs/xfs/xfs_aops.c:578
	 do_writepages+0x41/0xe0 mm/page-writeback.c:2344
	 __filemap_fdatawrite_range+0xd2/0x120 mm/filemap.c:421
	 file_write_and_wait_range+0x71/0xc0 mm/filemap.c:760
	 xfs_file_fsync+0x7a/0x2b0 fs/xfs/xfs_file.c:114
	 generic_write_sync include/linux/fs.h:2867 [inline]
	 xfs_file_buffered_aio_write+0x379/0x3b0 fs/xfs/xfs_file.c:691
	 call_write_iter include/linux/fs.h:1901 [inline]
	 new_sync_write+0x130/0x1d0 fs/read_write.c:483
	 __kernel_write+0x54/0xe0 fs/read_write.c:515
	 do_acct_process+0x122/0x170 kernel/acct.c:522
	 slow_acct_process kernel/acct.c:581 [inline]
	 acct_process+0x1d4/0x27c kernel/acct.c:607
	 do_exit+0x83d/0xbc0 kernel/exit.c:791
	 kthread+0xf1/0x140 kernel/kthread.c:257
	 ret_from_fork+0x27/0x50 arch/x86/entry/entry_64.S:352

This bug was originally reported by syzbot at
https://lore.kernel.org/r/0000000000000e7156059f751d7b@google.com.

Reported-by: syzbot+1f9dc49e8de2582d90c2@syzkaller.appspotmail.com
Signed-off-by: Eric Biggers <ebiggers@google.com>
---

v3: updated commit message again, this time to take into account the bug
    also being reproducible when the accounting file is located on XFS.

v2: include more details in the commit message.

 fs/xfs/xfs_trans_ail.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 00cc5b8734be8..3bc570c90ad97 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -529,8 +529,9 @@ xfsaild(
 {
 	struct xfs_ail	*ailp = data;
 	long		tout = 0;	/* milliseconds */
+	unsigned int	noreclaim_flag;
 
-	current->flags |= PF_MEMALLOC;
+	noreclaim_flag = memalloc_noreclaim_save();
 	set_freezable();
 
 	while (1) {
@@ -601,6 +602,7 @@ xfsaild(
 		tout = xfsaild_push(ailp);
 	}
 
+	memalloc_noreclaim_restore(noreclaim_flag);
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH v3] xfs: clear PF_MEMALLOC before exiting xfsaild thread

[Darrick J. Wong]

On Mon, Mar 09, 2020 at 11:57:14AM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Leaving PF_MEMALLOC set when exiting a kthread causes it to remain set
> during do_exit().  That can confuse things.  In particular, if BSD
> process accounting is enabled, then do_exit() writes data to an
> accounting file.  If that file has FS_SYNC_FL set, then this write
> occurs synchronously and can misbehave if PF_MEMALLOC is set.
> 
> For example, if the accounting file is located on an XFS filesystem,
> then a WARN_ON_ONCE() in iomap_do_writepage() is triggered and the data
> doesn't get written when it should.  Or if the accounting file is
> located on an ext4 filesystem without a journal, then a WARN_ON_ONCE()
> in ext4_write_inode() is triggered and the inode doesn't get written.
> 
> Fix this in xfsaild() by using the helper functions to save and restore
> PF_MEMALLOC.
> 
> This can be reproduced as follows in the kvm-xfstests test appliance
> modified to add the 'acct' Debian package, and with kvm-xfstests's
> recommended kconfig modified to add CONFIG_BSD_PROCESS_ACCT=y:
> 
>         mkfs.xfs -f /dev/vdb
>         mount /vdb
>         touch /vdb/file
>         chattr +S /vdb/file
>         accton /vdb/file
>         mkfs.xfs -f /dev/vdc
>         mount /vdc
>         umount /vdc
> 
> It causes:
> 	WARNING: CPU: 1 PID: 336 at fs/iomap/buffered-io.c:1534
> 	CPU: 1 PID: 336 Comm: xfsaild/vdc Not tainted 5.6.0-rc5 #3
> 	Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
> 	RIP: 0010:iomap_do_writepage+0x16b/0x1f0 fs/iomap/buffered-io.c:1534
> 	[...]
> 	Call Trace:
> 	 write_cache_pages+0x189/0x4d0 mm/page-writeback.c:2238
> 	 iomap_writepages+0x1c/0x33 fs/iomap/buffered-io.c:1642
> 	 xfs_vm_writepages+0x65/0x90 fs/xfs/xfs_aops.c:578
> 	 do_writepages+0x41/0xe0 mm/page-writeback.c:2344
> 	 __filemap_fdatawrite_range+0xd2/0x120 mm/filemap.c:421
> 	 file_write_and_wait_range+0x71/0xc0 mm/filemap.c:760
> 	 xfs_file_fsync+0x7a/0x2b0 fs/xfs/xfs_file.c:114
> 	 generic_write_sync include/linux/fs.h:2867 [inline]
> 	 xfs_file_buffered_aio_write+0x379/0x3b0 fs/xfs/xfs_file.c:691
> 	 call_write_iter include/linux/fs.h:1901 [inline]
> 	 new_sync_write+0x130/0x1d0 fs/read_write.c:483
> 	 __kernel_write+0x54/0xe0 fs/read_write.c:515
> 	 do_acct_process+0x122/0x170 kernel/acct.c:522
> 	 slow_acct_process kernel/acct.c:581 [inline]
> 	 acct_process+0x1d4/0x27c kernel/acct.c:607
> 	 do_exit+0x83d/0xbc0 kernel/exit.c:791
> 	 kthread+0xf1/0x140 kernel/kthread.c:257
> 	 ret_from_fork+0x27/0x50 arch/x86/entry/entry_64.S:352
> 
> This bug was originally reported by syzbot at
> https://lore.kernel.org/r/0000000000000e7156059f751d7b@google.com.
> 
> Reported-by: syzbot+1f9dc49e8de2582d90c2@syzkaller.appspotmail.com
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Seems reasonable to me, will give it a spin...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> v3: updated commit message again, this time to take into account the bug
>     also being reproducible when the accounting file is located on XFS.
> 
> v2: include more details in the commit message.
> 
>  fs/xfs/xfs_trans_ail.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 00cc5b8734be8..3bc570c90ad97 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -529,8 +529,9 @@ xfsaild(
>  {
>  	struct xfs_ail	*ailp = data;
>  	long		tout = 0;	/* milliseconds */
> +	unsigned int	noreclaim_flag;
>  
> -	current->flags |= PF_MEMALLOC;
> +	noreclaim_flag = memalloc_noreclaim_save();
>  	set_freezable();
>  
>  	while (1) {
> @@ -601,6 +602,7 @@ xfsaild(
>  		tout = xfsaild_push(ailp);
>  	}
>  
> +	memalloc_noreclaim_restore(noreclaim_flag);
>  	return 0;
>  }
>  
> -- 
> 2.25.1
> 

Re: [PATCH v3] xfs: clear PF_MEMALLOC before exiting xfsaild thread

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2] xfs: clear PF_MEMALLOC before exiting xfsaild thread

[Eric Biggers]

On Mon, Mar 09, 2020 at 09:24:39AM -0700, Darrick J. Wong wrote:
> > 
> > 	mkfs.ext2 -F /dev/vdb
> > 	mount /vdb -t ext4
> > 	touch /vdb/file
> > 	chattr +S /vdb/file
> 
> Does this trip if the process accounting file is also on an xfs
> filesystem?
> 
> > 	accton /vdb/file
> > 	mkfs.xfs -f /dev/vdc
> > 	mount /vdc
> > 	umount /vdc
> 
> ...and if so, can this be turned into an fstests case, please?
> 

Test sent out at
https://lkml.kernel.org/fstests/20200312221437.141484-1-ebiggers@kernel.org/

- Eric