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

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: <2B76632D-2642-4892-A721-8AC8DF48A8A4@dilger.ca>
Date:   Mon, 14 Mar 2022 13:55:34 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Artem Blagodarenko <artem.blagodarenko@...il.com>
Cc:     linux-ext4 <linux-ext4@...r.kernel.org>,
        Andrew Perepechko <andrew.perepechko@....com>
Subject: Re: [PATCH v2] ext4: truncate during setxattr leads to kernel panic

On Mar 12, 2022, at 4:18 PM, Artem Blagodarenko <artem.blagodarenko@...il.com> wrote:
> 
> From: Andrew Perepechko <andrew.perepechko@....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@....com>


Reviewed-by: Andreas Dilger <adilger@...ger.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






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)