linux-kernel - Re: [PATCH V1] f2fs: fix potentail deadloop issue in do_recover

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]

Date: Wed, 24 Jan 2024 22:54:33 +0800
From: Chao Yu <chao@...nel.org>
To: Zhiguo Niu <niuzhiguo84@...il.com>
Cc: Zhiguo Niu <zhiguo.niu@...soc.com>, jaegeuk@...nel.org,
 linux-f2fs-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
 ke.wang@...soc.com
Subject: Re: [PATCH V1] f2fs: fix potentail deadloop issue in do_recover_data

Zhiguo,

Can you please check below version? Is it fine to you?

https://lore.kernel.org/linux-f2fs-devel/20240124144915.19445-1-chao@kernel.org

On 2024/1/22 13:46, Zhiguo Niu wrote:
> Hi Chao
> 
> On Mon, Jan 22, 2024 at 11:46 AM Chao Yu <chao@...nel.org> wrote:
>>
>> On 2023/12/25 19:11, Zhiguo Niu wrote:
>>> There is a potentail deadloop issue in the corner case of
>>> CONFIG_F2FS_FAULT_INJECTION is enabled and the return value
>>> of f2fs_reserve_new_block is error but not -ENOSPC, such as
>>> this error case:
>>> if (unlikely(is_inode_flag_set(dn->inode, FI_NO_ALLOC)))
>>>                return -EPERM;
>>
>> I don't see any path to trigger this error? am I missing something?
>>
>>> besides, the mainly error -ENOSPC has been handled as bug on,
>>> so other error cases can be proecssed normally without looping.
>>
>> commit 975756c41332bc5e523e9f843271ed5ab6aaaaaa
>> Author: Jaegeuk Kim <jaegeuk@...nel.org>
>> Date:   Thu May 19 11:57:21 2016 -0700
>>
>>       f2fs: avoid ENOSPC fault in the recovery process
>>
>>       This patch avoids impossible error injection, ENOSPC, during recovery process.
>>
>> Please check above patch, I guess intention of adding such loop is
>> to avoid mount failure due to fault injection was triggered in
>> f2fs_reserve_new_block().
>>
>> What about change as blew?
>> - keep the loop to avoid mount failure.
>> - remove bug_on() to avoid panic due to fault injection error.
>>
>> #define DEFAULT_RETRY_COUNT             8
>>
>>                  for (loops = DEFAULT_RETRY_COUNT; loops > 0; loops--) {
>>                          err = f2fs_reserve_new_block(&dn);
>>                          if (!err ||
>>                                  !IS_ENABLED(CONFIG_F2FS_FAULT_INJECTION))
>>                                  break;
>>                  }
> 
> Thanks for your detailed explanation and I understand.
> It seems that the original process is also reasonable,
> so it’s okay to keep it as it is.
>>
>> Thanks,
>>
>>>
>>> Fixes: 956fa1ddc132 ("f2fs: fix to check return value of f2fs_reserve_new_block()")
>>> Signed-off-by: Zhiguo Niu <zhiguo.niu@...soc.com>
>>> ---
>>>    fs/f2fs/recovery.c | 26 ++++++++------------------
>>>    1 file changed, 8 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
>>> index 21381b7..5d658f6 100644
>>> --- a/fs/f2fs/recovery.c
>>> +++ b/fs/f2fs/recovery.c
>>> @@ -710,15 +710,10 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>>>                 */
>>>                if (dest == NEW_ADDR) {
>>>                        f2fs_truncate_data_blocks_range(&dn, 1);
>>> -                     do {
>>> -                             err = f2fs_reserve_new_block(&dn);
>>> -                             if (err == -ENOSPC) {
>>> -                                     f2fs_bug_on(sbi, 1);
>>> -                                     break;
>>> -                             }
>>> -                     } while (err &&
>>> -                             IS_ENABLED(CONFIG_F2FS_FAULT_INJECTION));
>>> -                     if (err)
>>> +                     err = f2fs_reserve_new_block(&dn);
>>> +                     if (err == -ENOSPC)
>>> +                             f2fs_bug_on(sbi, 1);
>>> +                     else if (err)
>>>                                goto err;
>>>                        continue;
>>>                }
>>> @@ -727,15 +722,10 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>>>                if (f2fs_is_valid_blkaddr(sbi, dest, META_POR)) {
>>>
>>>                        if (src == NULL_ADDR) {
>>> -                             do {
>>> -                                     err = f2fs_reserve_new_block(&dn);
>>> -                                     if (err == -ENOSPC) {
>>> -                                             f2fs_bug_on(sbi, 1);
>>> -                                             break;
>>> -                                     }
>>> -                             } while (err &&
>>> -                                     IS_ENABLED(CONFIG_F2FS_FAULT_INJECTION));
>>> -                             if (err)
>>> +                             err = f2fs_reserve_new_block(&dn);
>>> +                             if (err == -ENOSPC)
>>> +                                     f2fs_bug_on(sbi, 1);
>>> +                             else if (err)
>>>                                        goto err;
>>>                        }
>>>    retry_prev: