linux-kernel - Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum

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: <b65ee79a-8d3d-8ade-133b-913b2cbd0264@huawei.com>
Date:   Wed, 31 Jan 2018 15:14:56 +0800
From:   Chao Yu <yuchao0@...wei.com>
To:     Jaegeuk Kim <jaegeuk@...nel.org>
CC:     <linux-f2fs-devel@...ts.sourceforge.net>,
        <linux-kernel@...r.kernel.org>, <chao@...nel.org>
Subject: Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum

On 2018/1/31 10:02, Jaegeuk Kim wrote:
> What if we want to add more entries in addition to node_checksum? Do we have
> to add a new feature flag at every time? How about adding a layout value instead

Hmm.. for previous implementation, IMO, we'd better add a new feature flag at
every time, otherwise, w/ extra_nsize only, in current image, we can know a
valid range of extended area in node block, but we don't know which
fields/features are valid/enabled or not.

One more thing is that if we can add one feature flag for each field, we got one
more chance to disable it dynamically.

> of extra_nsize? For example, layout #1 means node_checksum with extra_nsize=X?
> 
> 
> What does 1017 mean? We need to make this structure more flexibly for new

Yes, using raw 1017 is not appropriate here.

> entries. Like this?
> 		union {
> 			struct node_v1;
> 			struct node_v2;
> 			struct node_v3;
> 			...
> 			struct direct_node dn;
> 			struct indirect_node in;
> 		};
> 	};
> 
> 	struct node_v1 {
> 		__le32 data[DEF_ADDRS_PER_BLOCK - V1_NSIZE=1];
> 		__le32 node_checksum;
> 	}
> 
> 	struct node_v2 {
> 		__le32 data[DEF_ADDRS_PER_BLOCK - V2_NSIZE=500];

Hmm.. If we only need to add one more 4 bytes field in struct node_v2, but
V2_NSIZE is defined as fixed 500, there must be 492 bytes wasted.

Or we can define V2_NSIZE as 8, but if there comes more and more extended
fields, node version count can be a large number, it results in complicated
version recognization and handling.

One more question is how can we control which fields are valid or not in
comp[Vx_NSIZE]?

Anyway, what I'm thinking is maybe we can restructure layout of node block like
the one used by f2fs_inode:

struct f2fs_node {
	union {
		struct f2fs_inode i;
		union {
			struct {
				__le32 node_checksum;
				__le32 feature_field_1;
				__le32 feature_field_2;
				....
				__le32 addr[];

			};
			struct direct_node dn;
			struct indirect_node in;
		};
	};
	struct node_footer footer;
} __packed;

Moving all extended fields to the head of f2fs_node, so we don't have to use
macro to indicate actual size of addr.

Thanks,

> 		__le32 comp[V2_NSIZE];
> 	}
> 	...
> 
>> +			};
>> +			struct direct_node dn;
>> +			struct indirect_node in;
>> +		};
>>  	};
>>  	struct node_footer footer;
>>  } __packed;
>> -- 
>> 2.15.0.55.gc2ece9dc4de6
> 
> .
>