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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250130195116.3930772-1-wnliu@google.com>
Date: Thu, 30 Jan 2025 19:51:15 +0000
From: Weinan Liu <wnliu@...gle.com>
To: jpoimboe@...nel.org
Cc: acme@...nel.org, adrian.hunter@...el.com, 
	alexander.shishkin@...ux.intel.com, andrii.nakryiko@...il.com, 
	broonie@...nel.org, fweimer@...hat.com, indu.bhagat@...cle.com, 
	irogers@...gle.com, jolsa@...nel.org, jordalgo@...a.com, jremus@...ux.ibm.com, 
	linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org, 
	linux-toolchains@...r.kernel.org, linux-trace-kernel@...r.kernel.org, 
	luto@...nel.org, mark.rutland@....com, mathieu.desnoyers@...icios.com, 
	mhiramat@...nel.org, mingo@...nel.org, namhyung@...nel.org, 
	peterz@...radead.org, rostedt@...dmis.org, sam@...too.org, wnliu@...gle.com, 
	x86@...nel.org
Subject: Re: [PATCH v4 19/39] unwind_user/sframe: Add support for reading
 .sframe contents

On Fri, Jan 24, 2025 at 1:41 PM Josh Poimboeuf <jpoimboe@...nel.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@...nel.org> wrote:
> > > +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".
> 

Nit: swap() might be a simplify way to alternate pointers between two
fre_addr[] entries.

For example,

static __always_inline int __find_fre(struct sframe_section *sec,
				      struct sframe_fde *fde, unsigned long ip,
				      struct unwind_user_frame *frame)
{
	/* intialize fres[] with invalid value */
	struct sframe_fre fres[2] = {0};
	struct sframe_fre *fre = &fres[1], *prev_fre = fres;

	for (i = 0; i < fde->fres_num; i++) {
		swap(fre, next_fre);
		ret = __read_fre(sec, fde, fre_addr, fre);
...

		if (fre->ip_off > ip_off)
			break;
	}

	if (fre->size == 0)
		return -EINVAL;
...

}