[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241030055314.2vg55ychg5osleja@treble.attlocal.net>
Date: Tue, 29 Oct 2024 22:53:14 -0700
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.kerne.org,
Jens Remus <jremus@...ux.ibm.com>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
Florian Weimer <fweimer@...hat.com>,
Andy Lutomirski <luto@...nel.org>
Subject: Re: [PATCH v3 09/19] unwind: Introduce sframe user space unwinding
On Tue, Oct 29, 2024 at 04:32:40PM -0700, Andrii Nakryiko wrote:
> It feels like this patch is trying to do too much. There is both new
> UAPI introduction, and SFrame format definition, and unwinder
> integration, etc, etc. Do you think it can be split further into more
> focused smaller patches?
True, let me see if I can split it up.
> > +
> > + if ((eppnt->p_flags & PF_X) && k < start_code)
> > + start_code = k;
> > +
> > + if ((eppnt->p_flags & PF_X) && k + eppnt->p_filesz > end_code)
> > + end_code = k + eppnt->p_filesz;
> > + break;
> > + }
> > + case PT_GNU_SFRAME:
> > + sframe_phdr = eppnt;
>
> if I understand correctly, there has to be only one sframe, is that
> right? Should we validate that?
Yes, there shouldn't be more than one PT_GNU_SFRAME for the executable
itself. I can validate that.
> > + break;
> > }
> > }
> >
> > + if (sframe_phdr)
> > + sframe_add_section(load_addr + sframe_phdr->p_vaddr,
> > + start_code, end_code);
> > +
>
> no error checking?
Good point. I remember discussing this with some people at Cauldon/LPC,
I just forgot to do it!
Right now it does all the validation at unwind, which could really slow
things down unnecessarily if the sframe isn't valid.
> > +#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME
> > +
> > +#define INIT_MM_SFRAME .sframe_mt = MTREE_INIT(sframe_mt, 0),
> > +
> > +extern void sframe_free_mm(struct mm_struct *mm);
> > +
> > +/* text_start, text_end, file_name are optional */
>
> what file_name? was that an extra argument that got removed?
Indeed, that was for some old code.
> > case PR_RISCV_SET_ICACHE_FLUSH_CTX:
> > error = RISCV_SET_ICACHE_FLUSH_CTX(arg2, arg3);
> > break;
> > + case PR_ADD_SFRAME:
> > + if (arg5)
> > + return -EINVAL;
> > + error = sframe_add_section(arg2, arg3, arg4);
>
> wouldn't it be better to make this interface extendable from the get
> go? Instead of passing 3 arguments with fixed meaning, why not pass a
> pointer to an extendable binary struct like seems to be the trend
> nowadays with nicely extensible APIs. See [0] for one such example
> (specifically, struct procmap_query). Seems more prudent, as we'll
> most probably will be adding flags, options, extra information, etc)
>
> [0] https://lore.kernel.org/linux-mm/20240627170900.1672542-3-andrii@kernel.org/
This ioctl interface was admittedly hacked together. I was hoping
somebody would suggest something better :-) I'll take a look.
> > +static int find_fde(struct sframe_section *sec, unsigned long ip,
> > + struct sframe_fde *fde)
> > +{
> > + struct sframe_fde __user *first, *last, *found = NULL;
> > + u32 ip_off, func_off_low = 0, func_off_high = -1;
> > +
> > + ip_off = ip - sec->sframe_addr;
>
> what if ip_off is larger than 4GB? ELF section can be bigger than 4GB, right?
That's baked into sframe v2.
> and also, does it mean that SFrame doesn't support executables with
> text bigger than 4GB?
Yes, but is that a realistic concern?
> > + } else {
> > + struct vm_area_struct *vma, *text_vma = NULL;
> > + VMA_ITERATOR(vmi, mm, 0);
> > +
> > + for_each_vma(vmi, vma) {
> > + if (vma->vm_file != sframe_vma->vm_file ||
> > + !(vma->vm_flags & VM_EXEC))
> > + continue;
> > +
> > + if (text_vma) {
> > + pr_warn_once("%s[%d]: multiple EXEC segments unsupported\n",
> > + current->comm, current->pid);
>
> is this just something that fundamentally can't be supported by SFrame
> format? Or just an implementation simplification?
It's a simplification I suppose.
> It's not illegal to have an executable with multiple VM_EXEC segments,
> no? Should this be a pr_warn_once() then?
I don't know, is it allowed? I've never seen it in practice. The
pr_warn_once() is not reporting that it's illegal but rather that this
corner case actually exists and maybe needs to be looked at.
--
Josh
Powered by blists - more mailing lists