Message-ID: <20250124214107.ycccp4gapbdudzux@jpoimboe>
Date: Fri, 24 Jan 2025 13:41:07 -0800
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: x86@...nel.org, Peter Zijlstra <peterz@...radead.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ingo Molnar <mingo@...nel.org>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	linux-kernel@...r.kernel.org, Indu Bhagat <indu.bhagat@...cle.com>,
	Mark Rutland <mark.rutland@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
	Ian Rogers <irogers@...gle.com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	linux-perf-users@...r.kernel.org, Mark Brown <broonie@...nel.org>,
	linux-toolchains@...r.kernel.org, Jordan Rome <jordalgo@...a.com>,
	Sam James <sam@...too.org>, linux-trace-kernel@...r.kernel.org,
	Jens Remus <jremus@...ux.ibm.com>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Florian Weimer <fweimer@...hat.com>,
	Andy Lutomirski <luto@...nel.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Weinan Liu <wnliu@...gle.com>
Subject: Re: [PATCH v4 19/39] unwind_user/sframe: Add support for reading
 .sframe contents

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@...nel.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.

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.

> > +               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.

> > +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".

> > +       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.

-- 
Josh

Powered by blists - more mailing lists