| 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: <2c781ecb-b611-4911-8918-e7a455f3f53b@oppo.com>
Date: Tue, 31 Dec 2024 14:52:03 +0800
From: Sheng Yong <shengyong@...o.com>
To: wangzijie <wangzijie1@...or.com>, jaegeuk@...nel.org, chao@...nel.org
CC: <linux-f2fs-devel@...ts.sourceforge.net>, <linux-kernel@...r.kernel.org>,
<bintian.wang@...or.com>, "mike@...o.com" <mike@...o.com>
Subject: Re: [PATCH v3] f2fs-tools: reduce memory alloc and copy for xattr
On 2024/12/30 21:23, wangzijie wrote:
> Currently when we check xattr of inode which dosen't have xnid, we make unnecessary
> memory alloc and copy. Followed by ShengYong's suggestion[1], change the behaviors of
> read_all_xattrs and write_all_xattrs, and add a new function free_xattrs.
>
> * read_all_xattrs: If xnid dosen't exist and there's no possibility to change xattr(which
> may enlarge xattr's size and leads to alloc new xnid) in next steps, return inline_xattr
> directly without alloc and memcpy.
> * write_all_xattrs checks whether inline_xattr and new txattr_addr have the same address
> to avoid copying back.
> * free_xattrs checks whether inline_xattr and new txattr_addr have the same address to
> free xattr buffer properly.
>
> After that, instances(except setxattr) where {read|write}_all_xattrs are called can avoid unnecessary
> memory alloc and copy.
>
> Use free_xattrs(xattrs) instead of free(xattrs) to free buffer properly.
>
> [1] https://lore.kernel.org/linux-f2fs-devel/502ae396-ae82-44d6-b08d-617e9e9c4092@oppo.com/
>
> Signed-off-by: wangzijie <wangzijie1@...or.com>
It looks good to me.
Reviewed-by: Sheng Yong <shengyong@...o.com>
thanks,
shengyong
> ---
> v2 -> v3:
> - fix read_all_xattrs logic to check and set xattr header
> - change parameter name for read_all_xattrs
> - change subject
> v1 -> v2:
> - Suggestions from ShengYong to change {read|write}_all_xattrs and add free_xattrs
> - If we may need change xattr, still alloc xattr buffer in read_all_xattrs
> ---
> ---
> fsck/dump.c | 4 ++--
> fsck/fsck.c | 6 +++---
> fsck/fsck.h | 3 ++-
> fsck/mount.c | 4 ++--
> fsck/xattr.c | 25 +++++++++++++++++++++----
> 5 files changed, 30 insertions(+), 12 deletions(-)
>
> diff --git a/fsck/dump.c b/fsck/dump.c
> index dc3c199..cc89909 100644
> --- a/fsck/dump.c
> +++ b/fsck/dump.c
> @@ -399,7 +399,7 @@ static void dump_xattr(struct f2fs_sb_info *sbi, struct f2fs_node *node_blk, int
> char xattr_name[F2FS_NAME_LEN] = {0};
> int ret;
>
> - xattr = read_all_xattrs(sbi, node_blk, true);
> + xattr = read_all_xattrs(sbi, node_blk, true, false);
> if (!xattr)
> return;
>
> @@ -478,7 +478,7 @@ static void dump_xattr(struct f2fs_sb_info *sbi, struct f2fs_node *node_blk, int
> free(name);
> }
>
> - free(xattr);
> + free_xattrs(node_blk, xattr);
> }
> #else
> static void dump_xattr(struct f2fs_sb_info *UNUSED(sbi),
> diff --git a/fsck/fsck.c b/fsck/fsck.c
> index aa3fb97..982defb 100644
> --- a/fsck/fsck.c
> +++ b/fsck/fsck.c
> @@ -844,7 +844,7 @@ int chk_extended_attributes(struct f2fs_sb_info *sbi, u32 nid,
> if (xattr_size == 0)
> return 0;
>
> - xattr = read_all_xattrs(sbi, inode, false);
> + xattr = read_all_xattrs(sbi, inode, false, false);
> ASSERT(xattr);
>
> last_base_addr = (void *)xattr + xattr_size;
> @@ -869,10 +869,10 @@ int chk_extended_attributes(struct f2fs_sb_info *sbi, u32 nid,
> memset(ent, 0, (u8 *)last_base_addr - (u8 *)ent);
> write_all_xattrs(sbi, inode, xattr_size, xattr);
> FIX_MSG("[0x%x] nullify wrong xattr entries", nid);
> - free(xattr);
> + free_xattrs(inode, xattr);
> return 1;
> }
> - free(xattr);
> + free_xattrs(inode, xattr);
> return 0;
> }
>
> diff --git a/fsck/fsck.h b/fsck/fsck.h
> index b581d3e..2897a5e 100644
> --- a/fsck/fsck.h
> +++ b/fsck/fsck.h
> @@ -341,9 +341,10 @@ struct hardlink_cache_entry *f2fs_search_hardlink(struct f2fs_sb_info *sbi,
> struct dentry *de);
>
> /* xattr.c */
> -void *read_all_xattrs(struct f2fs_sb_info *, struct f2fs_node *, bool);
> +void *read_all_xattrs(struct f2fs_sb_info *, struct f2fs_node *, bool, bool);
> void write_all_xattrs(struct f2fs_sb_info *sbi,
> struct f2fs_node *inode, __u32 hsize, void *txattr_addr);
> +void free_xattrs(struct f2fs_node *inode, void *txattr_addr);
>
> /* dir.c */
> int convert_inline_dentry(struct f2fs_sb_info *sbi, struct f2fs_node *node,
> diff --git a/fsck/mount.c b/fsck/mount.c
> index a189ba7..f6085e9 100644
> --- a/fsck/mount.c
> +++ b/fsck/mount.c
> @@ -370,7 +370,7 @@ void print_inode_info(struct f2fs_sb_info *sbi,
> DISP_u32(F2FS_INODE_NIDS(inode), i_nid[3]); /* indirect */
> DISP_u32(F2FS_INODE_NIDS(inode), i_nid[4]); /* double indirect */
>
> - xattr_addr = read_all_xattrs(sbi, node, true);
> + xattr_addr = read_all_xattrs(sbi, node, true, false);
> if (!xattr_addr)
> goto out;
>
> @@ -384,7 +384,7 @@ void print_inode_info(struct f2fs_sb_info *sbi,
> }
> print_xattr_entry(ent);
> }
> - free(xattr_addr);
> + free_xattrs(node, xattr_addr);
>
> out:
> printf("\n");
> diff --git a/fsck/xattr.c b/fsck/xattr.c
> index 6373c06..413cf73 100644
> --- a/fsck/xattr.c
> +++ b/fsck/xattr.c
> @@ -18,7 +18,7 @@
> #include "xattr.h"
>
> void *read_all_xattrs(struct f2fs_sb_info *sbi, struct f2fs_node *inode,
> - bool sanity_check)
> + bool sanity_check, bool for_change)
> {
> struct f2fs_xattr_header *header;
> void *txattr_addr;
> @@ -30,6 +30,11 @@ void *read_all_xattrs(struct f2fs_sb_info *sbi, struct f2fs_node *inode,
> return NULL;
> }
>
> + if (!xnid && !for_change) {
> + txattr_addr = inline_xattr_addr(&inode->i);
> + goto check_header;
> + }
> +
> txattr_addr = calloc(inline_size + F2FS_BLKSIZE, 1);
> ASSERT(txattr_addr);
>
> @@ -49,6 +54,7 @@ void *read_all_xattrs(struct f2fs_sb_info *sbi, struct f2fs_node *inode,
> sizeof(struct node_footer));
> }
>
> +check_header:
> header = XATTR_HDR(txattr_addr);
>
> /* Never been allocated xattrs */
> @@ -97,7 +103,8 @@ void write_all_xattrs(struct f2fs_sb_info *sbi,
> bool xattrblk_alloced = false;
> struct seg_entry *se;
>
> - memcpy(inline_xattr_addr(&inode->i), txattr_addr, inline_size);
> + if (inline_xattr_addr(&inode->i) != txattr_addr)
> + memcpy(inline_xattr_addr(&inode->i), txattr_addr, inline_size);
>
> if (hsize <= inline_size)
> return;
> @@ -137,6 +144,16 @@ free_xattr_node:
> ASSERT(ret >= 0);
> }
>
> +/*
> + * Different addresses between inline_xattr and txattr_addr means
> + * we newly allocate xattr buffer in read_all_xattrs, free it
> + */
> +void free_xattrs(struct f2fs_node *inode, void *txattr_addr)
> +{
> + if (inline_xattr_addr(&inode->i) != txattr_addr)
> + free(txattr_addr);
> +}
> +
> int f2fs_setxattr(struct f2fs_sb_info *sbi, nid_t ino, int index, const char *name,
> const void *value, size_t size, int flags)
> {
> @@ -174,7 +191,7 @@ int f2fs_setxattr(struct f2fs_sb_info *sbi, nid_t ino, int index, const char *na
> ret = dev_read_block(inode, ni.blk_addr);
> ASSERT(ret >= 0);
>
> - base_addr = read_all_xattrs(sbi, inode, true);
> + base_addr = read_all_xattrs(sbi, inode, true, true);
> ASSERT(base_addr);
>
> last_base_addr = (void *)base_addr + XATTR_SIZE(&inode->i);
> @@ -257,8 +274,8 @@ int f2fs_setxattr(struct f2fs_sb_info *sbi, nid_t ino, int index, const char *na
> /* inode need update */
> ASSERT(update_inode(sbi, inode, &ni.blk_addr) >= 0);
> exit:
> + free_xattrs(inode, base_addr);
> free(inode);
> - free(base_addr);
> return error;
> }
>
Powered by blists - more mailing lists