| 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
| ||
|
Message-ID: <874174e3-79ba-452d-afff-615fe0bc8d0d@kernel.org>
Date: Tue, 25 Jun 2024 11:52:42 +0800
From: Chao Yu <chao@...nel.org>
To: wangzijie <wangzijie1@...or.com>
Cc: jaegeuk@...nel.org, linux-f2fs-devel@...ts.sourceforge.net,
linux-kernel@...r.kernel.org, zhiguo.niu@...soc.com, bintian.wang@...or.com
Subject: Re: [f2fs-dev] [PATCH v3] f2fs: reduce expensive checkpoint trigger
frequency
On 2024/6/25 10:14, wangzijie wrote:
>> We may trigger high frequent checkpoint for below case:
>> 1. mkdir /mnt/dir1; set dir1 encrypted
>> 2. touch /mnt/file1; fsync /mnt/file1
>> 3. mkdir /mnt/dir2; set dir2 encrypted
>> 4. touch /mnt/file2; fsync /mnt/file2
>> ...
>>
>> Although, newly created dir and file are not related, due to
>> commit bbf156f7afa7 ("f2fs: fix lost xattrs of directories"), we will
>> trigger checkpoint whenever fsync() comes after a new encrypted dir
>> created.
>>
>> In order to avoid such condition, let's record an entry including
>> directory's ino into global cache when we initialize encryption policy
>> in a checkpointed directory, and then only trigger checkpoint() when
>> target file's parent has non-persisted encryption policy, for the case
>> its parent is not checkpointed, need_do_checkpoint() has cover that
>> by verifying it with f2fs_is_checkpointed_node().
>>
>> Reported-by: Zhiguo Niu <zhiguo.niu@...soc.com>
>> Tested-by: Zhiguo Niu <zhiguo.niu@...soc.com>
>> Reported-by: Yunlei He <heyunlei@...onor.com>
>> Signed-off-by: Chao Yu <chao@...nel.org>
>> ---
>> v3:
>> - Recently, Zhiguo Niu reported the same issue, so I repost this
>> patch for comments.
>> fs/f2fs/f2fs.h | 2 ++
>> fs/f2fs/file.c | 3 +++
>> fs/f2fs/xattr.c | 16 ++++++++++++++--
>> include/trace/events/f2fs.h | 3 ++-
>> 4 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index e2e0ca45f881..0094a8c85f4a 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -279,6 +279,7 @@ enum {
>> APPEND_INO, /* for append ino list */
>> UPDATE_INO, /* for update ino list */
>> TRANS_DIR_INO, /* for transactions dir ino list */
>> + ENC_DIR_INO, /* for encrypted dir ino list */
>> FLUSH_INO, /* for multiple device flushing */
>> MAX_INO_ENTRY, /* max. list */
>> };
>> @@ -1147,6 +1148,7 @@ enum cp_reason_type {
>> CP_FASTBOOT_MODE,
>> CP_SPEC_LOG_NUM,
>> CP_RECOVER_DIR,
>> + CP_ENC_DIR,
>> };
>>
>> enum iostat_type {
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 8198afb5fb9c..18b33b1f0c83 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -218,6 +218,9 @@ static inline enum cp_reason_type need_do_checkpoint(struct inode *inode)
>> f2fs_exist_written_data(sbi, F2FS_I(inode)->i_pino,
>> TRANS_DIR_INO))
>> cp_reason = CP_RECOVER_DIR;
>> + else if (f2fs_exist_written_data(sbi, F2FS_I(inode)->i_pino,
>> + ENC_DIR_INO))
>> + cp_reason = CP_ENC_DIR;
>>
>> return cp_reason;
>> }
>> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
>> index f290fe9327c4..cbd1b88297fe 100644
>> --- a/fs/f2fs/xattr.c
>> +++ b/fs/f2fs/xattr.c
>> @@ -629,6 +629,7 @@ static int __f2fs_setxattr(struct inode *inode, int index,
>> const char *name, const void *value, size_t size,
>> struct page *ipage, int flags)
>> {
>> + struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>> struct f2fs_xattr_entry *here, *last;
>> void *base_addr, *last_base_addr;
>> int found, newsize;
>> @@ -772,8 +773,19 @@ static int __f2fs_setxattr(struct inode *inode, int index,
>> if (index == F2FS_XATTR_INDEX_ENCRYPTION &&
>> !strcmp(name, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT))
>> f2fs_set_encrypted_inode(inode);
>> - if (S_ISDIR(inode->i_mode))
>> - set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP);
>> +
>> + if (S_ISDIR(inode->i_mode)) {
>> + /*
>> + * In restrict mode, fsync() always tries triggering checkpoint
>> + * for all metadata consistency, in other mode, it only triggers
>> + * checkpoint when parent's encryption metadata updates.
>> + */
>> + if (F2FS_OPTION(sbi).fsync_mode == FSYNC_MODE_STRICT)
>> + set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP);
>> + else if (IS_ENCRYPTED(inode) &&
>> + f2fs_is_checkpointed_node(sbi, inode->i_ino))
>> + f2fs_add_ino_entry(sbi, inode->i_ino, ENC_DIR_INO);
>> + }
>>
>> same:
>> if (is_inode_flag_set(inode, FI_ACL_MODE)) {
>> diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
>> index 7ed0fc430dc6..48f2e399e184 100644
>> --- a/include/trace/events/f2fs.h
>> +++ b/include/trace/events/f2fs.h
>> @@ -139,7 +139,8 @@ TRACE_DEFINE_ENUM(EX_BLOCK_AGE);
>> { CP_NODE_NEED_CP, "node needs cp" }, \
>> { CP_FASTBOOT_MODE, "fastboot mode" }, \
>> { CP_SPEC_LOG_NUM, "log type is 2" }, \
>> - { CP_RECOVER_DIR, "dir needs recovery" })
>> + { CP_RECOVER_DIR, "dir needs recovery" }, \
>> + { CP_ENC_DIR, "persist encryption policy" })
>>
>> #define show_shutdown_mode(type) \
>> __print_symbolic(type, \
>> --
>> 2.40.1
>
> Hi, Chao
> I noticed the discussion about patch v2, Jaegeuk mentioned no encryption case:
> 1) parent is checkpointed
> 2) set_xattr(dir) w/ new xnid
> 3) create(file)
> 4) fsync(file)
> We will not trigger checkpoint() after this change.
> So, how about adding a rule in need_do_checkpoint? We can judge if the parent has xnid
> being checkpointed or not, if not we can still trigger checkpoint() and keep the same
> behavior before this change.
Hi wangzijie,
It may cause performance overhead to get parent's xnid by parent ino,
so I prefer to tag ENC_DIR_INO for the inode if its parent's xnid is
not checkpointed, it that fine to you?
Thanks,
>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@...ts.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
Powered by blists - more mailing lists