Re: [PATCH v4 19/39] unwind_user/sframe: Add support for reading .sframe contents

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Jan 24, 2025 at 1:41 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Fri, Jan 24, 2025 at 10:02:46AM -0800, Andrii Nakryiko wrote:
> > On Tue, Jan 21, 2025 at 6:32 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> > > +static __always_inline int __read_fde(struct sframe_section *sec,
> > > +                                     unsigned int fde_num,
> > > +                                     struct sframe_fde *fde)
> > > +{
> > > +       unsigned long fde_addr, ip;
> > > +
> > > +       fde_addr = sec->fdes_start + (fde_num * sizeof(struct sframe_fde));
> > > +       unsafe_copy_from_user(fde, (void __user *)fde_addr,
> > > +                             sizeof(struct sframe_fde), Efault);
> > > +
> > > +       ip = sec->sframe_start + fde->start_addr;
> > > +       if (ip < sec->text_start || ip > sec->text_end)
> >
> > ip >= sec->test_end ? ip == sec->test_end doesn't make sense, no?
>
> Believe it or not, this may have been intentional: 'text_end' can
> actually be a valid stack return address if the last instruction of the
> section is a call to a noreturn function.  The unwinder still needs to
> know the state of the stack at that point.
>

TIL...

> That said, there would be no reason to put an FDE at the end.  So yeah,
> it should be '>='.
>
> > > +static __always_inline int __read_fre(struct sframe_section *sec,
> > > +                                     struct sframe_fde *fde,
> > > +                                     unsigned long fre_addr,
> > > +                                     struct sframe_fre *fre)
> > > +{
> > > +       unsigned char fde_type = SFRAME_FUNC_FDE_TYPE(fde->info);
> > > +       unsigned char fre_type = SFRAME_FUNC_FRE_TYPE(fde->info);
> > > +       unsigned char offset_count, offset_size;
> > > +       s32 ip_off, cfa_off, ra_off, fp_off;
> > > +       unsigned long cur = fre_addr;
> > > +       unsigned char addr_size;
> > > +       u8 info;
> > > +
> > > +       addr_size = fre_type_to_size(fre_type);
> > > +       if (!addr_size)
> > > +               return -EFAULT;
> > > +
> > > +       if (fre_addr + addr_size + 1 > sec->fres_end)
> >
> > nit: isn't this the same as `fre_addr + addr_size >= sec->fres_end` ?
>
> Yes, but that would further obfuscate the fact that this is validating
> that the next two reads don't go past sec->fres_end:
>
>         UNSAFE_GET_USER_INC(ip_off, cur, addr_size, Efault);
>         UNSAFE_GET_USER_INC(info, cur, 1, Efault);
>
> The explicit "1" is for that second read.
>
> I'll do something like SFRAME_FRE_INFO_SIZE instead of 1 to make that
> clearer.

yeah, I definitely didn't connect 1 with the size of fre info byte,
named #define makes sense, thanks!

>
> > > +               return -EFAULT;
> > > +
> > > +       UNSAFE_GET_USER_INC(ip_off, cur, addr_size, Efault);
> > > +       if (fde_type == SFRAME_FDE_TYPE_PCINC && ip_off > fde->func_size)
> >
> > is ip_off == fde->func_size allowable?
>
> This was another one where I was probably thinking about the whole "last
> insn could be a call to noreturn" scenario.
>
> But even in that case, the frame would be unchanged after the call, so
> there shouldn't need to be an FRE there.
>
> In fact, for the unwinder to deal with that scenario, for a call frame
> (as opposed to an interrupted one), it should always subtract one from
> the return address before calling into sframe so that it points to the
> calling instruction.  </me makes note to go do that>
>
> So yeah, this looks like another off-by-one, caused by overthinking yet
> not thinking enough.
>
> > > +               return -EFAULT;
> > > +
> > > +       UNSAFE_GET_USER_INC(info, cur, 1, Efault);
> > > +       offset_count = SFRAME_FRE_OFFSET_COUNT(info);
> > > +       offset_size  = offset_size_enum_to_size(SFRAME_FRE_OFFSET_SIZE(info));
> > > +       if (!offset_count || !offset_size)
> > > +               return -EFAULT;
> > > +
> > > +       if (cur + (offset_count * offset_size) > sec->fres_end)
> >
> > offset_count * offset_size done in u8 can overflow, no? maybe upcast
> > to unsigned long or use check_add_overflow?
>
> offset_size is <= 2 as returned by offset_size_enum_to_size().
>
> offset_count is expected to be <= 3, enforced by the !offset_count check
> at the bottom.
>
> An overflow here would be harmless as it would be caught by the
> !offset_count anyway.  Though I also notice offset_count isn't big
> enough to hold the 2-byte SFRAME_FRE_OFFSET_COUNT() value.  Which is
> harmless for the same reason, but yeah I'll make offset_count an
> unsigned int.

yeah, the less whoever is reading has to worry about overflows, the
better, thanks!

>
> > > +static __always_inline int __find_fre(struct sframe_section *sec,
> > > +                                     struct sframe_fde *fde, unsigned long ip,
> > > +                                     struct unwind_user_frame *frame)
> > > +{
> > > +       unsigned char fde_type = SFRAME_FUNC_FDE_TYPE(fde->info);
> > > +       struct sframe_fre *fre, *prev_fre = NULL;
> > > +       struct sframe_fre fres[2];
> >
> > you only need prev_fre->ip_off, so why all this `which` and `fres[2]`
> > business if all you need is prev_fre_off and a bool whether you have
> > prev_fre at all?
>
> So this cleverness probably needs a comment.  prev_fre is actually
> needed after the loop:
>
> > > +       if (!prev_fre)
> > > +               return -EINVAL;
> > > +       fre = prev_fre;
>
> In the body of the loop, prev_fre is a tentative match, unless the next
> fre also matches, in which case that one becomes the new tentative
> match.
>
> I'll add a comment.  Also it'll probably be less confusing if I rename
> "prev_fre" to "fre", and "fre" to "next_fre".

ah, yeah, I missed that, you are right

>
> > > +       unsigned long fre_addr;
> > > +       bool which = false;
> > > +       unsigned int i;
> > > +       s32 ip_off;
> > > +
> > > +       ip_off = (s32)(ip - sec->sframe_start) - fde->start_addr;
> > > +
> > > +       if (fde_type == SFRAME_FDE_TYPE_PCMASK)
> > > +               ip_off %= fde->rep_size;
> >
> > did you check that fde->rep_size is not zero?
>
> I did not, good catch!
>
> > > +       fre_addr = sec->fres_start + fde->fres_off;
> > > +
> > > +       for (i = 0; i < fde->fres_num; i++) {
> >
> > why not binary search? seem more logical to guard against cases with
> > lots of FREs and be pretty fast in common case anyways.
>
> That would be nice, but the FREs are variable-sized and you don't know
> how big one is until you start reading it.

ah, another non-obvious thing, yeah... do you think it's worth fixing
this and making FREs binary searchable in v3?

Indu, do you have some stats on distribution of FRE count per FDE in practice?

Tbh, FRE format is bothering me quite a lot... but let's discuss that
in another thread with you and Indu

>
> --
> Josh