[PATCH] libfs: fix infinite directory reads for offset dir

[PATCH] libfs: fix infinite directory reads for offset dir

[ciprietti]

From: yangerkun <yangerkun@huawei.com>

[ Upstream commit 64a7ce76fb901bf9f9c36cf5d681328fc0fd4b5a ]

After we switch tmpfs dir operations from simple_dir_operations to
simple_offset_dir_operations, every rename happened will fill new dentry
to dest dir's maple tree(&SHMEM_I(inode)->dir_offsets->mt) with a free
key starting with octx->newx_offset, and then set newx_offset equals to
free key + 1. This will lead to infinite readdir combine with rename
happened at the same time, which fail generic/736 in xfstests(detail show
as below).

1. create 5000 files(1 2 3...) under one dir
2. call readdir(man 3 readdir) once, and get one entry
3. rename(entry, "TEMPFILE"), then rename("TEMPFILE", entry)
4. loop 2~3, until readdir return nothing or we loop too many
   times(tmpfs break test with the second condition)

We choose the same logic what commit 9b378f6ad48cf ("btrfs: fix infinite
directory reads") to fix it, record the last_index when we open dir, and
do not emit the entry which index >= last_index. The file->private_data
now used in offset dir can use directly to do this, and we also update
the last_index when we llseek the dir file.

Fixes: a2e459555c5f ("shmem: stable directory offsets")
Signed-off-by: yangerkun <yangerkun@huawei.com>
Link: https://lore.kernel.org/r/20240731043835.1828697-1-yangerkun@huawei.com
Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
[brauner: only update last_index after seek when offset is zero like Jan suggested]
Signed-off-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Andrea Ciprietti <ciprietti@google.com>
---
 fs/libfs.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index dc0f7519045f..916c39e758b1 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -371,6 +371,15 @@ void simple_offset_destroy(struct offset_ctx *octx)
 	xa_destroy(&octx->xa);
 }
 
+static int offset_dir_open(struct inode *inode, struct file *file)
+{
+	struct offset_ctx *ctx = inode->i_op->get_offset_ctx(inode);
+	unsigned long next_offset = (unsigned long)ctx->next_offset;
+
+	file->private_data = (void *)next_offset;
+	return 0;
+}
+
 /**
  * offset_dir_llseek - Advance the read position of a directory descriptor
  * @file: an open directory whose position is to be updated
@@ -384,6 +393,9 @@ void simple_offset_destroy(struct offset_ctx *octx)
  */
 static loff_t offset_dir_llseek(struct file *file, loff_t offset, int whence)
 {
+	struct inode *inode = file->f_inode;
+	struct offset_ctx *ctx = inode->i_op->get_offset_ctx(inode);
+
 	switch (whence) {
 	case SEEK_CUR:
 		offset += file->f_pos;
@@ -397,7 +409,11 @@ static loff_t offset_dir_llseek(struct file *file, loff_t offset, int whence)
 	}
 
 	/* In this case, ->private_data is protected by f_pos_lock */
-	file->private_data = NULL;
+	if (!offset) {
+		unsigned long next_offset = (unsigned long)ctx->next_offset;
+
+		file->private_data = (void *)next_offset;
+	}
 	return vfs_setpos(file, offset, U32_MAX);
 }
 
@@ -427,7 +443,7 @@ static bool offset_dir_emit(struct dir_context *ctx, struct dentry *dentry)
 			  inode->i_ino, fs_umode_to_dtype(inode->i_mode));
 }
 
