linux-kernel - Re: [PATCH v2 03/11] unwind: Introduce SFrame user space unwinding

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] [day] [month] [year] [list]

Message-ID: <20241027174922.cdlkeidvciwlv5an@treble.attlocal.net>
Date: Sun, 27 Oct 2024 10:49:22 -0700
From: Josh Poimboeuf <jpoimboe@...hat.com>
To: Jens Remus <jremus@...ux.ibm.com>
Cc: 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>, x86@...nel.org,
	Heiko Carstens <hca@...ux.ibm.com>,
	Ilya Leoshkevich <iii@...ux.ibm.com>,
	Vasily Gorbik <gor@...ux.ibm.com>
Subject: Re: [PATCH v2 03/11] unwind: Introduce SFrame user space unwinding

On Wed, Oct 23, 2024 at 03:59:53PM +0200, Jens Remus wrote:
> > +++ b/arch/Kconfig
> > @@ -428,6 +428,9 @@ config HAVE_HARDLOCKUP_DETECTOR_ARCH
> >   config HAVE_USER_UNWIND
> >   	bool
> > +config HAVE_USER_UNWIND_SFRAME
> > +	bool
> 
> I found this unwinder of userspace using SFrame to depend on your generic
> unwinder of userspace framework. Does this warrant to add:

Based on your other suggestion I now have:

config UNWIND_USER
	bool

config HAVE_UNWIND_USER_FP
	bool
	select UNWIND_USER

config HAVE_UNWIND_USER_SFRAME
	bool
	select UNWIND_USER

The arches set HAVE_*, which in turn sets UNWIND_USER.

> > +static int find_fre(struct sframe_section *sec, struct sframe_fde *fde,
> > +		    unsigned long ip, struct user_unwind_frame *frame)
> > +{
> > +	unsigned char fde_type = SFRAME_FUNC_FDE_TYPE(fde->info);
> > +	unsigned char fre_type = SFRAME_FUNC_FRE_TYPE(fde->info);
> > +	s32 fre_ip_off, cfa_off, ra_off, fp_off, ip_off;
> 
> Doesn't fre_ip_off need to be u32 (see also below)? The SFrame format
> specification states the FRE sfre_start_address is either u8, u16, or u32:
> https://sourceware.org/binutils/docs/sframe-spec.html#The-SFrame-FRE-Types

Indeed, I also noticed that.

> > +	SFRAME_GET_USER_UNSIGNED(cfa_off, f, offset_size);
> 
> As far as I know the CFA offset from CFA base register is signed in the
> SFrame file format. See Binutils include/sframe-api.h,
> sframe_fre_get_cfa_offset(). Therefore use SFRAME_GET_USER_SIGNED(). Both
> cfa_off and struct user_unwind_frame cfa_off are already defined as s32.

Good catch.  There's no need to have separate SIGNED/UNSIGNED variants
anyway, I'll simplify that into SFRAME_GET_USER().

> > +++ b/mm/init-mm.c
> > @@ -11,6 +11,7 @@
> >   #include <linux/atomic.h>
> >   #include <linux/user_namespace.h>
> >   #include <linux/iommu.h>
> > +#include <linux/sframe.h>
> >   #include <asm/mmu.h>
> >   #ifndef INIT_MM_CONTEXT
> > @@ -44,7 +45,8 @@ struct mm_struct init_mm = {
> >   #endif
> >   	.user_ns	= &init_user_ns,
> >   	.cpu_bitmap	= CPU_BITS_NONE,
> > -	INIT_MM_CONTEXT(init_mm)
> > +	INIT_MM_CONTEXT(init_mm),
> > +	INIT_MM_SFRAME,
> 
> This does not compile on s390, as INIT_MM_CONTEXT() is defined with a
> trailing comma, leading to two consecutive commas. Already reported by the
> kernel test robot for other architectures.
> Same if INIT_MM_SFRAME expands into nothing there would be two consecutive
> commas.

Yeah, I saw those build errors and I've got that fixed now.

Thanks for the review!

-- 
Josh