linux-kernel - Re: [f2fs-dev] f2fs-tools: Check and fix inline xattr inplace

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]

Message-ID: <20241205034035.3119810-1-wangzijie1@honor.com>
Date: Thu, 5 Dec 2024 11:40:34 +0800
From: wangzijie <wangzijie1@...or.com>
To: <linux-f2fs-devel@...ts.sourceforge.net>
CC: <chao@...nel.org>, <jaegeuk@...nel.org>, <linux-kernel@...r.kernel.org>,
	<shengyong@...o.com>, <wangzijie1@...or.com>
Subject: Re: [f2fs-dev] f2fs-tools: Check and fix inline xattr inplace

>On 2024/12/4 20:23, wangzijie wrote:
>> When we check inode which just has inline xattr data, we copy
>> inline xattr data from inode, check it(maybe fix it) and copy
>> it again to inode. We can check and fix xattr inplace for this
>> kind of inode to reduce memcpy times.
>> 
>> Signed-off-by: wangzijie <wangzijie1@...or.com>
>> ---
>>   fsck/fsck.c | 18 ++++++++++++++----
>>   1 file changed, 14 insertions(+), 4 deletions(-)
>> 
>> diff --git a/fsck/fsck.c b/fsck/fsck.c
>> index aa3fb97..fd8b082 100644
>> --- a/fsck/fsck.c
>> +++ b/fsck/fsck.c
>> @@ -840,11 +840,18 @@ int chk_extended_attributes(struct f2fs_sb_info *sbi, u32 nid,
>>   	struct f2fs_xattr_entry *ent;
>>   	__u32 xattr_size = XATTR_SIZE(&inode->i);
>>   	bool need_fix = false;
>> +	bool has_xattr_node = false;
>> +	nid_t xnid = le32_to_cpu(inode->i.i_xattr_nid);
>>   
>>   	if (xattr_size == 0)
>>   		return 0;
>>   
>> -	xattr = read_all_xattrs(sbi, inode, false);
>> +	if (xattr_size <= inline_xattr_size(&inode->i) && !xnid)
>Hi, zijie,
>
>I propose to change the behavors of read_all_xattrs and write_all_xattrs, and to add a
>new free_xattrs.
>
>* read_all_xattrs checks whether xnid exist. If it's not, 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.
>
>In this way, all instances where {read|write}_all_xattrs are called can avoid unnecessary
>memory alloc and copy. free_xattrs(xattrs) should be used instead of free(xattrs).
>
>thanks,
>shengyong

Hi, yong
Thanks for sharing what you proposed to do. By the way, I noticed that 
when setattr, read_all_xattr will set xattr header's magic and refcount,
but it seems we don't check these values in header(kernel/fsck). Should we 
add check logic for it?

>> +		xattr = inline_xattr_addr(&inode->i);
>> +	else {
>> +		xattr = read_all_xattrs(sbi, inode, false);
>> +		has_xattr_node = true;
>> +	}
>>   	ASSERT(xattr);
>>   
>>   	last_base_addr = (void *)xattr + xattr_size;
>> @@ -867,12 +874,15 @@ int chk_extended_attributes(struct f2fs_sb_info *sbi, u32 nid,
>>   	}
>>   	if (need_fix && c.fix_on) {
>>   		memset(ent, 0, (u8 *)last_base_addr - (u8 *)ent);
>> -		write_all_xattrs(sbi, inode, xattr_size, xattr);
>> +		if (has_xattr_node) {
>> +			write_all_xattrs(sbi, inode, xattr_size, xattr);
>> +			free(xattr);
>> +		}
>>   		FIX_MSG("[0x%x] nullify wrong xattr entries", nid);
>> -		free(xattr);
>>   		return 1;
>>   	}
>> -	free(xattr);
>> +	if (has_xattr_node)
>> +		free(xattr);
>>   	return 0;
>>   }
>>   
>
>
>
>_______________________________________________
>Linux-f2fs-devel mailing list
>Linux-f2fs-devel@...ts.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel