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/[email protected]/

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