linux-kernel - Re: [PATCH] fs/ntfs3: Update inode->i_mapping->a

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: <Z5KljQpaTp9ZlJxj@dread.disaster.area>
Date: Fri, 24 Jan 2025 07:24:45 +1100
From: Dave Chinner <david@...morbit.com>
To: Konstantin Komarov <almaz.alexandrovich@...agon-software.com>
Cc: ntfs3@...ts.linux.dev, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, Jiaji Qin <jjtan24@...udan.edu.cn>,
	Kun Hu <huk23@...udan.edu.cn>
Subject: Re: [PATCH] fs/ntfs3: Update inode->i_mapping->a_ops on compression
 state change

On Thu, Jan 23, 2025 at 04:53:35PM +0300, Konstantin Komarov wrote:
> Update inode->i_mapping->a_ops when the compression state changes to
> ensure correct address space operations.
> Clear ATTR_FLAG_SPARSED/FILE_ATTRIBUTE_SPARSE_FILE when enabling
> compression to prevent flag conflicts.
> 
> Fixes: 6b39bfaeec44 ("fs/ntfs3: Add support for the compression attribute")
> Reported-by: Kun Hu <huk23@...udan.edu.cn>, Jiaji Qin <jjtan24@...udan.edu.cn>
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@...agon-software.com>
> ---
>  fs/ntfs3/attrib.c  | 1 +
>  fs/ntfs3/file.c    | 2 ++
>  fs/ntfs3/frecord.c | 6 ++++--
>  3 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ntfs3/attrib.c b/fs/ntfs3/attrib.c
> index af94e3737470..d2410ab6c7bf 100644
> --- a/fs/ntfs3/attrib.c
> +++ b/fs/ntfs3/attrib.c
> @@ -2666,6 +2666,7 @@ int attr_set_compress(struct ntfs_inode *ni, bool compr)
>  
>  	/* Update data attribute flags. */
>  	if (compr) {
> +		attr->flags &= ATTR_FLAG_SPARSED;
>  		attr->flags |= ATTR_FLAG_COMPRESSED;
>  		attr->nres.c_unit = NTFS_LZNT_CUNIT;
>  	} else {
> diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c
> index 3f96a11804c9..e8f452f47cd5 100644
> --- a/fs/ntfs3/file.c
> +++ b/fs/ntfs3/file.c
> @@ -105,6 +105,8 @@ int ntfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
>  		int err = ni_set_compress(inode, flags & FS_COMPR_FL);
>  		if (err)
>  			return err;
> +		inode->i_mapping->a_ops =
> +			(flags & FS_COMPR_FL) ? &ntfs_aops_cmpr : &ntfs_aops;

It is not safe to change a_ops dynamically like this.

There can be other operations running concurrently using the
existing aops, and some functionality (e.g. the page fault paths)
have dependencies between aops methods and swapping the aops between
those operations can cause all sorts of problem (including crashing
the kernel).

This is the reason why we cannot turn functionality like FSDAX on
and off via fileattr_set() methods setting/clearing inode flags.
The inode has to transition out of cache and be re-instantiated to
change the inode aops methods safely...

-Dave.
-- 
Dave Chinner
david@...morbit.com