[PATCH v2] ext4: truncate during setxattr leads to kernel panic

[PATCH v2] ext4: truncate during setxattr leads to kernel panic

[Artem Blagodarenko]

From: Andrew Perepechko <andrew.perepechko@hpe.com>

When changing a large xattr value to a different large xattr value,
the old xattr inode is freed. Truncate during the final iput causes
current transaction restart. Eventually, parent inode bh is marked
dirty and kernel panic happens when jbd2 figures out that this bh
belongs to the committed transaction.

A possible fix is to call this final iput in a separate thread.
This way, setxattr transactions will never be split into two.
Since the setxattr code adds xattr inodes with nlink=0 into the
orphan list, old xattr inodes will be properly cleaned up in
any case.

Signed-off-by: Andrew Perepechko <andrew.perepechko@hpe.com>
HPE-bug-id: LUS-10534

Changes since v1:
- fixed bug added during the porting

---
 fs/ext4/super.c |  1 +
 fs/ext4/xattr.c | 34 ++++++++++++++++++++++++++++++++--
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c5021ca0a28a..8c04c19fa4b8 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1199,6 +1199,7 @@ static void ext4_put_super(struct super_block *sb)
 	int aborted = 0;
 	int i, err;
 
+	flush_scheduled_work();
 	ext4_unregister_li_request(sb);
 	ext4_quota_off_umount(sb);
 
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 042325349098..13c396e354c8 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1544,6 +1544,31 @@ static int ext4_xattr_inode_lookup_create(handle_t *handle, struct inode *inode,
 	return 0;
 }
 
