1. Patchew
  2. linux
  3. View series

[PATCH] fs/ntfs3: Add missing direct_IO in ntfs_aops_cmpr

Lizhi Xu posted 1 patch 3 weeks, 6 days ago
There is a newer version of this series
fs/ntfs3/inode.c | 1 +
1 file changed, 1 insertion(+)
[PATCH] fs/ntfs3: Add missing direct_IO in ntfs_aops_cmpr
Posted by Lizhi Xu 3 weeks, 6 days ago 
The ntfs3 can use the page cache directly, so its address_space_operations
need direct_IO.

Fixes: b432163ebd15 ("fs/ntfs3: Update inode->i_mapping->a_ops on compression state")
Reported-by: syzbot+e36cc3297bd3afd25e19@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e36cc3297bd3afd25e19
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 fs/ntfs3/inode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
index 3e2957a1e360..50524f573d3a 100644
--- a/fs/ntfs3/inode.c
+++ b/fs/ntfs3/inode.c
@@ -2068,5 +2068,6 @@ const struct address_space_operations ntfs_aops_cmpr = {
 	.read_folio	= ntfs_read_folio,
 	.readahead	= ntfs_readahead,
 	.dirty_folio	= block_dirty_folio,
+	.direct_IO	= ntfs_direct_IO,
 };
 // clang-format on
-- 
2.43.0
Re: [PATCH] fs/ntfs3: Add missing direct_IO in ntfs_aops_cmpr
Posted by Christoph Hellwig 3 weeks, 3 days ago 
On Fri, Apr 11, 2025 at 09:24:27AM +0800, Lizhi Xu wrote:
> The ntfs3 can use the page cache directly, so its address_space_operations
> need direct_IO.

I can't parse that sentence.  What are you trying to say with it?
Re: [PATCH] fs/ntfs3: Add missing direct_IO in ntfs_aops_cmpr
Posted by Lizhi Xu 3 weeks, 2 days ago 
On Sun, 13 Apr 2025 22:50:54 -0700, Christoph Hellwig wrote:
> On Fri, Apr 11, 2025 at 09:24:27AM +0800, Lizhi Xu wrote:
> > The ntfs3 can use the page cache directly, so its address_space_operations
> > need direct_IO.
> 
> I can't parse that sentence.  What are you trying to say with it?
The comments [1] of generic_file_read_iter() clearly states "read_iter()
for all filesystems that can use the page cache directly".

In the calltrace of this example, it is clear that direct_IO is not set.
In [3], it is also clear that the lack of direct_IO in ntfs_aops_cmpr
caused this problem.

In summary, direct_IO must be set in this issue.

[1]
 * generic_file_read_iter - generic filesystem read routine
 * @iocb:	kernel I/O control block
 * @iter:	destination for the data read
 *
 * This is the "read_iter()" routine for all filesystems
 * that can use the page cache directly.

