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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Oct 29, 2024 at 10:53 PM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> 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.

I believe we do have large production binaries with more than 4GB of
text, what are we going to do about them? It would be interesting to
hear sframe people's opinion. Adding such a far-reaching new format in
2024 with these limitations is kind of sad. At the very least maybe we
should allow some form of chaining sframe definitions to cover more
than 4GB segments? Please CC relevant folks, I'm wondering what
they're thinking about this.

>
> > and also, does it mean that SFrame doesn't support executables with
> > text bigger than 4GB?
>
> Yes, but is that a realistic concern?

See above, yes. You'd be surprised. As somewhat corroborating
evidence, there were tons of problems and churn (within at least Meta)
with DWARF not supporting more than 2GB sizes, so yes, this is not an
abstract problem for sure. Modern production applications can be
ridiculously big.

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

That's a rather random limitation, IMO... How hard would it be to not
make that assumption?

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

I'm pretty sure you can do that with a custom linker script, at the
very least. Normally this probably won't happen, but I don't think
Linux dictates how many executable VMAs an application can have. And
it probably just naturally happens for JIT-ted applications (Java, Go,
etc).

Linux kernel itself has two executable segments, for instance (though
kernel is special, of course, but still).

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

This warn() will be logged across millions of machines in the fleet,
triggering alarms, people looking at this, making custom internal
patches to disable the known-to-happen warn. Why do we need all this?
This is an issue that is trivial to trigger by user process that's not
doing anything illegal. Why?

>
> --
> Josh