• Re: [RFC PATCH] arm64: compat: Implement misalignment fixups for multiw

    From Arnd Bergmann@21:1/5 to ardb@kernel.org on Tue Jun 28 15:30:01 2022
    On Thu, Jun 23, 2022 at 7:24 PM Ard Biesheuvel <ardb@kernel.org> wrote:

    The 32-bit ARM kernel implements fixups on behalf of user space when
    using LDM/STM or LDRD/STRD instructions on addresses that are not 32-bit aligned. This is not something that is supported by the architecture,
    but was done anyway to increase compatibility with user space software,
    which mostly targeted x86 at the time and did not care about aligned accesses.

    This feature is one of the remaining impediments to being able to switch
    to 64-bit kernels on 64-bit capable hardware running 32-bit user space,
    so let's implement it for the arm64 compat layer as well.

    Note that the intent is to implement the exact same handling of
    misaligned multi-word loads and stores as the 32-bit kernel does,
    including what appears to be missing support for user space programs
    that rely on SETEND to switch to a different byte order and back. Also,
    like the 32-bit ARM version, we rely on the faulting address reported by
    the CPU to infer the memory address, instead of decoding the instruction fully to obtain this information.

    This implementation is taken from the 32-bit ARM tree, with all pieces removed that deal with instructions other than LDRD/STRD and LDM/STM, or
    that deal with alignment exceptions taken in kernel mode.

    Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

    Thanks a lot for implementing this! I know this has been a problem in particular for Debian, as the lack of this emulation has prevented them
    from migrating the build environment to modern 64-bit machines while
    the remaining 32-bit boxes in the build infrastructure are suffering from
    age.

    I've added the debian arm list and some developers that are probably
    interested in it.

    +config COMPAT_ALIGNMENT_FIXUPS
    + bool "Fix up misaligned multi-word loads and stores in user space"
    + default y

    My initial thought was that we probably want both compile-time and
    runtime switches for this, the same way that ARMV8_DEPRECATED
    does, but keeping it simple with just a compile-time option is totally
    fine as well, as far as I'm concerned.

    If we end up wanting an runtime switch after all, it should probably
    follow the interface from Documentation/arm64/legacy_instructions.rst,
    though it's not obvious how to best do it, since the instructions are
    otherwise available for aligned data as designed, and not deprecated
    at all.

    Adding calls to trace_instruction_emulation() might be helpful
    to allow tracing the fixups the say way we do for setend, swp
    and the cp15 barriers.

    + * *** NOTE ***
    + * This code is not portable to processors with late data abort handling.
    + */

    I see the comment is copied from the arm32 version. Is it actually relevant
    for arm64 though?

    +static void *
    +do_alignment_t32_to_handler(u32 *pinstr, struct pt_regs *regs,
    + union offset_union *poffset)
    +{
    + u32 instr = *pinstr;
    + u16 tinst1 = (instr >> 16) & 0xffff;
    + u16 tinst2 = instr & 0xffff;
    +
    + switch (tinst1 & 0xffe0) {
    + /* A6.3.5 Load/Store multiple */
    + case 0xe880: /* STM/STMIA/STMEA,LDM/LDMIA, PUSH/POP T2 */ + case 0xe8a0: /* ...above writeback version */
    + case 0xe900: /* STMDB/STMFD, LDMDB/LDMEA */
    + case 0xe920: /* ...above writeback version */
    + /* no need offset decision since handler calculates it */
    + return do_alignment_ldmstm;
    +
    + case 0xf840: /* POP/PUSH T3 (single register) */
    + if (RN_BITS(instr) == 13 && (tinst2 & 0x09ff) == 0x0904) {
    + u32 L = !!(LDST_L_BIT(instr));
    + const u32 subset[2] = {
    + 0xe92d0000, /* STMDB sp!,{registers} */
    + 0xe8bd0000, /* LDMIA sp!,{registers} */
    + };
    + *pinstr = subset[L] | (1<<RD_BITS(instr));
    + return do_alignment_ldmstm;
    + }

    The code clearly shows its age here, I doubt we'd do the function pointer handling the same way these days, but I think you made the right call here
    in keeping close to the original version while removing most of the irrelevant cases.

    +static int alignment_get_arm(struct pt_regs *regs, u32 __user *ip, u32 *inst)
    +{
    + u32 instr = 0;
    + int fault;
    +
    + fault = get_user(instr, ip);
    + if (fault)
    + return fault;
    +
    + *inst = __le32_to_cpu(instr);
    + return 0;
    +}
    +
    +static int alignment_get_thumb(struct pt_regs *regs, u16 __user *ip, u16 *inst)
    +{
    + u16 instr = 0;

    I think the types need to be adjusted, e.g. s/u32/__le32/ to avoid sparse warnings.

    Arnd

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Ard Biesheuvel@21:1/5 to Arnd Bergmann on Tue Jun 28 16:20:02 2022
    On Tue, 28 Jun 2022 at 15:23, Arnd Bergmann <arnd@arndb.de> wrote:

    On Thu, Jun 23, 2022 at 7:24 PM Ard Biesheuvel <ardb@kernel.org> wrote:

    The 32-bit ARM kernel implements fixups on behalf of user space when
    using LDM/STM or LDRD/STRD instructions on addresses that are not 32-bit aligned. This is not something that is supported by the architecture,
    but was done anyway to increase compatibility with user space software, which mostly targeted x86 at the time and did not care about aligned accesses.

    This feature is one of the remaining impediments to being able to switch
    to 64-bit kernels on 64-bit capable hardware running 32-bit user space,
    so let's implement it for the arm64 compat layer as well.

    Note that the intent is to implement the exact same handling of
    misaligned multi-word loads and stores as the 32-bit kernel does,
    including what appears to be missing support for user space programs
    that rely on SETEND to switch to a different byte order and back. Also, like the 32-bit ARM version, we rely on the faulting address reported by the CPU to infer the memory address, instead of decoding the instruction fully to obtain this information.

    This implementation is taken from the 32-bit ARM tree, with all pieces removed that deal with instructions other than LDRD/STRD and LDM/STM, or that deal with alignment exceptions taken in kernel mode.

    Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

    Thanks a lot for implementing this! I know this has been a problem in particular for Debian, as the lack of this emulation has prevented them
    from migrating the build environment to modern 64-bit machines while
    the remaining 32-bit boxes in the build infrastructure are suffering from age.

    I've added the debian arm list and some developers that are probably interested in it.

    +config COMPAT_ALIGNMENT_FIXUPS
    + bool "Fix up misaligned multi-word loads and stores in user space" + default y

    My initial thought was that we probably want both compile-time and
    runtime switches for this, the same way that ARMV8_DEPRECATED
    does, but keeping it simple with just a compile-time option is totally
    fine as well, as far as I'm concerned.

    If we end up wanting an runtime switch after all, it should probably
    follow the interface from Documentation/arm64/legacy_instructions.rst,
    though it's not obvious how to best do it, since the instructions are otherwise available for aligned data as designed, and not deprecated
    at all.

    Adding calls to trace_instruction_emulation() might be helpful
    to allow tracing the fixups the say way we do for setend, swp
    and the cp15 barriers.


    Yeah and I noticed some perf accounting as well, which might also be useful.

    + * *** NOTE ***
    + * This code is not portable to processors with late data abort handling. + */

    I see the comment is copied from the arm32 version. Is it actually relevant for arm64 though?


    I'll drop that - I don't know what it means anyway

    +static void *
    +do_alignment_t32_to_handler(u32 *pinstr, struct pt_regs *regs,
    + union offset_union *poffset)
    +{
    + u32 instr = *pinstr;
    + u16 tinst1 = (instr >> 16) & 0xffff;
    + u16 tinst2 = instr & 0xffff;
    +
    + switch (tinst1 & 0xffe0) {
    + /* A6.3.5 Load/Store multiple */
    + case 0xe880: /* STM/STMIA/STMEA,LDM/LDMIA, PUSH/POP T2 */
    + case 0xe8a0: /* ...above writeback version */
    + case 0xe900: /* STMDB/STMFD, LDMDB/LDMEA */
    + case 0xe920: /* ...above writeback version */
    + /* no need offset decision since handler calculates it */
    + return do_alignment_ldmstm;
    +
    + case 0xf840: /* POP/PUSH T3 (single register) */
    + if (RN_BITS(instr) == 13 && (tinst2 & 0x09ff) == 0x0904) { + u32 L = !!(LDST_L_BIT(instr));
    + const u32 subset[2] = {
    + 0xe92d0000, /* STMDB sp!,{registers} */ + 0xe8bd0000, /* LDMIA sp!,{registers} */ + };
    + *pinstr = subset[L] | (1<<RD_BITS(instr));
    + return do_alignment_ldmstm;
    + }

    The code clearly shows its age here, I doubt we'd do the function pointer handling the same way these days, but I think you made the right call here
    in keeping close to the original version while removing most of the irrelevant
    cases.


    Yeah.

    I did notice just now that the original code does not advance the IT
    state when emulating conditional Thumb instructions. Fortunately, if
    the fault is taken, we now that condition check passed, so we don't
    have to bother with that, but we should make sure that the IT state
    does not get out of sync with the subsequent instructions.

    +static int alignment_get_arm(struct pt_regs *regs, u32 __user *ip, u32 *inst)
    +{
    + u32 instr = 0;
    + int fault;
    +
    + fault = get_user(instr, ip);
    + if (fault)
    + return fault;
    +
    + *inst = __le32_to_cpu(instr);
    + return 0;
    +}
    +
    +static int alignment_get_thumb(struct pt_regs *regs, u16 __user *ip, u16 *inst)
    +{
    + u16 instr = 0;

    I think the types need to be adjusted, e.g. s/u32/__le32/ to avoid sparse warnings.


    I'll check with sparse, and fix accordingly.

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)