[v1] exfat: fix exfat_find_empty_entry() not returning error on failure

Commit Message

[e-mail] Dec. 3, 2024, 5:32 a.m. UTC

On failure, "dentry" is the error code. If the error code indicates
that there is no space, a new cluster may need to be allocated; for
other errors, it should be returned directly.

Only on success, "dentry" is the index of the directory entry, and
it needs to be converted into the directory entry index within the
cluster where it is located.

Fixes: 8a3f5711ad74 ("exfat: reduce FAT chain traversal")
Reported-by: [e-mail]
Tested-by: [e-mail]
Signed-off-by: Yuezhang Mo <[e-mail]>
---
 fs/exfat/namei.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Namjae Jeon Dec. 4, 2024, 4:18 a.m. UTC | #1

On Tue, Dec 3, 2024 at 2:33 PM [e-mail]
<[e-mail]> wrote:
>
> On failure, "dentry" is the error code. If the error code indicates
> that there is no space, a new cluster may need to be allocated; for
> other errors, it should be returned directly.
>
> Only on success, "dentry" is the index of the directory entry, and
> it needs to be converted into the directory entry index within the
> cluster where it is located.
>
> Fixes: 8a3f5711ad74 ("exfat: reduce FAT chain traversal")
This issue caused by this patch ? If yes, Could you elaborate how this
patch make this issue ?
> Reported-by: [e-mail]
> Tested-by: [e-mail]
I can not reproduce it using C-reproducer. Have you reproduced it ?
Thanks.
> Signed-off-by: Yuezhang Mo <[e-mail]>
> ---
>  fs/exfat/namei.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
> index f203c53277e2..c24b62681535 100644
> --- a/fs/exfat/namei.c
> +++ b/fs/exfat/namei.c
> @@ -330,8 +330,8 @@ static int exfat_find_empty_entry(struct inode *inode,
>
>         while ((dentry = exfat_search_empty_slot(sb, &hint_femp, p_dir,
>                                         num_entries, es)) < 0) {
> -               if (dentry == -EIO)
> -                       break;
> +               if (dentry != -ENOSPC)
> +                       return dentry;
>
>                 if (exfat_check_max_dentries(inode))
>                         return -ENOSPC;
> --
> 2.43.0
>

[e-mail] Dec. 4, 2024, 8:06 a.m. UTC | #2

>> Fixes: 8a3f5711ad74 ("exfat: reduce FAT chain traversal")
> This issue caused by this patch ? If yes, Could you elaborate how this
patch make this issue ?

Yes.

This issue caused by the change in this patch.

-       return dentry;
+       p_dir->dir = exfat_sector_to_cluster(sbi, es->bh[0]->b_blocknr);
+       p_dir->size -= dentry / sbi->dentries_per_clu;
+
+       return dentry & (sbi->dentries_per_clu - 1);
 }

'dentry' is -EIO or -ENOMEM when reading directory entries fails.

"dentry & (sbi->dentries_per_clu - 1)" makes the return value a positive value,
so that exfat_add_entry() always thinks that the directory entry is read successfully.

> I can not reproduce it using C-reproducer. Have you reproduced it ?

This issue occurs when reading directory entries fails(this can be confirmed by
https://syzkaller.appspot.com/text?tag=Patch&x=1068bd30580000). 
Reproducing it requires a disk with bad blocks, I can not reproduce it too.

Namjae Jeon Dec. 5, 2024, 12:45 p.m. UTC | #3

On Tue, Dec 3, 2024 at 2:33 PM [e-mail]
<[e-mail]> wrote:
>
> On failure, "dentry" is the error code. If the error code indicates
> that there is no space, a new cluster may need to be allocated; for
> other errors, it should be returned directly.
>
> Only on success, "dentry" is the index of the directory entry, and
> it needs to be converted into the directory entry index within the
> cluster where it is located.
>
> Fixes: 8a3f5711ad74 ("exfat: reduce FAT chain traversal")
> Reported-by: [e-mail]
> Tested-by: [e-mail]
> Signed-off-by: Yuezhang Mo <[e-mail]>
Applied it to #dev.
Thanks for your patch!

Patch

From 29a1dc3d2f9b59fd4392ac0ae63c942384f179a7 Mon Sep 17 00:00:00 2001
From: Yuezhang Mo <[e-mail]>
Date: Mon, 2 Dec 2024 09:53:17 +0800
Subject: [PATCH v1] exfat: fix exfat_find_empty_entry() not returning error on
 failure

On failure, "dentry" is the error code. If the error code indicates
that there is no space, a new cluster may need to be allocated; for
other errors, it should be returned directly.

Only on success, "dentry" is the index of the directory entry, and
it needs to be converted into the directory entry index within the
cluster where it is located.

Fixes: 8a3f5711ad74 ("exfat: reduce FAT chain traversal")
Reported-by: [e-mail]
Tested-by: [e-mail]
Signed-off-by: Yuezhang Mo <[e-mail]>
---
 fs/exfat/namei.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index f203c53277e2..c24b62681535 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -330,8 +330,8 @@  static int exfat_find_empty_entry(struct inode *inode,
 
 	while ((dentry = exfat_search_empty_slot(sb, &hint_femp, p_dir,
 					num_entries, es)) < 0) {
-		if (dentry == -EIO)
-			break;
+		if (dentry != -ENOSPC)
+			return dentry;
 
 		if (exfat_check_max_dentries(inode))
 			return -ENOSPC;
-- 
2.43.0