From 0c17f85710a70a24ec19852a2f6f0749e807656f Mon Sep 17 00:00:00 2001 From: "mwleeds@mailtundra.com" Date: Sat, 6 Apr 2024 18:47:25 -0700 Subject: [PATCH 1/5] zfs: Fix malloc() success check This code was hitting the error code path whenever malloc() succeeded rather than when it failed, so presumably this part of the code hasn't been tested. I had to apply this fix (and others) to get U-Boot to boot from ZFS on an Nvidia Jetson TX2 NX SoM (an aarch64 computer). Signed-off-by: Phaedrus Leeds --- fs/zfs/zfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c index 1fec96cd5ce..14779dee32d 100644 --- a/fs/zfs/zfs.c +++ b/fs/zfs/zfs.c @@ -655,7 +655,7 @@ dmu_read(dnode_end_t *dn, uint64_t blkid, void **buf, dn->endian) << SPA_MINBLOCKSHIFT; *buf = malloc(size); - if (*buf) { + if (!*buf) { err = ZFS_ERR_OUT_OF_MEMORY; break; } From 6bbd7e820d3eba14eceb46c3bb42bfad1af6be3f Mon Sep 17 00:00:00 2001 From: "mwleeds@mailtundra.com" Date: Sat, 6 Apr 2024 18:47:26 -0700 Subject: [PATCH 2/5] zfs: Add a comment to clarify nvlist memory layout Signed-off-by: Phaedrus Leeds --- fs/zfs/zfs.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c index 14779dee32d..61d58fce68d 100644 --- a/fs/zfs/zfs.c +++ b/fs/zfs/zfs.c @@ -1617,6 +1617,11 @@ zfs_nvlist_lookup_nvlist(char *nvlist, char *name) &size, 0); if (!found) return 0; + + /* Allocate 12 bytes in addition to the nvlist size: One uint32 before the + * nvlist to hold the encoding method, and two zero uint32's after the + * nvlist as the NULL terminator. + */ ret = calloc(1, size + 3 * sizeof(uint32_t)); if (!ret) return 0; From 1fe745b4b92e532b0789f4fe6da6f829bba91417 Mon Sep 17 00:00:00 2001 From: "mwleeds@mailtundra.com" Date: Sat, 6 Apr 2024 18:47:27 -0700 Subject: [PATCH 3/5] zfs: Fix unaligned read of uint64 Without this patch, when trying to boot zfs using U-Boot on a Jetson TX2 NX (which is aarch64), I get a CPU reset error like so: "Synchronous Abort" handler, esr 0x96000021 elr: 00000000800c9000 lr : 00000000800c8ffc (reloc) elr: 00000000fff77000 lr : 00000000fff76ffc x0 : 00000000ffb40f04 x1 : 0000000000000000 x2 : 000000000000000a x3 : 0000000003100000 x4 : 0000000003100000 x5 : 0000000000000034 x6 : 00000000fff9cc6e x7 : 000000000000000f x8 : 00000000ff7f84a0 x9 : 0000000000000008 x10: 00000000ffb40f04 x11: 0000000000000006 x12: 000000000001869f x13: 0000000000000001 x14: 00000000ff7f84bc x15: 0000000000000010 x16: 0000000000002080 x17: 00000000001fffff x18: 00000000ff7fbdd8 x19: 00000000ffb405f8 x20: 00000000ffb40dd0 x21: 00000000fffabe5e x22: 000000ea77940000 x23: 00000000ffb42090 x24: 0000000000000000 x25: 0000000000000000 x26: 0000000000000000 x27: 0000000000000000 x28: 0000000000bab10c x29: 00000000ff7f85f0 Code: d00001a0 9103a000 94006ac6 f9401ba0 (f9400000) Resetting CPU ... This happens when be64_to_cpu() is called on a value that exists at a memory address that's 4 byte aligned but not 8 byte aligned (e.g. an address ending in 04). The call stack where that happens is: check_pool_label() -> zfs_nvlist_lookup_uint64(vdevnvlist, ZPOOL_CONFIG_ASHIFT,...) -> be64_to_cpu() Signed-off-by: Phaedrus Leeds Fixes: 4d3c95f5ea7c ("zfs: Add ZFS filesystem support") --- fs/zfs/zfs.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c index 61d58fce68d..9a50deac18b 100644 --- a/fs/zfs/zfs.c +++ b/fs/zfs/zfs.c @@ -1559,6 +1559,10 @@ nvlist_find_value(char *nvlist, char *name, int valtype, char **val, return 0; } +int is_word_aligned_ptr(void *ptr) { + return ((uintptr_t)ptr & (sizeof(void *) - 1)) == 0; +} + int zfs_nvlist_lookup_uint64(char *nvlist, char *name, uint64_t *out) { @@ -1574,6 +1578,20 @@ zfs_nvlist_lookup_uint64(char *nvlist, char *name, uint64_t *out) return ZFS_ERR_BAD_FS; } + /* On arm64, calling be64_to_cpu() on a value stored at a memory address + * that's not 8-byte aligned causes the CPU to reset. Avoid that by copying the + * value somewhere else if needed. + */ + if (!is_word_aligned_ptr((void *)nvpair)) { + uint64_t *alignedptr = malloc(sizeof(uint64_t)); + if (!alignedptr) + return 0; + memcpy(alignedptr, nvpair, sizeof(uint64_t)); + *out = be64_to_cpu(*alignedptr); + free(alignedptr); + return 1; + } + *out = be64_to_cpu(*(uint64_t *) nvpair); return 1; } From 1e85ddb784375a20e80a60d185998c2753757616 Mon Sep 17 00:00:00 2001 From: "mwleeds@mailtundra.com" Date: Sat, 6 Apr 2024 18:47:28 -0700 Subject: [PATCH 4/5] zfs: Fix return value of fs_devread() As evidenced by how other filesystems handle it, a return value of 0 from fs_devread() means failure; nonzero means success. The opposite assumption was being made in zfs.c for the use of zfs_devread() so fix the confusion by making zfs_devread() return 0 on success. It probably doesn't make sense to change the handling of zfs_devread() in zfs.c instead, because as it is it matches the semantics of the other functions there. Signed-off-by: Phaedrus Leeds --- fs/zfs/dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/zfs/dev.c b/fs/zfs/dev.c index 251e7d1f74f..fcd9893b3ac 100644 --- a/fs/zfs/dev.c +++ b/fs/zfs/dev.c @@ -26,5 +26,5 @@ void zfs_set_blk_dev(struct blk_desc *rbdd, struct disk_partition *info) int zfs_devread(int sector, int byte_offset, int byte_len, char *buf) { return fs_devread(zfs_blk_desc, part_info, sector, byte_offset, - byte_len, buf); + byte_len, buf) ? 0 : 1; } From 730c69f133edf8a38c9479e116dbd944ebb2cb6c Mon Sep 17 00:00:00 2001 From: "mwleeds@mailtundra.com" Date: Sat, 6 Apr 2024 18:47:29 -0700 Subject: [PATCH 5/5] zfs: Fix zfs_read() to actually work Without this patch, the while loop being modified goes on infinitely, but with the patch I am able to boot linux on zfs on a jetson tx2 nx. It seems like this code was never tested because the logic is clearly wrong. The function do_div(a,b) does a division that modifies the first parameter to have a = a / b, and returns the remainder of the division. So clearly in the usual case when file->offset = 0, the line "blkid = do_div(blkid, blksz);" just results in blkid being set to zero on every iteration of the loop, rather than being incremented as blocks are read. Hence the zeroth block is read over and over and this becomes an infinite loop. So instead capture the remainder of the division in a "blkoff" variable, and use that to properly calculate the memory address to move from in memmove() below. For example, if file->offset were 1337, on the first iteration of the loop blkid would be 0 and blkoff would be 1337. If the blksz is 131072 (as it was for me), that amount of data would be copied into data->file_buf. movesize would be 131072 - 1337 = 129735 so 129735 bytes would be moved into buf. On the second iteration of the loop (assuming there is one), red would be 129735, blkid would be 1, blkoff would be 0, and 131072 bytes would be copied into buf. And so on... Signed-off-by: Phaedrus Leeds --- fs/zfs/zfs.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c index 9a50deac18b..bfc11fa6676 100644 --- a/fs/zfs/zfs.c +++ b/fs/zfs/zfs.c @@ -2135,7 +2135,7 @@ zfs_read(zfs_file_t file, char *buf, uint64_t len) * Find requested blkid and the offset within that block. */ uint64_t blkid = file->offset + red; - blkid = do_div(blkid, blksz); + uint64_t blkoff = do_div(blkid, blksz); free(data->file_buf); data->file_buf = 0; @@ -2150,8 +2150,7 @@ zfs_read(zfs_file_t file, char *buf, uint64_t len) movesize = min(length, data->file_end - (int)file->offset - red); - memmove(buf, data->file_buf + file->offset + red - - data->file_start, movesize); + memmove(buf, data->file_buf + blkoff, movesize); buf += movesize; length -= movesize; red += movesize;