+struct delayed_iput_work {
+	struct work_struct work;
+	struct inode *inode;
+};
+
+static void delayed_iput_fn(struct work_struct *work)
+{
+	struct delayed_iput_work *diwork;
+
+	diwork = container_of(work, struct delayed_iput_work, work);
+	iput(diwork->inode);
+	kfree(diwork);
+}
+
+static void delayed_iput(struct inode *inode, struct delayed_iput_work *work)
+{
+	if (!work) {
+		iput(inode);
+	} else {
+		INIT_WORK(&work->work, delayed_iput_fn);
+		work->inode = inode;
+		schedule_work(&work->work);
+	}
+}
+
 /*
  * Reserve min(block_size/8, 1024) bytes for xattr entries/names if ea_inode
  * feature is enabled.
@@ -1561,6 +1586,7 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
 	int in_inode = i->in_inode;
 	struct inode *old_ea_inode = NULL;
 	struct inode *new_ea_inode = NULL;
+	struct delayed_iput_work *diwork = NULL;
 	size_t old_size, new_size;
 	int ret;
 
@@ -1637,7 +1663,11 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
 	 * Finish that work before doing any modifications to the xattr data.
 	 */
 	if (!s->not_found && here->e_value_inum) {
-		ret = ext4_xattr_inode_iget(inode,
+		diwork = kmalloc(sizeof(*diwork), GFP_NOFS);
+		if (!diwork)
+			ret = -ENOMEM;
+		else
+			ret = ext4_xattr_inode_iget(inode,
 					    le32_to_cpu(here->e_value_inum),
 					    le32_to_cpu(here->e_hash),
 					    &old_ea_inode);
@@ -1790,7 +1820,7 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
 
 	ret = 0;
 out:
-	iput(old_ea_inode);
+	delayed_iput(old_ea_inode, diwork);
 	iput(new_ea_inode);
 	return ret;
 }
-- 
2.31.1

Re: [PATCH v2] ext4: truncate during setxattr leads to kernel panic

[Andreas Dilger]

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

On Mar 12, 2022, at 4:18 PM, Artem Blagodarenko <artem.blagodarenko@gmail.com> wrote:
> 
> From: Andrew Perepechko <andrew.perepechko@hpe.com>
> 
> When changing a large xattr value to a different large xattr value,
> the old xattr inode is freed. Truncate during the final iput causes
> current transaction restart. Eventually, parent inode bh is marked
> dirty and kernel panic happens when jbd2 figures out that this bh
> belongs to the committed transaction.
> 
> A possible fix is to call this final iput in a separate thread.
> This way, setxattr transactions will never be split into two.
> Since the setxattr code adds xattr inodes with nlink=0 into the
> orphan list, old xattr inodes will be properly cleaned up in
> any case.
> 
> Signed-off-by: Andrew Perepechko <andrew.perepechko@hpe.com>


Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> HPE-bug-id: LUS-10534
> 
> Changes since v1:
> - fixed bug added during the porting
> 
> ---
> fs/ext4/super.c |  1 +
> fs/ext4/xattr.c | 34 ++++++++++++++++++++++++++++++++--
> 2 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index c5021ca0a28a..8c04c19fa4b8 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1199,6 +1199,7 @@ static void ext4_put_super(struct super_block *sb)
> 	int aborted = 0;
> 	int i, err;
> 
> +	flush_scheduled_work();
> 	ext4_unregister_li_request(sb);
> 	ext4_quota_off_umount(sb);
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 042325349098..13c396e354c8 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -1544,6 +1544,31 @@ static int ext4_xattr_inode_lookup_create(handle_t *handle, struct inode *inode,
> 	return 0;
> }
> 
> +struct delayed_iput_work {
> +	struct work_struct work;
> +	struct inode *inode;
> +};
> +
> +static void delayed_iput_fn(struct work_struct *work)
> +{
> +	struct delayed_iput_work *diwork;
> +
> +	diwork = container_of(work, struct delayed_iput_work, work);
> +	iput(diwork->inode);
> +	kfree(diwork);
> +}
> +
> +static void delayed_iput(struct inode *inode, struct delayed_iput_work *work)
> +{
> +	if (!work) {
> +		iput(inode);
> +	} else {
> +		INIT_WORK(&work->work, delayed_iput_fn);
> +		work->inode = inode;
> +		schedule_work(&work->work);
> +	}
> +}
> +
> /*
>  * Reserve min(block_size/8, 1024) bytes for xattr entries/names if ea_inode
>  * feature is enabled.
> @@ -1561,6 +1586,7 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
> 	int in_inode = i->in_inode;
> 	struct inode *old_ea_inode = NULL;
> 	struct inode *new_ea_inode = NULL;
> +	struct delayed_iput_work *diwork = NULL;
> 	size_t old_size, new_size;
> 	int ret;
> 
> @@ -1637,7 +1663,11 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
> 	 * Finish that work before doing any modifications to the xattr data.
> 	 */
> 	if (!s->not_found && here->e_value_inum) {
> -		ret = ext4_xattr_inode_iget(inode,
> +		diwork = kmalloc(sizeof(*diwork), GFP_NOFS);
> +		if (!diwork)
> +			ret = -ENOMEM;
> +		else
> +			ret = ext4_xattr_inode_iget(inode,
> 					    le32_to_cpu(here->e_value_inum),
> 					    le32_to_cpu(here->e_hash),
> 					    &old_ea_inode);
> @@ -1790,7 +1820,7 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
> 
> 	ret = 0;
> out:
> -	iput(old_ea_inode);
> +	delayed_iput(old_ea_inode, diwork);
> 	iput(new_ea_inode);
> 	return ret;
> }
> --
> 2.31.1
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH v2] ext4: truncate during setxattr leads to kernel panic

[Theodore Ts'o]

On Sat, 12 Mar 2022 18:18:30 -0500, Artem Blagodarenko wrote:
> From: Andrew Perepechko <andrew.perepechko@hpe.com>
> 
> When changing a large xattr value to a different large xattr value,
> the old xattr inode is freed. Truncate during the final iput causes
> current transaction restart. Eventually, parent inode bh is marked
> dirty and kernel panic happens when jbd2 figures out that this bh
> belongs to the committed transaction.
> 
> [...]

Applied, thanks!

[1/1] ext4: truncate during setxattr leads to kernel panic
      commit: c7cded845fc192cc35a1ca37c0cd957ee35abdf8

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>

Re: [PATCH v2] ext4: truncate during setxattr leads to kernel panic

[Theodore Ts'o]

On Fri, Mar 25, 2022 at 02:49:50PM -0400, Theodore Ts'o wrote:
> On Sat, 12 Mar 2022 18:18:30 -0500, Artem Blagodarenko wrote:
> > From: Andrew Perepechko <andrew.perepechko@hpe.com>
> > 
> > When changing a large xattr value to a different large xattr value,
> > the old xattr inode is freed. Truncate during the final iput causes
> > current transaction restart. Eventually, parent inode bh is marked
> > dirty and kernel panic happens when jbd2 figures out that this bh
> > belongs to the committed transaction.
> > 
> > [...]
> 
> Applied, thanks!
> 
> [1/1] ext4: truncate during setxattr leads to kernel panic
>       commit: c7cded845fc192cc35a1ca37c0cd957ee35abdf8

I'm going to drop this patch from the dev branch for now, due to the
issue pointed out here[1].  The solution, as suggested in [2], is we
need to use our own subsystem workqueue.  Perhaps we can reuse
rsv_conversion_wq for that purpose (after renaming it, of course).

[1] https://lore.kernel.org/all/385ce718-f965-4005-56b6-34922c4533b8@I-love.SAKURA.ne.jp/
[2] https://lore.kernel.org/all/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp/T/#u

						- Ted