Am 2016-08-14 03:50, schrieb Stefan Bruens:
On Freitag, 12. August 2016 15:16:20 CEST Michael Walle wrote:
All fields were accessed directly instead of using the proper byte swap functions. Thus, ext4 write support was only usable on little-endian architectures. Fix this.
Signed-off-by: Michael Walle michael@walle.cc
I have tested this on sandbox (x86_64), no regressions found. Some remarks below.
Reviewed-by: Stefan Brüns stefan.bruens@rwth-aachen.de Tested-by: Stefan Brüns stefan.bruens@rwth-aachen.de
Ok this patch is huge, please comment. I know, checkpatch fails because there are longer lines than 80 characters. I don't want do be rude, but the coding style is awful :/ Eg.
http://git.denx.de/?p=u-boot.git;a=blob;f=fs/ext4/ext4_common.c;h=eb49fce04 c5a290e839575945da722bc97d2f670;hb=HEAD#l440 http://git.denx.de/?p=u-boot.git;a=blob;f=fs/ext4/ext4_write.c;h=e027916763 f9b52937c7fe13f1698b67b94eb901;hb=HEAD#l137
Many ugly places seems to come from the 80 characters limit. Most of the code looks like its right-aligned. I know there is a valid point for having this limit, eg refactor code into small functions. And frankly, the ext4 write code badly needs such a refactoring. But I won't and can't do it. I can resend a new version which meet the 80 character restriction, but it won't make the code easier to read. And believe me, it is already hard to do it.
Ok, back to business. Sparse really helped to find the places where the byteswaps were missing once you annotated it correctly with the __le16/__le32 types. If someone want to do check again, just run make C=1 See Documentation/sparse.txt in the linux kernel for more information. I've done a short test, to verify I can (over)write a 700kB file. No cornertests etc.
fs/ext4/ext4_common.c | 260 ++++++++++++++++++++++++++----------------------- fs/ext4/ext4_common.h | 4 +- fs/ext4/ext4_journal.c | 15 +-- fs/ext4/ext4_journal.h | 4 +- fs/ext4/ext4_write.c | 195 +++++++++++++++++++------------------ include/ext_common.h | 2 +- 6 files changed, 251 insertions(+), 229 deletions(-)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index e8ed30a..4eb4e18 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -33,19 +33,22 @@
struct ext2_data *ext4fs_root; struct ext2fs_node *ext4fs_file; -uint32_t *ext4fs_indir1_block; +__le32 *ext4fs_indir1_block; int ext4fs_indir1_size; int ext4fs_indir1_blkno = -1; -uint32_t *ext4fs_indir2_block; +__le32 *ext4fs_indir2_block; int ext4fs_indir2_size; int ext4fs_indir2_blkno = -1;
-uint32_t *ext4fs_indir3_block; +__le32 *ext4fs_indir3_block; int ext4fs_indir3_size; int ext4fs_indir3_blkno = -1; struct ext2_inode *g_parent_inode; static int symlinknest;
+#define DEC_LE16(x) (x = cpu_to_le16(le16_to_cpu(x) - 1)) +#define DEC_LE32(x) (x = cpu_to_le32(le32_to_cpu(x) - 1))
I would prefer a static function *per variable/field* - this will make it easier to manipulate any high/low split counts for FEATURE_64BIT. Same for the INC_xxx in ext4_write.c
ok, sounds right ;)
@@ -360,6 +364,9 @@ void ext4fs_update_parent_dentry(char *filename, int *p_ino, int file_type) /* directory entry */ struct ext2_dirent *dir; char *temp_dir = NULL;
uint32_t new_blk_no;
uint32_t new_size;
uint32_t new_blockcnt;
zero_buffer = zalloc(fs->blksz); if (!zero_buffer) {
@@ -396,7 +403,7 @@ restart: goto fail; dir = (struct ext2_dirent *)root_first_block_buffer; totalbytes = 0;
- while (dir->direntlen > 0) {
- while (le16_to_cpu(dir->direntlen) > 0) {
direntlen is at least 8, so this is "while (true) {"
I'll put this and all remarks below, into a separate patch, because I want this patch to only fix the endianess problem without any functional changes.
-michael
/* * blocksize-totalbytes because last directory length * i.e. dir->direntlen is free availble space in the@@ -405,7 +412,7 @@ restart: */
/* traversing the each directory entry */
if (fs->blksz - totalbytes == dir->direntlen) {
if (fs->blksz - totalbytes == le16_to_cpu(dir->direntlen)) { if (strlen(filename) % 4 != 0) padding_factor = 4 - (strlen(filename) % 4);@@ -430,32 +437,34 @@ restart: printf("Directory exceeds limit\n"); goto fail; }
g_parent_inode->b.blocks.dir_blocks[direct_blk_idx] = ext4fs_get_new_blk_no();if (g_parent_inode->b.blocks.dir_blocks[direct_blk_idx] == -1) {
new_blk_no = ext4fs_get_new_blk_no();if (new_blk_no == -1) { printf("no block left to assign\n"); goto fail; }
put_ext4(((uint64_t)((uint64_t)g_parent_inode->b.blocks.dir_blocks[direct_blk_idx] *(uint64_t)fs->blksz)), zero_buffer, fs->blksz);g_parent_inode->size =g_parent_inode->size + fs->blksz;g_parent_inode->blockcnt =g_parent_inode->blockcnt + fs->sect_perblk;
put_ext4((uint64_t)new_blk_no * fs->blksz, zero_buffer, fs-blksz);
g_parent_inode->b.blocks.dir_blocks[direct_blk_idx] =cpu_to_le32(new_blk_no);This is (already was) broken if the directory uses extents, we have to fix this (in a seperate patch).
new_size = le32_to_cpu(g_parent_inode->size);new_size += fs->blksz;g_parent_inode->size = cpu_to_le32(new_size);new_blockcnt = le32_to_cpu(g_parent_inode->blockcnt);new_blockcnt += fs->sect_perblk;g_parent_inode->blockcnt = cpu_to_le32(new_blockcnt);if (ext4fs_put_metadata (root_first_block_buffer, first_block_no_of_root)) goto fail; goto restart; }
dir->direntlen = last_entry_dirlen;
dir->direntlen = cpu_to_le16(last_entry_dirlen); break;
templength = dir->direntlen;
templength = le16_to_cpu(dir->direntlen);@@ -534,7 +543,7 @@ static int search_dir(struct ext2_inode *parent_inode, char *dirname) dir = (struct ext2_dirent *)block_buffer; ptr = (char *)dir; totalbytes = 0;
while (dir->direntlen >= 0) {
while (le16_to_cpu(dir->direntlen) >= 0) {dito, "while (true) { "
@@ -780,7 +789,7 @@ static int check_filename(char *filename, unsigned int blknr) dir = (struct ext2_dirent *)root_first_block_buffer; ptr = (char *)dir; totalbytes = 0;
- while (dir->direntlen >= 0) {
- while (le16_to_cpu(dir->direntlen) >= 0) {
dito
@@ -2234,7 +2246,7 @@ int ext4fs_mount(unsigned part_length) * and we do not support metadata_csum (and cannot reliably find * files when it is set. Refuse to mount. */
- if (data->sblock.feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT) {
- if (le32_to_cpu(data->sblock.feature_incompat) &
EXT4_FEATURE_INCOMPAT_64BIT) { printf("Unsupported feature found (64bit, possibly metadata_csum), not mounting\n"); goto fail; }
This should have a if ((data->sblock.revision_level !=0) && ... in front, features are not defined for revision 0. Applies to other places as well ...
diff --git a/fs/ext4/ext4_common.h b/fs/ext4/ext4_common.h index 48fd2ac..370a717 100644 --- a/fs/ext4/ext4_common.h +++ b/fs/ext4/ext4_common.h @@ -59,10 +59,10 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name,
#if defined(CONFIG_EXT4_WRITE) uint32_t ext4fs_div_roundup(uint32_t size, uint32_t n); -int ext4fs_checksum_update(unsigned int i); +uint16_t ext4fs_checksum_update(unsigned int i); int ext4fs_get_parent_inode_num(const char *dirname, char *dname, int flags); void ext4fs_update_parent_dentry(char *filename, int *p_ino, int file_type); -long int ext4fs_get_new_blk_no(void); +uint32_t ext4fs_get_new_blk_no(void); int ext4fs_get_new_inode_no(void); void ext4fs_reset_block_bmap(long int blockno, unsigned char *buffer, int index); diff --git a/fs/ext4/ext4_journal.c b/fs/ext4/ext4_journal.c index 3f61335..cf14049 100644 --- a/fs/ext4/ext4_journal.c +++ b/fs/ext4/ext4_journal.c @@ -151,7 +151,7 @@ int ext4fs_log_gdt(char *gd_table)
- journal_buffer -- Buffer containing meta data
- blknr -- Block number on disk of the meta data buffer
*/ -int ext4fs_log_journal(char *journal_buffer, long int blknr) +int ext4fs_log_journal(char *journal_buffer, uint32_t blknr) { struct ext_filesystem *fs = get_fs(); short i; @@ -183,7 +183,7 @@ int ext4fs_log_journal(char *journal_buffer, long int blknr) * metadata_buffer -- Buffer containing meta data
- blknr -- Block number on disk of the meta data buffer
*/ -int ext4fs_put_metadata(char *metadata_buffer, long int blknr) +int ext4fs_put_metadata(char *metadata_buffer, uint32_t blknr) { struct ext_filesystem *fs = get_fs(); if (!metadata_buffer) { @@ -215,7 +215,7 @@ void print_revoke_blks(char *revk_blk) printf("total bytes %d\n", max);
while (offset < max) {
blocknr = be32_to_cpu(*((long int *)(revk_blk + offset)));
blocknr = be32_to_cpu(*((__be32 *)(revk_blk + offset)));@@ -302,7 +302,7 @@ int check_blknr_for_revoke(long int blknr, int sequence_no) max = be32_to_cpu(header->r_count);
while (offset < max) {
blocknr = be32_to_cpu(*((long int *)
blocknr = be32_to_cpu(*((__be32 *) (revk_blk + offset))); if (blocknr == blknr) goto found;@@ -420,7 +420,7 @@ int ext4fs_check_journal_state(int recovery_flag) temp_buff); jsb = (struct journal_superblock_t *) temp_buff;
- if (fs->sb->feature_incompat & EXT3_FEATURE_INCOMPAT_RECOVER) {
- if (le32_to_cpu(fs->sb->feature_incompat) &
EXT3_FEATURE_INCOMPAT_RECOVER)
if (fs->sb->revision_level != 0) && ..