[PATCH 3/5] zfs: Fix unaligned read of uint64

[Caleb Connolly]

On 12/04/2024 20:50, Caleb Connolly wrote:
> Hi Phaedrus,
> 
> On 07/04/2024 03:47, mwleeds at mailtundra.com wrote:
>> 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()
> 
> This is very odd, aarch64 doesn't generally have these restrictions. I 
> got a bit nerdsniped when I saw this so I did some digging and figured 
> this:
> 
> 1. Your abort exception doesn't include the FAR_ELx register (which 
> should contain the address that was being accessed when the abort 
> occured). This means your board is running in EL3.
> 2. It turns out there is an "A" flag in the SCTLR_ELx register, when set 
> this flag causes a fault when trying to load from an address that isn't 
> aligned to the size of the data element (see "A, bit" on 
> https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/SCTLR-EL3--System-Control-Register--EL3-
> 
> I'm not sure who's in the "wrong" here, maybe the driver should avoid 
> unaligned accesses? But then again, I don't think you should be running 
> a ZFS driver in EL3.
> 
> I'm not familiar with the Jetson Nano, but maybe there's a documented 
> way to run U-Boot so that it isn't executing in EL3? Or if not you could 
> also try unsetting the A flag.
> 
> If this really is something to fix in the driver, I don't think 
> hotpatching every unaligned access with a malloc() is the right solution.
> 
>>
>> Signed-off-by: Phaedrus Leeds <mwleeds at mailtundra.com>
>> Tested-by: Phaedrus Leeds <mwleeds at mailtundra.com>
> 
> regarding your question about re-sending to remove these tags, I'd say 
> probably yes, and especially if you're going to send a new revision anyway.
> 
> fwiw you seem to have gotten pretty much everything else about the patch 
> submission process spot on :)

Oh, by the way, instead of Tested-by tags, can you add Fixes tags on the 
patches that contain bug fixes?

In case you're unfamiliar, you can generate them like this:

$ git config --global pretty.fixes = "Fixes: %h (\"%s\")"
$ git log --oneline --pretty=fixes <SHA>

Probably for all of yours that will be:

Fixes: 4d3c95f5ea7c ("zfs: Add ZFS filesystem support")
> 
> Kind regards,
>> ---
>>   fs/zfs/zfs.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c
>> index 61d58fce68..9a50deac18 100644
>> --- a/fs/zfs/zfs.c
>> +++ b/fs/zfs/zfs.c
>> @@ -1552,35 +1552,53 @@ nvlist_find_value(char *nvlist, char *name, 
>> int valtype, char **val,
>>               if (nelm_out)
>>                   *nelm_out = nelm;
>>               return 1;
>>           }
>>           nvlist += encode_size;    /* goto the next nvpair */
>>       }
>>       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)
>>   {
>>       char *nvpair;
>>       size_t size;
>>       int found;
>>       found = nvlist_find_value(nvlist, name, DATA_TYPE_UINT64, 
>> &nvpair, &size, 0);
>>       if (!found)
>>           return 0;
>>       if (size < sizeof(uint64_t)) {
>>           printf("invalid uint64\n");
>>           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;
>>   }
>>   char *
>>   zfs_nvlist_lookup_string(char *nvlist, char *name)
>>   {
>>       char *nvpair;
>>       char *ret;
>>       size_t slen;
> 

-- 
// Caleb (they/them)