[2]
generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
{
	size_t count = iov_iter_count(iter);
	ssize_t retval = 0;

	if (!count)
		return 0; /* skip atime */

	if (iocb->ki_flags & IOCB_DIRECT) {
		struct file *file = iocb->ki_filp;
		struct address_space *mapping = file->f_mapping;
		struct inode *inode = mapping->host;

		retval = kiocb_write_and_wait(iocb, count);
		if (retval < 0)
			return retval;
		file_accessed(file);

		retval = mapping->a_ops->direct_IO(iocb, iter); 
[3]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b432163ebd15a0fb74051949cb61456d6c55ccbd
diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c
index 4d9d84cc3c6f55..9b6a3f8d2e7c5c 100644
--- a/fs/ntfs3/file.c
+++ b/fs/ntfs3/file.c
@@ -101,8 +101,26 @@ int ntfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
 	/* Allowed to change compression for empty files and for directories only. */
 	if (!is_dedup(ni) && !is_encrypted(ni) &&
 	    (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
-		/* Change compress state. */
-		int err = ni_set_compress(inode, flags & FS_COMPR_FL);
+		int err = 0;
+		struct address_space *mapping = inode->i_mapping;
+
+		/* write out all data and wait. */
+		filemap_invalidate_lock(mapping);
+		err = filemap_write_and_wait(mapping);
+
+		if (err >= 0) {
+			/* Change compress state. */
+			bool compr = flags & FS_COMPR_FL;
+			err = ni_set_compress(inode, compr);
+
+			/* For files change a_ops too. */
+			if (!err)
+				mapping->a_ops = compr ? &ntfs_aops_cmpr :
+							 &ntfs_aops;

BR,
Lizhi
Re: [PATCH] fs/ntfs3: Add missing direct_IO in ntfs_aops_cmpr
Posted by Jan Kara 3 weeks, 2 days ago 
On Tue 15-04-25 09:05:18, Lizhi Xu wrote:
> On Sun, 13 Apr 2025 22:50:54 -0700, Christoph Hellwig wrote:
> > On Fri, Apr 11, 2025 at 09:24:27AM +0800, Lizhi Xu wrote:
> > > The ntfs3 can use the page cache directly, so its address_space_operations
> > > need direct_IO.
> > 
> > I can't parse that sentence.  What are you trying to say with it?
> The comments [1] of generic_file_read_iter() clearly states "read_iter()
> for all filesystems that can use the page cache directly".
> 
> In the calltrace of this example, it is clear that direct_IO is not set.
> In [3], it is also clear that the lack of direct_IO in ntfs_aops_cmpr
> caused this problem.
> 
> In summary, direct_IO must be set in this issue.

I agree that you need to set .direct_IO in ntfs_aops_cmpr but since
compressed files do not *support* direct IO (at least I don't see any such
support in ntfs_direct_IO()) you either need to also handle these files in
ntfs_direct_IO() or you need to set special direct IO handler that will
just return 0 and thus fall back to buffered IO. So I don't think your
patch is correct as is.

									Honza

> [1]
>  * generic_file_read_iter - generic filesystem read routine
>  * @iocb:	kernel I/O control block
>  * @iter:	destination for the data read
>  *
>  * This is the "read_iter()" routine for all filesystems
>  * that can use the page cache directly.
> 
> [2]
> generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> {
> 	size_t count = iov_iter_count(iter);
> 	ssize_t retval = 0;
> 
> 	if (!count)
> 		return 0; /* skip atime */
> 
> 	if (iocb->ki_flags & IOCB_DIRECT) {
> 		struct file *file = iocb->ki_filp;
> 		struct address_space *mapping = file->f_mapping;
> 		struct inode *inode = mapping->host;
> 
> 		retval = kiocb_write_and_wait(iocb, count);
> 		if (retval < 0)
> 			return retval;
> 		file_accessed(file);
> 
> 		retval = mapping->a_ops->direct_IO(iocb, iter); 
> [3]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b432163ebd15a0fb74051949cb61456d6c55ccbd
> diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c
> index 4d9d84cc3c6f55..9b6a3f8d2e7c5c 100644
> --- a/fs/ntfs3/file.c
> +++ b/fs/ntfs3/file.c
> @@ -101,8 +101,26 @@ int ntfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
>  	/* Allowed to change compression for empty files and for directories only. */
>  	if (!is_dedup(ni) && !is_encrypted(ni) &&
>  	    (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
> -		/* Change compress state. */
> -		int err = ni_set_compress(inode, flags & FS_COMPR_FL);
> +		int err = 0;
> +		struct address_space *mapping = inode->i_mapping;
> +
> +		/* write out all data and wait. */
> +		filemap_invalidate_lock(mapping);
> +		err = filemap_write_and_wait(mapping);
> +
> +		if (err >= 0) {
> +			/* Change compress state. */
> +			bool compr = flags & FS_COMPR_FL;
> +			err = ni_set_compress(inode, compr);
> +
> +			/* For files change a_ops too. */
> +			if (!err)
> +				mapping->a_ops = compr ? &ntfs_aops_cmpr :
> +							 &ntfs_aops;
> 
> BR,
> Lizhi
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR
[PATCH V2] fs/ntfs3: Add missing direct_IO in ntfs_aops_cmpr
Posted by Lizhi Xu 3 weeks, 2 days ago 
The ntfs3 can use the page cache directly, so its address_space_operations
need direct_IO. Exit ntfs_direct_IO() if it is a compressed file.

Fixes: b432163ebd15 ("fs/ntfs3: Update inode->i_mapping->a_ops on compression state")
Reported-by: syzbot+e36cc3297bd3afd25e19@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e36cc3297bd3afd25e19
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
V1 -> V2: exit direct io if it is a compressed file.

 fs/ntfs3/inode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
index 3e2957a1e360..0f0d27d4644a 100644
--- a/fs/ntfs3/inode.c
+++ b/fs/ntfs3/inode.c
@@ -805,6 +805,10 @@ static ssize_t ntfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 		ret = 0;
 		goto out;
 	}
+	if (is_compressed(ni)) {
+		ret = 0;
+		goto out;
+	}
 
 	ret = blockdev_direct_IO(iocb, inode, iter,
 				 wr ? ntfs_get_block_direct_IO_W :
@@ -2068,5 +2072,6 @@ const struct address_space_operations ntfs_aops_cmpr = {
 	.read_folio	= ntfs_read_folio,
 	.readahead	= ntfs_readahead,
 	.dirty_folio	= block_dirty_folio,
+	.direct_IO	= ntfs_direct_IO,
 };
 // clang-format on
-- 
2.43.0
Re: [PATCH V2] fs/ntfs3: Add missing direct_IO in ntfs_aops_cmpr
Posted by Christoph Hellwig 3 weeks, 1 day ago 
On Tue, Apr 15, 2025 at 05:26:37PM +0800, Lizhi Xu wrote:
> The ntfs3 can use the page cache directly, so its address_space_operations
> need direct_IO. 

This sentence still does not make any sense.
Re: [PATCH V2] fs/ntfs3: Add missing direct_IO in ntfs_aops_cmpr
Posted by Lizhi Xu 3 weeks, 1 day ago 
On Tue, 15 Apr 2025 21:37:04 -0700, Christoph Hellwig wrote:
> > The ntfs3 can use the page cache directly, so its address_space_operations
> > need direct_IO.
> 
> This sentence still does not make any sense.
Did you see the following comments?
https://lore.kernel.org/all/20250415010518.2008216-1-lizhi.xu@windriver.com/
Re: [PATCH V2] fs/ntfs3: Add missing direct_IO in ntfs_aops_cmpr
Posted by Christoph Hellwig 3 weeks, 1 day ago 
On Wed, Apr 16, 2025 at 01:34:26PM +0800, Lizhi Xu wrote:
> On Tue, 15 Apr 2025 21:37:04 -0700, Christoph Hellwig wrote:
> > > The ntfs3 can use the page cache directly, so its address_space_operations
> > > need direct_IO.
> > 
> > This sentence still does not make any sense.
> Did you see the following comments?
> https://lore.kernel.org/all/20250415010518.2008216-1-lizhi.xu@windriver.com/

I did, but that changes nothing about the fact that the above sentence
doesn't make sense.
Re: [PATCH V2] fs/ntfs3: Add missing direct_IO in ntfs_aops_cmpr
Posted by Lizhi Xu 3 weeks, 1 day ago 
On Tue, 15 Apr 2025 22:35:30 -0700, Christoph Hellwig wrote:
> > > > The ntfs3 can use the page cache directly, so its address_space_operations
> > > > need direct_IO.
> > >
> > > This sentence still does not make any sense.
> > Did you see the following comments?
> > https://lore.kernel.org/all/20250415010518.2008216-1-lizhi.xu@windriver.com/
> 
> I did, but that changes nothing about the fact that the above sentence
> doesn't make sense.
In the reproducer, the second file passed in by the system call sendfile()
sets the file flag O_DIRECT when opening the file, which bypasses the page
cache and accesses the direct io interface of the ntfs3 file system.
However, ntfs3 does not set direct_IO for compressed files in ntfs_aops_cmpr.
Re: [PATCH V2] fs/ntfs3: Add missing direct_IO in ntfs_aops_cmpr
Posted by Christoph Hellwig 3 weeks, 1 day ago 
On Wed, Apr 16, 2025 at 02:03:51PM +0800, Lizhi Xu wrote:
> In the reproducer, the second file passed in by the system call sendfile()
> sets the file flag O_DIRECT when opening the file, which bypasses the page
> cache and accesses the direct io interface of the ntfs3 file system.
> However, ntfs3 does not set direct_IO for compressed files in ntfs_aops_cmpr.

Not allowing direct I/O is perfectly fine.  If you think you need to
support direct I/O for this case it is also fine.  But none of this
has anything to do with 'can use the page cache' and there are also
plenty of ways to support direct I/O without ->direct_IO.
Re: [PATCH V2] fs/ntfs3: Add missing direct_IO in ntfs_aops_cmpr
Posted by Lizhi Xu 3 weeks, 1 day ago 
On Tue, 15 Apr 2025 23:05:56 -0700, Christoph Hellwig wrote:
> > In the reproducer, the second file passed in by the system call sendfile()
> > sets the file flag O_DIRECT when opening the file, which bypasses the page
> > cache and accesses the direct io interface of the ntfs3 file system.
> > However, ntfs3 does not set direct_IO for compressed files in ntfs_aops_cmpr.
> 
> Not allowing direct I/O is perfectly fine.  If you think you need to
> support direct I/O for this case it is also fine.  But none of this
> has anything to do with 'can use the page cache' and there are also
The "The ntfs3 can use the page cache directly" I mentioned in the patch
is to explain that the calltrace is the direct I/O of ntfs3 called from
generic_file_read_iter().
Re: [PATCH V2] fs/ntfs3: Add missing direct_IO in ntfs_aops_cmpr
Posted by Jan Kara 3 weeks, 1 day ago 
On Tue 15-04-25 17:26:37, Lizhi Xu wrote:
> The ntfs3 can use the page cache directly, so its address_space_operations
> need direct_IO. Exit ntfs_direct_IO() if it is a compressed file.
> 
> Fixes: b432163ebd15 ("fs/ntfs3: Update inode->i_mapping->a_ops on compression state")
> Reported-by: syzbot+e36cc3297bd3afd25e19@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e36cc3297bd3afd25e19
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>

OK, this looks sensible to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> V1 -> V2: exit direct io if it is a compressed file.
> 
>  fs/ntfs3/inode.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
> index 3e2957a1e360..0f0d27d4644a 100644
> --- a/fs/ntfs3/inode.c
> +++ b/fs/ntfs3/inode.c
> @@ -805,6 +805,10 @@ static ssize_t ntfs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>  		ret = 0;
>  		goto out;
>  	}
> +	if (is_compressed(ni)) {
> +		ret = 0;
> +		goto out;
> +	}
>  
>  	ret = blockdev_direct_IO(iocb, inode, iter,
>  				 wr ? ntfs_get_block_direct_IO_W :
> @@ -2068,5 +2072,6 @@ const struct address_space_operations ntfs_aops_cmpr = {
>  	.read_folio	= ntfs_read_folio,
>  	.readahead	= ntfs_readahead,
>  	.dirty_folio	= block_dirty_folio,
> +	.direct_IO	= ntfs_direct_IO,
>  };
>  // clang-format on
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.