Address rename/readdir bugs in fs/libfs.c

From: yangerkun <[email protected]>

[ 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 <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Reviewed-by Chuck Lever <[email protected]>

[brauner: only update last_index after seek when offset is zero like Jan suggested]

Signed-off-by Christian Brauner <[email protected]>

Link https://nvd.nist.gov/vuln/detail/CVE-2024-46701

[ cel: adjusted to apply to origin/linux-6.6.y ]

Signed-off-by Chuck Lever <[email protected]>

---
  fs/libfs.c | 37 +++++++++++++++++++++++++------------
  1 file changed, 25 insertions(+), 12 deletions(-)

fs/libfs.c +0 -0

index a87005c89534..b59ff0dfea1f 100644

fs/libfs.c +18 -12

+
  /**
   * offset_dir_llseek - Advance the read position of a directory descriptor
   * @file: an open directory whose position is to be updated

@@ -462,6 +470,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;

@@ -475,8 +486,9 @@ 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;
-	return vfs_setpos(file, offset, U32_MAX);
+	if (!offset)
+		file->private_data = (void *)ctx->next_offset;
+	return vfs_setpos(file, offset, LONG_MAX);
  }
 
  static struct dentry *offset_find_next(struct xa_state *xas)

@@ -505,7 +517,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);

@@ -514,17 +526,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;
  }
 
  /**

@@ -551,22 +567,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 == DIR_OFFSET_MIN)
-		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,

Hi,

Quoted 9 lines · 在 2024/11/11 8:52, [email protected] 写道:

From: yangerkun <[email protected]>
[ 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).

Quoted 8 lines

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

Please notice this requires last_index should never overflow, otherwise readdir will be messed up.

Quoted 22 lines

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 <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Reviewed-by Chuck Lever <[email protected]>

[brauner: only update last_index after seek when offset is zero like Jan suggested]

Signed-off-by Christian Brauner <[email protected]>

Link https://nvd.nist.gov/vuln/detail/CVE-2024-46701

[ cel: adjusted to apply to origin/linux-6.6.y ]

Signed-off-by Chuck Lever <[email protected]>

---
 fs/libfs.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

fs/libfs.c +0 -0

index a87005c89534..b59ff0dfea1f 100644
@@ -449,6 +449,14 @@ void simple_offset_destroy(struct offset_ctx *octx)
  xa_destroy(&octx->xa);
 }
 +static int offset_dir_open(struct inode *inode, struct file *file)

fs/libfs.c void simple_offset_destroy(struct offset_ctx *octx) · line 458

Quoted 6 lines

fs/libfs.c +6 -0

+{
+ struct offset_ctx *ctx = inode->i_op->get_offset_ctx(inode);
+
+ file->private_data = (void *)ctx->next_offset;
+ return 0;
+}

Looks like xarray is still used.

I'm in the cc list ,so I assume you saw my set, then I don't know why you're ignoring my concerns.

1) next_offset is 32-bit and can overflow in a long-time running machine.
2) Once next_offset overflows, readdir will skip the files that offset is bigger.

Thanks,
Kuai

Quoted 68 lines

fs/libfs.c +16 -10

+
 /**
  * offset_dir_llseek - Advance the read position of a directory descriptor
  * @file: an open directory whose position is to be updated

@@ -462,6 +470,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;

@@ -475,8 +486,9 @@ 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;
- return vfs_setpos(file, offset, U32_MAX);
+ if (!offset)
+ file->private_data = (void *)ctx->next_offset;
+ return vfs_setpos(file, offset, LONG_MAX);
 }
   static struct dentry *offset_find_next(struct xa_state *xas)

@@ -505,7 +517,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);

@@ -514,17 +526,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;
 }
   /**

@@ -551,22 +567,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 == DIR_OFFSET_MIN)
- file->private_data = NULL;
- else if (file->private_data == ERR_PTR(-ENOENT))
- return 0;
- file->private_data = offset_iterate_dir(d_inode(dir), ctx);

fs/libfs.c static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx) · line 585

Quoted 8 lines

fs/libfs.c +2 -0

+ 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,

Quoted 25 lines · Yu Kuai

Hi,

Quoted 9 lines · 在 2024/11/11 8:52, [email protected] 写道:

From: yangerkun <[email protected]>
[ 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).

Quoted 8 lines

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

Quoted 2 lines

Please notice this requires last_index should never overflow, otherwise readdir will be messed up.

It would help your cause if you could be more specific than "messed up".

Quoted 22 lines

Quoted 22 lines

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 <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Reviewed-by Chuck Lever <[email protected]>

[brauner: only update last_index after seek when offset is zero like Jan suggested]

Signed-off-by Christian Brauner <[email protected]>

Link https://nvd.nist.gov/vuln/detail/CVE-2024-46701

[ cel: adjusted to apply to origin/linux-6.6.y ]

Signed-off-by Chuck Lever <[email protected]>

---
  fs/libfs.c | 37 +++++++++++++++++++++++++------------
  1 file changed, 25 insertions(+), 12 deletions(-)

fs/libfs.c +0 -0

index a87005c89534..b59ff0dfea1f 100644
@@ -449,6 +449,14 @@ void simple_offset_destroy(struct offset_ctx *octx)
   xa_destroy(&octx->xa);
  }
  +static int offset_dir_open(struct inode *inode, struct file *file)

Quoted 8 lines

fs/libfs.c void simple_offset_destroy(struct offset_ctx *octx) · line 458

Quoted 6 lines

fs/libfs.c +6 -0

+{
+ struct offset_ctx *ctx = inode->i_op->get_offset_ctx(inode);
+
+ file->private_data = (void *)ctx->next_offset;
+ return 0;
+}

Looks like xarray is still used.

That's not going to change, as several folks have already explained.

Quoted 7 lines

I'm in the cc list ,so I assume you saw my set, then I don't know why you're ignoring my concerns.

1) next_offset is 32-bit and can overflow in a long-time running machine.
2) Once next_offset overflows, readdir will skip the files that offset is bigger.

In that case, that entry won't be visible via getdents(3) until the directory is re-opened or the process does an lseek(fd, 0, SEEK_SET).

That is the proper and expected behavior. I suspect you will see exactly that behavior with ext4 and 32-bit directory offsets, for example.

Does that not directly address your concern? Or do you mean that Erkun's patch introduces a new issue?

If there is a problem here, please construct a reproducer against this patch set and post it.

Quoted 71 lines

Thanks,
Kuai

Quoted 68 lines

fs/libfs.c +16 -10

+
  /**
   * offset_dir_llseek - Advance the read position of a directory descriptor
   * @file: an open directory whose position is to be updated

@@ -462,6 +470,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;

@@ -475,8 +486,9 @@ 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;
- return vfs_setpos(file, offset, U32_MAX);
+ if (!offset)
+ file->private_data = (void *)ctx->next_offset;
+ return vfs_setpos(file, offset, LONG_MAX);
  }
    static struct dentry *offset_find_next(struct xa_state *xas)

@@ -505,7 +517,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);

@@ -514,17 +526,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;
  }
    /**

@@ -551,22 +567,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 == DIR_OFFSET_MIN)
- file->private_data = NULL;
- else if (file->private_data == ERR_PTR(-ENOENT))
- return 0;
- file->private_data = offset_iterate_dir(d_inode(dir), ctx);

Quoted 8 lines

fs/libfs.c static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx) · line 585

Quoted 8 lines

fs/libfs.c +2 -0

+ 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,

--
Chuck Lever

Quoted 57 lines · Chuck Lever III

Quoted 23 lines · Yu Kuai

Hi,

Quoted 9 lines · 在 2024/11/11 8:52, [email protected] 写道:

From: yangerkun <[email protected]>
[ 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).

Quoted 8 lines

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

Quoted 2 lines

Please notice this requires last_index should never overflow, otherwise readdir will be messed up.

It would help your cause if you could be more specific than "messed up".

Quoted 22 lines

Quoted 22 lines

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 <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Reviewed-by Chuck Lever <[email protected]>

[brauner: only update last_index after seek when offset is zero like Jan suggested]

Signed-off-by Christian Brauner <[email protected]>

Link https://nvd.nist.gov/vuln/detail/CVE-2024-46701

[ cel: adjusted to apply to origin/linux-6.6.y ]

Signed-off-by Chuck Lever <[email protected]>

---
 fs/libfs.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

fs/libfs.c +0 -0

index a87005c89534..b59ff0dfea1f 100644
@@ -449,6 +449,14 @@ void simple_offset_destroy(struct offset_ctx *octx)
  xa_destroy(&octx->xa);
 }
 +static int offset_dir_open(struct inode *inode, struct file *file)

Quoted 8 lines

fs/libfs.c void simple_offset_destroy(struct offset_ctx *octx) · line 458

Quoted 6 lines

fs/libfs.c +6 -0

+{
+ struct offset_ctx *ctx = inode->i_op->get_offset_ctx(inode);
+
+ file->private_data = (void *)ctx->next_offset;
+ return 0;
+}

Looks like xarray is still used.

Quoted 8 lines

That's not going to change, as several folks have already explained.

Quoted 6 lines

I'm in the cc list ,so I assume you saw my set, then I don't know why you're ignoring my concerns.
1) next_offset is 32-bit and can overflow in a long-time running machine.
2) Once next_offset overflows, readdir will skip the files that offset is bigger.

I'm sorry, I'm a little busy these days, so I haven't responded to this series of emails.

Quoted 3 lines

In that case, that entry won't be visible via getdents(3) until the directory is re-opened or the process does an lseek(fd, 0, SEEK_SET).

Yes.

Quoted 3 lines

That is the proper and expected behavior. I suspect you will see exactly that behavior with ext4 and 32-bit directory offsets, for example.

Emm...

For this case like this:

1. mkdir /tmp/dir and touch /tmp/dir/file1 /tmp/dir/file2
2. open /tmp/dir with fd1
3. readdir and get /tmp/dir/file1
4. rm /tmp/dir/file2
5. touch /tmp/dir/file2
4. loop 4~5 for 2^32 times
5. readdir /tmp/dir with fd1

For tmpfs now, we may see no /tmp/dir/file2, since the offset has been overflow, for ext4 it is ok... So we think this will be a problem.

Quoted 2 lines

Does that not directly address your concern? Or do you mean that Erkun's patch introduces a new issue?

Yes, to be honest, my personal feeling is a problem. But for 64bit, it may never been trigger.

Quoted 75 lines

If there is a problem here, please construct a reproducer against this patch set and post it.

Quoted 73 lines

Thanks,
Kuai

Quoted 70 lines

fs/libfs.c +17 -10

+
 /**
  * offset_dir_llseek - Advance the read position of a directory descriptor
  * @file: an open directory whose position is to be updated

@@ -462,6 +470,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;

@@ -475,8 +486,9 @@ 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;
- return vfs_setpos(file, offset, U32_MAX);
+ if (!offset)
+ file->private_data = (void *)ctx->next_offset;
+ return vfs_setpos(file, offset, LONG_MAX);
 }
   static struct dentry *offset_find_next(struct xa_state *xas)

@@ -505,7 +517,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);

@@ -514,17 +526,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;
 }
   /**

@@ -551,22 +567,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 == DIR_OFFSET_MIN)
- 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;

Quoted 8 lines

Quoted 6 lines

fs/libfs.c static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx) · line 585

Quoted 6 lines

fs/libfs.c +1 -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,

--
Chuck Lever

Quoted 103 lines · yangerkun

Quoted 57 lines · Chuck Lever III

Quoted 23 lines · Yu Kuai

Hi,

Quoted 9 lines · 在 2024/11/11 8:52, [email protected] 写道:

From: yangerkun <[email protected]>
[ 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).

Quoted 8 lines

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

Quoted 2 lines

Please notice this requires last_index should never overflow, otherwise readdir will be messed up.

It would help your cause if you could be more specific than "messed up".

Quoted 22 lines

Quoted 22 lines

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 <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Reviewed-by Chuck Lever <[email protected]>

[brauner: only update last_index after seek when offset is zero like Jan suggested]

Signed-off-by Christian Brauner <[email protected]>

Link https://nvd.nist.gov/vuln/detail/CVE-2024-46701

[ cel: adjusted to apply to origin/linux-6.6.y ]

Signed-off-by Chuck Lever <[email protected]>

---
  fs/libfs.c | 37 +++++++++++++++++++++++++------------
  1 file changed, 25 insertions(+), 12 deletions(-)

fs/libfs.c +0 -0

index a87005c89534..b59ff0dfea1f 100644
@@ -449,6 +449,14 @@ void simple_offset_destroy(struct offset_ctx *octx)
   xa_destroy(&octx->xa);
  }
  +static int offset_dir_open(struct inode *inode, struct file *file)

Quoted 8 lines

fs/libfs.c void simple_offset_destroy(struct offset_ctx *octx) · line 458

Quoted 6 lines

fs/libfs.c +6 -0

+{
+ struct offset_ctx *ctx = inode->i_op->get_offset_ctx(inode);
+
+ file->private_data = (void *)ctx->next_offset;
+ return 0;
+}

Looks like xarray is still used.

Quoted 8 lines

That's not going to change, as several folks have already explained.

Quoted 6 lines

I'm in the cc list ,so I assume you saw my set, then I don't know why you're ignoring my concerns.
1) next_offset is 32-bit and can overflow in a long-time running machine.
2) Once next_offset overflows, readdir will skip the files that offset is bigger.

I'm sorry, I'm a little busy these days, so I haven't responded to this series of emails.

Quoted 3 lines

In that case, that entry won't be visible via getdents(3) until the directory is re-opened or the process does an lseek(fd, 0, SEEK_SET).

Yes.

Quoted 3 lines

That is the proper and expected behavior. I suspect you will see exactly that behavior with ext4 and 32-bit directory offsets, for example.

Emm...

For this case like this:

1. mkdir /tmp/dir and touch /tmp/dir/file1 /tmp/dir/file2
2. open /tmp/dir with fd1
3. readdir and get /tmp/dir/file1
4. rm /tmp/dir/file2
5. touch /tmp/dir/file2
4. loop 4~5 for 2^32 times
5. readdir /tmp/dir with fd1

For tmpfs now, we may see no /tmp/dir/file2, since the offset has been overflow, for ext4 it is ok... So we think this will be a problem.

Quoted 2 lines

Does that not directly address your concern? Or do you mean that Erkun's patch introduces a new issue?

Quoted 1 line

Yes, to be honest, my personal feeling is a problem. But for 64bit, it may never been trigger.

Thanks for confirming.

In that case, the preferred way to handle it is to fix the issue in upstream, and then backport that fix to LTS. Dependence on 64-bit offsets to avoid a failure case should be considered a workaround, not a real fix, IMHO.

Do you have a few moments to address it, or if not I will see to it.

I think reducing the xa_limit in simple_offset_add() to, say, 2..16 would make the reproducer fire almost immediately.

Quoted 77 lines

Quoted 77 lines

If there is a problem here, please construct a reproducer against this patch set and post it.

Quoted 73 lines

Thanks,
Kuai

Quoted 70 lines

fs/libfs.c +17 -10

+
  /**
   * offset_dir_llseek - Advance the read position of a directory descriptor
   * @file: an open directory whose position is to be updated

@@ -462,6 +470,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;

@@ -475,8 +486,9 @@ 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;
- return vfs_setpos(file, offset, U32_MAX);
+ if (!offset)
+ file->private_data = (void *)ctx->next_offset;
+ return vfs_setpos(file, offset, LONG_MAX);
  }
    static struct dentry *offset_find_next(struct xa_state *xas)

@@ -505,7 +517,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);

@@ -514,17 +526,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;
  }
    /**

@@ -551,22 +567,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 == DIR_OFFSET_MIN)
- 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;

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

fs/libfs.c static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx) · line 585

Quoted 6 lines

fs/libfs.c +1 -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,

--
Chuck Lever

--
Chuck Lever

Quoted 101 lines · Chuck Lever III

Quoted 101 lines · yangerkun

Quoted 57 lines · Chuck Lever III

Quoted 23 lines · Yu Kuai

Hi,

Quoted 9 lines · 在 2024/11/11 8:52, [email protected] 写道:

From: yangerkun <[email protected]>
[ 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).

Quoted 8 lines

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

Quoted 2 lines

Please notice this requires last_index should never overflow, otherwise readdir will be messed up.

It would help your cause if you could be more specific than "messed up".

Quoted 22 lines

Quoted 22 lines

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 <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Reviewed-by Chuck Lever <[email protected]>

[brauner: only update last_index after seek when offset is zero like Jan suggested]

Signed-off-by Christian Brauner <[email protected]>

Link https://nvd.nist.gov/vuln/detail/CVE-2024-46701

[ cel: adjusted to apply to origin/linux-6.6.y ]

Signed-off-by Chuck Lever <[email protected]>

---
 fs/libfs.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

fs/libfs.c +0 -0

index a87005c89534..b59ff0dfea1f 100644
@@ -449,6 +449,14 @@ void simple_offset_destroy(struct offset_ctx *octx)
  xa_destroy(&octx->xa);
 }
 +static int offset_dir_open(struct inode *inode, struct file *file)

Quoted 8 lines

fs/libfs.c void simple_offset_destroy(struct offset_ctx *octx) · line 458

Quoted 6 lines

fs/libfs.c +6 -0

+{
+ struct offset_ctx *ctx = inode->i_op->get_offset_ctx(inode);
+
+ file->private_data = (void *)ctx->next_offset;
+ return 0;
+}

Looks like xarray is still used.

Quoted 8 lines

That's not going to change, as several folks have already explained.

Quoted 6 lines

I'm in the cc list ,so I assume you saw my set, then I don't know why you're ignoring my concerns.
1) next_offset is 32-bit and can overflow in a long-time running machine.
2) Once next_offset overflows, readdir will skip the files that offset is bigger.

I'm sorry, I'm a little busy these days, so I haven't responded to this series of emails.

Quoted 3 lines

In that case, that entry won't be visible via getdents(3) until the directory is re-opened or the process does an lseek(fd, 0, SEEK_SET).

Yes.

Quoted 3 lines

That is the proper and expected behavior. I suspect you will see exactly that behavior with ext4 and 32-bit directory offsets, for example.

Emm...

For this case like this:

1. mkdir /tmp/dir and touch /tmp/dir/file1 /tmp/dir/file2
2. open /tmp/dir with fd1
3. readdir and get /tmp/dir/file1
4. rm /tmp/dir/file2
5. touch /tmp/dir/file2
4. loop 4~5 for 2^32 times
5. readdir /tmp/dir with fd1

For tmpfs now, we may see no /tmp/dir/file2, since the offset has been overflow, for ext4 it is ok... So we think this will be a problem.

Quoted 2 lines

Does that not directly address your concern? Or do you mean that Erkun's patch introduces a new issue?

Quoted 6 lines

Quoted 1 line

Yes, to be honest, my personal feeling is a problem. But for 64bit, it may never been trigger.

Thanks for confirming.
In that case, the preferred way to handle it is to fix the issue in upstream, and then backport that fix to LTS. Dependence on 64-bit offsets to avoid a failure case should be considered a workaround, not a real fix, IMHO.

Yes.

Quoted 2 lines

Do you have a few moments to address it, or if not I will see to it.

You can try to do this, for the reason I am quite busy now until end of this month... Sorry.

Quoted 3 lines

I think reducing the xa_limit in simple_offset_add() to, say, 2..16 would make the reproducer fire almost immediately.

Yes.

Quoted 85 lines

Quoted 75 lines

Quoted 75 lines

If there is a problem here, please construct a reproducer against this patch set and post it.

Quoted 73 lines

Thanks,
Kuai

Quoted 70 lines

fs/libfs.c +17 -10

+
 /**
  * offset_dir_llseek - Advance the read position of a directory descriptor
  * @file: an open directory whose position is to be updated

@@ -462,6 +470,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;

@@ -475,8 +486,9 @@ 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;
- return vfs_setpos(file, offset, U32_MAX);
+ if (!offset)
+ file->private_data = (void *)ctx->next_offset;
+ return vfs_setpos(file, offset, LONG_MAX);
 }
   static struct dentry *offset_find_next(struct xa_state *xas)

@@ -505,7 +517,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);

@@ -514,17 +526,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;
 }
   /**

@@ -551,22 +567,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 == DIR_OFFSET_MIN)
- 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;

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

fs/libfs.c static void *offset_iterate_dir(struct inode *inode, struct dir_context *ctx) · line 585

Quoted 6 lines

fs/libfs.c +1 -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,

--
Chuck Lever

--
Chuck Lever

Quoted 9 lines · Chuck Lever III

Quoted 9 lines · Yu Kuai

I'm in the cc list ,so I assume you saw my set, then I don't know why you're ignoring my concerns.
1) next_offset is 32-bit and can overflow in a long-time running machine.
2) Once next_offset overflows, readdir will skip the files that offset is bigger.

I'm sorry, I'm a little busy these days, so I haven't responded to this series of emails.

Quoted 3 lines

In that case, that entry won't be visible via getdents(3) until the directory is re-opened or the process does an lseek(fd, 0, SEEK_SET).

Yes.

Quoted 4 lines

That is the proper and expected behavior. I suspect you will see exactly that behavior with ext4 and 32-bit directory offsets, for example.

Emm...

For this case like this:

1. mkdir /tmp/dir and touch /tmp/dir/file1 /tmp/dir/file2
2. open /tmp/dir with fd1
3. readdir and get /tmp/dir/file1
4. rm /tmp/dir/file2
5. touch /tmp/dir/file2
4. loop 4~5 for 2^32 times
5. readdir /tmp/dir with fd1

For tmpfs now, we may see no /tmp/dir/file2, since the offset has been overflow, for ext4 it is ok... So we think this will be a problem.

Quoted 2 lines

Does that not directly address your concern? Or do you mean that Erkun's patch introduces a new issue?

Yes, to be honest, my personal feeling is a problem. But for 64bit, it may never been trigger.

Quoted 2 lines

If there is a problem here, please construct a reproducer against this patch set and post it.

Quoted 39 lines · yangerkun

Quoted 9 lines · Chuck Lever III

Quoted 9 lines · Yu Kuai

I'm in the cc list ,so I assume you saw my set, then I don't know why you're ignoring my concerns.
1) next_offset is 32-bit and can overflow in a long-time running machine.
2) Once next_offset overflows, readdir will skip the files that offset is bigger.

I'm sorry, I'm a little busy these days, so I haven't responded to this series of emails.

Quoted 3 lines

In that case, that entry won't be visible via getdents(3) until the directory is re-opened or the process does an lseek(fd, 0, SEEK_SET).

Yes.

Quoted 4 lines

That is the proper and expected behavior. I suspect you will see exactly that behavior with ext4 and 32-bit directory offsets, for example.

Emm...

For this case like this:

1. mkdir /tmp/dir and touch /tmp/dir/file1 /tmp/dir/file2
2. open /tmp/dir with fd1
3. readdir and get /tmp/dir/file1
4. rm /tmp/dir/file2
5. touch /tmp/dir/file2
4. loop 4~5 for 2^32 times
5. readdir /tmp/dir with fd1

Quoted 2 lines

For tmpfs now, we may see no /tmp/dir/file2, since the offset has been overflow, for ext4 it is ok... So we think this will be a problem.

I constructed a simple test program using the above steps:

/*
* 1. mkdir /tmp/dir and touch /tmp/dir/file1 /tmp/dir/file2
* 2. open /tmp/dir with fd1
* 3. readdir and get /tmp/dir/file1
* 4. rm /tmp/dir/file2
* 5. touch /tmp/dir/file2
* 6. loop 4~5 for 2^32 times
* 7. readdir /tmp/dir with fd1
*/

#include <sys/types.h>
#include <sys/stat.h>

#include <dirent.h>
#include <errno.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdbool.h>
#include <stdio.h>
#include <string.h>

static void list_directory(DIR *dirp)

{
	struct dirent *de;

	errno = 0;
	do {
		de = readdir(dirp);
		if (!de)
			break;

		printf("d_off:  %lld\n", de->d_off);
		printf("d_name: %s\n", de->d_name);
	} while (true);

	if (errno)
		perror("readdir");
	else
		printf("EOD\n");
}

int main(int argc, char **argv)

{
	unsigned long i;
	DIR *dirp;
	int ret;

	/* 1. */
	ret = mkdir("/tmp/dir", 0755);
	if (ret < 0) {
		perror("mkdir");
		return 1;
	}

	ret = creat("/tmp/dir/file1", 0644);
	if (ret < 0) {
		perror("creat");
		return 1;
	}
	close(ret);

	ret = creat("/tmp/dir/file2", 0644);
	if (ret < 0) {
		perror("creat");
		return 1;
	}
	close(ret);

	/* 2. */
	errno = 0;
	dirp = opendir("/tmp/dir");
	if (!dirp) {
		if (errno)
			perror("opendir");
		else
			fprintf(stderr, "EOD\n");
		closedir(dirp);
		return 1;
	}

	/* 3. */
	errno = 0;
	do {
		struct dirent *de;

		de = readdir(dirp);
		if (!de) {
			if (errno) {
				perror("readdir");
				closedir(dirp);
				return 1;
			}
			break;
		}
		if (strcmp(de->d_name, "file1") == 0) {
			printf("Found 'file1'\n");
			break;
		}
	} while (true);

	/* run the test. */
	for (i = 0; i < 10000; i++) {
		/* 4. */
		ret = unlink("/tmp/dir/file2");
		if (ret < 0) {
			perror("unlink");
			closedir(dirp);
			return 1;
		}

		/* 5. */
		ret = creat("/tmp/dir/file2", 0644);
		if (ret < 0) {
			perror("creat");
			fprintf(stderr, "i = %lu\n", i);
			closedir(dirp);
			return 1;
		}
		close(ret);
	}

	/* 7. */
	printf("\ndirectory after test:\n");
	list_directory(dirp);

	/* cel. */
	rewinddir(dirp);
	printf("\ndirectory after rewind:\n");
	list_directory(dirp);

	closedir(dirp);
	return 0;
}

Quoted 5 lines

Quoted 2 lines

Does that not directly address your concern? Or do you mean that Erkun's patch introduces a new issue?

Yes, to be honest, my personal feeling is a problem. But for 64bit, it may never been trigger.

I ran the test program above on this kernel:

https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/log/?h=nfsd-testing

Note that it has a patch to restrict the range of directory offset values for tmpfs to 2..4096.

I did not observe any unexpected behavior after the offset values wrapped. At step 7, I can always see file2, and its offset is always
4. At step "cel" I can see all expected directory entries.

I tested on v6.12-rc7 with the same range restriction but using Maple tree and 64-bit offsets. No unexpected behavior there either.

So either we're still missing something, or there is no problem. My only theory is maybe it's an issue with an implicit integer sign conversion, and we should restrict the offset range to 2..S32_MAX.

I can try testing with a range of (U32_MAX - 4096)..(U32_MAX).

Quoted 4 lines

Quoted 4 lines

If there is a problem here, please construct a reproducer against this patch set and post it.

Invitation still stands: if you have a solid reproducer, please post it.