Re: [PATCH v3 09/19] unwind: Introduce sframe user space unwinding

From: Josh Poimboeuf
Date: Wed Oct 30 2024 - 01:53:27 EST


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@xxxxxxxxxx/

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