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

On 11/1/24 11:38 AM, Andrii Nakryiko wrote:
> On Thu, Oct 31, 2024 at 2:38 PM Indu Bhagat <[email protected]> wrote:
>>
>> On 10/31/24 1:57 PM, Andrii Nakryiko wrote:
>>> On Tue, Oct 29, 2024 at 10:53 PM Josh Poimboeuf <[email protected]> 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/[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.
>>>
>>> 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.
>>>
>>
>> SFrame V2 does have that limitation. We can try to have 64-bit
>> representation for the 'ip' in the SFrame FDE and conditionalize it
>> somehow (say, with a flag in the header) so as to not bloat the majority
>> of applications.
> 
> Hi Indu,
> 
> I think that's prudent if we believe that SFrame is the solution here.
> See my reply to Josh. Real-world already approach 4GB limits, and
> things are not going to shrink in the years to come. So yeah, probably
> we need some adjustments to the format to at least allow 64-bit
> offsets (though trying to stick to 32-bit as much as possible, of
> course, if they work).
> 
> I'm not really familiar with the nuances of the format just yet, so
> can't really provide anything more useful at this point. What would be
> the sort of gold reference for Sframe format to familiarize myself
> thoroughly?
> 

There are some links on the SFrame wiki that can be helpful
https://sourceware.org/binutils/wiki/sframe

> BTW, I wanted to ask. Are there any plans to add SFrame support to
> Clang as well? It feels like without that there is no future for
> SFrame as a general-purpose solution for stack traces.
> 
>>
>>>>
>>>>> 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
>>