-static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
+static void offset_iterate_dir(struct inode *inode, struct dir_context *ctx, long last_index)
 {
 	struct offset_ctx *so_ctx = inode->i_op->get_offset_ctx(inode);
 	XA_STATE(xas, &so_ctx->xa, ctx->pos);
@@ -436,17 +452,21 @@ static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
 	while (true) {
 		dentry = offset_find_next(&xas);
 		if (!dentry)
-			return ERR_PTR(-ENOENT);
+			return;
+
+		if (dentry2offset(dentry) >= last_index) {
+			dput(dentry);
+			return;
+		}
 
 		if (!offset_dir_emit(ctx, dentry)) {
 			dput(dentry);
-			break;
+			return;
 		}
 
 		dput(dentry);
 		ctx->pos = xas.xa_index + 1;
 	}
-	return NULL;
 }
 
 /**
@@ -473,22 +493,19 @@ static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
 static int offset_readdir(struct file *file, struct dir_context *ctx)
 {
 	struct dentry *dir = file->f_path.dentry;
+	long last_index = (long)file->private_data;
 
 	lockdep_assert_held(&d_inode(dir)->i_rwsem);
 
 	if (!dir_emit_dots(file, ctx))
 		return 0;
 
-	/* In this case, ->private_data is protected by f_pos_lock */
-	if (ctx->pos == 2)
-		file->private_data = NULL;
-	else if (file->private_data == ERR_PTR(-ENOENT))
-		return 0;
-	file->private_data = offset_iterate_dir(d_inode(dir), ctx);
+	offset_iterate_dir(d_inode(dir), ctx, last_index);
 	return 0;
 }
 
 const struct file_operations simple_offset_dir_operations = {
+	.open		= offset_dir_open,
 	.llseek		= offset_dir_llseek,
 	.iterate_shared	= offset_readdir,
 	.read		= generic_read_dir,
-- 
2.48.1.262.g85cc9f2d1e-goog

Re: [PATCH] libfs: fix infinite directory reads for offset dir

[Chuck Lever]

On 1/28/25 10:03 AM, ciprietti@google.com wrote:
> From: yangerkun <yangerkun@huawei.com>
> 
> [ Upstream commit 64a7ce76fb901bf9f9c36cf5d681328fc0fd4b5a ]
> 
> After we switch tmpfs dir operations from simple_dir_operations to
> simple_offset_dir_operations, every rename happened will fill new dentry
> to dest dir's maple tree(&SHMEM_I(inode)->dir_offsets->mt) with a free
> key starting with octx->newx_offset, and then set newx_offset equals to
> free key + 1. This will lead to infinite readdir combine with rename
> happened at the same time, which fail generic/736 in xfstests(detail show
> as below).
> 
> 1. create 5000 files(1 2 3...) under one dir
> 2. call readdir(man 3 readdir) once, and get one entry
> 3. rename(entry, "TEMPFILE"), then rename("TEMPFILE", entry)
> 4. loop 2~3, until readdir return nothing or we loop too many
>     times(tmpfs break test with the second condition)
> 
> We choose the same logic what commit 9b378f6ad48cf ("btrfs: fix infinite
> directory reads") to fix it, record the last_index when we open dir, and
> do not emit the entry which index >= last_index. The file->private_data
> now used in offset dir can use directly to do this, and we also update
> the last_index when we llseek the dir file.
> 
> Fixes: a2e459555c5f ("shmem: stable directory offsets")
> Signed-off-by: yangerkun <yangerkun@huawei.com>
> Link: https://lore.kernel.org/r/20240731043835.1828697-1-yangerkun@huawei.com
> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> [brauner: only update last_index after seek when offset is zero like Jan suggested]
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Andrea Ciprietti <ciprietti@google.com>
> ---
>   fs/libfs.c | 39 ++++++++++++++++++++++++++++-----------
>   1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index dc0f7519045f..916c39e758b1 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -371,6 +371,15 @@ void simple_offset_destroy(struct offset_ctx *octx)
>   	xa_destroy(&octx->xa);
>   }
>   
> +static int offset_dir_open(struct inode *inode, struct file *file)
> +{
> +	struct offset_ctx *ctx = inode->i_op->get_offset_ctx(inode);
> +	unsigned long next_offset = (unsigned long)ctx->next_offset;
> +
> +	file->private_data = (void *)next_offset;
> +	return 0;
> +}
> +
>   /**
>    * offset_dir_llseek - Advance the read position of a directory descriptor
>    * @file: an open directory whose position is to be updated
> @@ -384,6 +393,9 @@ void simple_offset_destroy(struct offset_ctx *octx)
>    */
>   static loff_t offset_dir_llseek(struct file *file, loff_t offset, int whence)
>   {
> +	struct inode *inode = file->f_inode;
> +	struct offset_ctx *ctx = inode->i_op->get_offset_ctx(inode);
> +
>   	switch (whence) {
>   	case SEEK_CUR:
>   		offset += file->f_pos;
> @@ -397,7 +409,11 @@ static loff_t offset_dir_llseek(struct file *file, loff_t offset, int whence)
>   	}
>   
>   	/* In this case, ->private_data is protected by f_pos_lock */
> -	file->private_data = NULL;
> +	if (!offset) {
> +		unsigned long next_offset = (unsigned long)ctx->next_offset;
> +
> +		file->private_data = (void *)next_offset;
> +	}
>   	return vfs_setpos(file, offset, U32_MAX);
>   }
>   
> @@ -427,7 +443,7 @@ static bool offset_dir_emit(struct dir_context *ctx, struct dentry *dentry)
>   			  inode->i_ino, fs_umode_to_dtype(inode->i_mode));
>   }
>   
> -static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
> +static void offset_iterate_dir(struct inode *inode, struct dir_context *ctx, long last_index)
>   {
>   	struct offset_ctx *so_ctx = inode->i_op->get_offset_ctx(inode);
>   	XA_STATE(xas, &so_ctx->xa, ctx->pos);
> @@ -436,17 +452,21 @@ static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
>   	while (true) {
>   		dentry = offset_find_next(&xas);
>   		if (!dentry)
> -			return ERR_PTR(-ENOENT);
> +			return;
> +
> +		if (dentry2offset(dentry) >= last_index) {
> +			dput(dentry);
> +			return;
> +		}
>   
>   		if (!offset_dir_emit(ctx, dentry)) {
>   			dput(dentry);
> -			break;
> +			return;
>   		}
>   
>   		dput(dentry);
>   		ctx->pos = xas.xa_index + 1;
>   	}
> -	return NULL;
>   }
>   
>   /**
> @@ -473,22 +493,19 @@ static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx)
>   static int offset_readdir(struct file *file, struct dir_context *ctx)
>   {
>   	struct dentry *dir = file->f_path.dentry;
> +	long last_index = (long)file->private_data;
>   
>   	lockdep_assert_held(&d_inode(dir)->i_rwsem);
>   
>   	if (!dir_emit_dots(file, ctx))
>   		return 0;
>   
> -	/* In this case, ->private_data is protected by f_pos_lock */
> -	if (ctx->pos == 2)
> -		file->private_data = NULL;
> -	else if (file->private_data == ERR_PTR(-ENOENT))
> -		return 0;
> -	file->private_data = offset_iterate_dir(d_inode(dir), ctx);
> +	offset_iterate_dir(d_inode(dir), ctx, last_index);
>   	return 0;
>   }
>   
>   const struct file_operations simple_offset_dir_operations = {
> +	.open		= offset_dir_open,
>   	.llseek		= offset_dir_llseek,
>   	.iterate_shared	= offset_readdir,
>   	.read		= generic_read_dir,

That commit has some other problems, so we're currently considering this
series instead:

https://lore.kernel.org/linux-fsdevel/20250124191946.22308-1-cel@kernel.org/


-- 
Chuck Lever

Re: [PATCH] libfs: fix infinite directory reads for offset dir

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

The upstream commit SHA1 provided is correct: 64a7ce76fb901bf9f9c36cf5d681328fc0fd4b5a

WARNING: Author mismatch between patch and upstream commit:
Backport author: ciprietti@google.com
Commit author: yangerkun<yangerkun@huawei.com>


Status in newer kernel trees:
6.13.y | Branch not found

Note: The patch differs from the upstream commit:
---
Failed to apply patch cleanly, falling back to interdiff...
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-6.13.y       |  Failed     |  N/A       |
| stable/linux-6.12.y       |  Failed     |  N/A       |
| stable/linux-6.6.y        |  Success    |  Success   |
| stable/linux-6.1.y        |  Failed     |  N/A       |
| stable/linux-5.15.y       |  Failed     |  N/A       |
| stable/linux-5.10.y       |  Failed     |  N/A       |
| stable/linux-5.4.y        |  Failed     |  N/A       |

Re: [PATCH] libfs: fix infinite directory reads for offset dir

[Greg KH]

On Tue, Jan 28, 2025 at 03:03:22PM +0000, ciprietti@google.com wrote:
> From: yangerkun <yangerkun@huawei.com>
> 
> [ Upstream commit 64a7ce76fb901bf9f9c36cf5d681328fc0fd4b5a ]
> 
> After we switch tmpfs dir operations from simple_dir_operations to
> simple_offset_dir_operations, every rename happened will fill new dentry
> to dest dir's maple tree(&SHMEM_I(inode)->dir_offsets->mt) with a free
> key starting with octx->newx_offset, and then set newx_offset equals to
> free key + 1. This will lead to infinite readdir combine with rename
> happened at the same time, which fail generic/736 in xfstests(detail show
> as below).
> 
> 1. create 5000 files(1 2 3...) under one dir
> 2. call readdir(man 3 readdir) once, and get one entry
> 3. rename(entry, "TEMPFILE"), then rename("TEMPFILE", entry)
> 4. loop 2~3, until readdir return nothing or we loop too many
>    times(tmpfs break test with the second condition)
> 
> We choose the same logic what commit 9b378f6ad48cf ("btrfs: fix infinite
> directory reads") to fix it, record the last_index when we open dir, and
> do not emit the entry which index >= last_index. The file->private_data
> now used in offset dir can use directly to do this, and we also update
> the last_index when we llseek the dir file.
> 
> Fixes: a2e459555c5f ("shmem: stable directory offsets")
> Signed-off-by: yangerkun <yangerkun@huawei.com>
> Link: https://lore.kernel.org/r/20240731043835.1828697-1-yangerkun@huawei.com
> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> [brauner: only update last_index after seek when offset is zero like Jan suggested]
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Andrea Ciprietti <ciprietti@google.com>

You forgot to mention what you changed.

> ---
>  fs/libfs.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index dc0f7519045f..916c39e758b1 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -371,6 +371,15 @@ void simple_offset_destroy(struct offset_ctx *octx)
>  	xa_destroy(&octx->xa);
>  }
>  
> +static int offset_dir_open(struct inode *inode, struct file *file)
> +{
> +	struct offset_ctx *ctx = inode->i_op->get_offset_ctx(inode);
> +	unsigned long next_offset = (unsigned long)ctx->next_offset;
> +
> +	file->private_data = (void *)next_offset;

Why do you need 2 casts here when the original did not?

thanks,

greg k-h

Re: [PATCH] libfs: fix infinite directory reads for offset dir

[Greg KH]

On Tue, Jan 28, 2025 at 03:03:22PM +0000, ciprietti@google.com wrote:
> From: yangerkun <yangerkun@huawei.com>
> 
> [ Upstream commit 64a7ce76fb901bf9f9c36cf5d681328fc0fd4b5a ]
> 
> After we switch tmpfs dir operations from simple_dir_operations to
> simple_offset_dir_operations, every rename happened will fill new dentry
> to dest dir's maple tree(&SHMEM_I(inode)->dir_offsets->mt) with a free
> key starting with octx->newx_offset, and then set newx_offset equals to
> free key + 1. This will lead to infinite readdir combine with rename
> happened at the same time, which fail generic/736 in xfstests(detail show
> as below).
> 
> 1. create 5000 files(1 2 3...) under one dir
> 2. call readdir(man 3 readdir) once, and get one entry
> 3. rename(entry, "TEMPFILE"), then rename("TEMPFILE", entry)
> 4. loop 2~3, until readdir return nothing or we loop too many
>    times(tmpfs break test with the second condition)
> 
> We choose the same logic what commit 9b378f6ad48cf ("btrfs: fix infinite
> directory reads") to fix it, record the last_index when we open dir, and
> do not emit the entry which index >= last_index. The file->private_data
> now used in offset dir can use directly to do this, and we also update
> the last_index when we llseek the dir file.
> 
> Fixes: a2e459555c5f ("shmem: stable directory offsets")
> Signed-off-by: yangerkun <yangerkun@huawei.com>
> Link: https://lore.kernel.org/r/20240731043835.1828697-1-yangerkun@huawei.com
> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> [brauner: only update last_index after seek when offset is zero like Jan suggested]
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> Signed-off-by: Andrea Ciprietti <ciprietti@google.com>
> ---
>  fs/libfs.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)

No hint as to what kernel tree(s) this is for?

Re: [PATCH] libfs: fix infinite directory reads for offset dir

[Chuck Lever]

On 1/28/25 12:12 PM, Greg KH wrote:
> On Tue, Jan 28, 2025 at 03:03:22PM +0000, ciprietti@google.com wrote:
>> From: yangerkun <yangerkun@huawei.com>
>>
>> [ Upstream commit 64a7ce76fb901bf9f9c36cf5d681328fc0fd4b5a ]
>>
>> After we switch tmpfs dir operations from simple_dir_operations to
>> simple_offset_dir_operations, every rename happened will fill new dentry
>> to dest dir's maple tree(&SHMEM_I(inode)->dir_offsets->mt) with a free
>> key starting with octx->newx_offset, and then set newx_offset equals to
>> free key + 1. This will lead to infinite readdir combine with rename
>> happened at the same time, which fail generic/736 in xfstests(detail show
>> as below).
>>
>> 1. create 5000 files(1 2 3...) under one dir
>> 2. call readdir(man 3 readdir) once, and get one entry
>> 3. rename(entry, "TEMPFILE"), then rename("TEMPFILE", entry)
>> 4. loop 2~3, until readdir return nothing or we loop too many
>>     times(tmpfs break test with the second condition)
>>
>> We choose the same logic what commit 9b378f6ad48cf ("btrfs: fix infinite
>> directory reads") to fix it, record the last_index when we open dir, and
>> do not emit the entry which index >= last_index. The file->private_data
>> now used in offset dir can use directly to do this, and we also update
>> the last_index when we llseek the dir file.
>>
>> Fixes: a2e459555c5f ("shmem: stable directory offsets")
>> Signed-off-by: yangerkun <yangerkun@huawei.com>
>> Link: https://lore.kernel.org/r/20240731043835.1828697-1-yangerkun@huawei.com
>> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
>> [brauner: only update last_index after seek when offset is zero like Jan suggested]
>> Signed-off-by: Christian Brauner <brauner@kernel.org>
>> Signed-off-by: Andrea Ciprietti <ciprietti@google.com>
>> ---
>>   fs/libfs.c | 39 ++++++++++++++++++++++++++++-----------
>>   1 file changed, 28 insertions(+), 11 deletions(-)
> 
> No hint as to what kernel tree(s) this is for?
> 

Just to be absolutely clear: NAK - there are some problems already
reported with this patch upstream.

We have a patch series pending that addresses this issue in v6.6:

https://lore.kernel.org/linux-fsdevel/20250124191946.22308-1-cel@kernel.org/

-- 
Chuck Lever

Re: [PATCH] libfs: fix infinite directory reads for offset dir

[Andrea Ciprietti]

Hi all, thanks for the replies! Understood the NACK, so let's drop this
backport.

Just not to let Greg's questoins go unanswered: this was meant for
linux-6.6.y (sorry forgot to mention it) and the additional cast is
required because the type of `ctx->next_offset` is a u32 in the 6.6
version, later changed to unsigned long which made it possible to cast it
to (void *) directly.

Best,
Andrea