[PATCH 10/13] arm64: extable: add `type` and `data` fields

[Ard Biesheuvel]

On Tue, 19 Oct 2021 at 14:05, Will Deacon <will at kernel.org> wrote:
>
> On Tue, Oct 19, 2021 at 12:50:22PM +0100, Mark Rutland wrote:
> > On Tue, Oct 19, 2021 at 12:29:55PM +0100, Will Deacon wrote:
> > > On Wed, Oct 13, 2021 at 12:00:56PM +0100, Mark Rutland wrote:
> > > > Subsequent patches will add specialized handlers for fixups, in addition
> > > > to the simple PC fixup and BPF handlers we have today. In preparation,
> > > > this patch adds a new `type` field to struct exception_table_entry, and
> > > > uses this to distinguish the fixup and BPF cases. A `data` field is also
> > > > added so that subsequent patches can associate data specific to each
> > > > exception site (e.g. register numbers).
> > > >
> > > > Handlers are named ex_handler_*() for consistency, following the exmaple
> > > > of x86. At the same time, get_ex_fixup() is split out into a helper so
> > > > that it can be used by other ex_handler_*() functions ins subsequent
> > > > patches.
> > > >
> > > > This patch will increase the size of the exception tables, which will be
> > > > remedied by subsequent patches removing redundant fixup code. There
> > > > should be no functional change as a result of this patch.
> > > >
> > > > Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> > > > Cc: Alexei Starovoitov <ast at kernel.org>
> > > > Cc: Andrii Nakryiko <andrii at kernel.org>
> > > > Cc: Ard Biesheuvel <ardb at kernel.org>
> > > > Cc: Catalin Marinas <catalin.marinas at arm.com>
> > > > Cc: Daniel Borkmann <daniel at iogearbox.net>
> > > > Cc: James Morse <james.morse at arm.com>
> > > > Cc: Jean-Philippe Brucker <jean-philippe at linaro.org>
> > > > Cc: Robin Murphy <robin.murphy at arm.com>
> > > > Cc: Will Deacon <will at kernel.org>
> > > > ---
> > > >  arch/arm64/include/asm/asm-extable.h | 32 ++++++++++++++++++++------------
> > > >  arch/arm64/include/asm/extable.h     | 19 +++++++++++++++----
> > > >  arch/arm64/mm/extable.c              | 29 +++++++++++++++++++++++++----
> > > >  arch/arm64/net/bpf_jit_comp.c        |  7 +++++--
> > > >  scripts/sorttable.c                  | 30 ++++++++++++++++++++++++++++++
> > > >  5 files changed, 95 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/asm-extable.h b/arch/arm64/include/asm/asm-extable.h
> > > > index 986b4c0d4792..5ee748edaef1 100644
> > > > --- a/arch/arm64/include/asm/asm-extable.h
> > > > +++ b/arch/arm64/include/asm/asm-extable.h
> > > > @@ -2,13 +2,19 @@
> > > >  #ifndef __ASM_ASM_EXTABLE_H
> > > >  #define __ASM_ASM_EXTABLE_H
> > > >
> > > > +#define EX_TYPE_NONE                     0
> > > > +#define EX_TYPE_FIXUP                    1
> > > > +#define EX_TYPE_BPF                      2
> > > > +
> > > >  #ifdef __ASSEMBLY__
> > > >
> > > > -#define __ASM_EXTABLE_RAW(insn, fixup)           \
> > > > - .pushsection    __ex_table, "a";        \
> > > > - .align          3;                      \
> > > > - .long           ((insn) - .);           \
> > > > - .long           ((fixup) - .);          \
> > > > +#define __ASM_EXTABLE_RAW(insn, fixup, type, data)       \
> > > > + .pushsection    __ex_table, "a";                \
> > > > + .align          2;                              \
> > > > + .long           ((insn) - .);                   \
> > > > + .long           ((fixup) - .);                  \
> > > > + .short          (type);                         \
> > > > + .short          (data);                         \
> > >
> > > Why are you reducing the alignment here?
> >
> > That's because the size of each entry is now 12 bytes, and
> > `.align 3` aligns to 8 bytes, which would leave a gap between entries.
> > We only require the fields are naturally aligned, so `.align 2` is
> > sufficient, and doesn't waste space.
> >
> > I'll update the commit message to call that out.
>
> I think the part which is confusing me is that I would expect the alignment
> here to match the alignment of the corresponding C type, but the old value
> of '3' doesn't seem to do that, so is this patch fixing an earlier bug?
>
> Without your patches in the picture, we're using a '.align 3' in
> _asm_extable, but with:
>
> struct exception_table_entry
> {
>         int insn, fixup;
> };
>
> I suppose it works out because that over-alignment doesn't result in any
> additional padding, but I think we could reduce the current alignment
> without any of these other changes, no?
>

If a struct type's size happens to be a power of 2, it is perfectly
reasonable to align it to its size rather than use the minimum
alignment required by its members, as this will generally result in
more efficient placement wrt cachelines, pages, etc.

So the change is obviously needed here, but I wouldn't consider the
old alignment value to be a bug.

-- 
Ard.