Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0c31a02c-183e-cac6-8826-82330c6fd830@linux.microsoft.com>
Date:   Sun, 26 Jun 2022 23:51:23 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v15 3/6] arm64: Make the unwind loop in unwind() similar
 to other architectures
Content-Language: en-US
To:     Mark Rutland <mark.rutland@arm.com>
Cc:     broonie@kernel.org, jpoimboe@redhat.com, ardb@kernel.org,
        nobuta.keiya@fujitsu.com, sjitindarsingh@gmail.com,
        catalin.marinas@arm.com, will@kernel.org,
        jamorris@linux.microsoft.com, linux-arm-kernel@lists.infradead.org,
        live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
References: <ff68fb850d42e1adaa6a0a6c9c258acabb898b24>
 <20220617210717.27126-1-madvenka@linux.microsoft.com>
 <20220617210717.27126-4-madvenka@linux.microsoft.com>
 <YrgXA/x6uw1z75R2@FVFF77S0Q05N>
From:   "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
In-Reply-To: <YrgXA/x6uw1z75R2@FVFF77S0Q05N>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 6/26/22 03:21, Mark Rutland wrote:
> On Fri, Jun 17, 2022 at 04:07:14PM -0500, madvenka@linux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>
>> Change the loop in unwind()
>> ===========================
>>
>> Change the unwind loop in unwind() to:
>>
>> 	while (unwind_continue(state, consume_entry, cookie))
>> 		unwind_next(state);
>>
>> This is easy to understand and maintain.
>> New function unwind_continue()
>> ==============================
>>
>> Define a new function unwind_continue() that is used in the unwind loop
>> to check for conditions that terminate a stack trace.
>>
>> The conditions checked are:
>>
>> 	- If the bottom of the stack (final frame) has been reached,
>> 	  terminate.
>>
>> 	- If the consume_entry() function returns false, the caller of
>> 	  unwind has asked to terminate the stack trace. So, terminate.
>>
>> 	- If unwind_next() failed for some reason (like stack corruption),
>> 	  terminate.
> 
> I'm a bit confused as to why this structure, since AFAICT this doesn't match
> other architectures (looking at x86, powerpc, and s390). I note that x86 has:
> 
> * In arch_stack_walk():
> 
>         for (unwind_start(&state, task, regs, NULL); !unwind_done(&state);
>              unwind_next_frame(&state)) {
> 		...
> 		if (!consume_entry(...))
> 			break;
> 		...
> 	}
> 
> * In arch_stack_walk_reliable():
> 
>         for (unwind_start(&state, task, NULL, NULL);
>              !unwind_done(&state) && !unwind_error(&state);
>              unwind_next_frame(&state)) {
> 		...
> 		if (!consume_entry(...)
> 			return -EINVAL;
> 	}
> 
> ... and back in v6 I suggeted exactly that shape:
> 
>   https://lore.kernel.org/linux-arm-kernel/20210728165635.GA47345@C02TD0UTHF1T.local/
> 

OK. I will take a look at your suggestion and resend this patch.

>>
>> Do not return an error value from unwind_next()
>> ===============================================
>>
>> We want to check for terminating conditions only in unwind_continue() from
>> the unwinder loop. So, do not return an error value from unwind_next().
>> Simply set a flag in unwind_state and check the flag in unwind_continue().
> 
> I'm fine with the concept of moving ghe return value out of unwind_next() (e.g.
> if we go with an x86-like structure), but I don't think that we should
> centralize the other checks *and* the consumption within unwind_continue(), as
> I think those are two separate things.
> 

OK. I will address this in the next version.

>>
>> Final FP
>> ========
>>
>> Introduce a new field "final_fp" in "struct unwind_state". Initialize this
>> to the final frame of the stack trace:
>>
>> 	task_pt_regs(task)->stackframe
>>
>> This is where the stacktrace must terminate if it is successful. Add an
>> explicit comment to that effect.
> 
> Can we please make this change as a preparatory step, as with the 'task' field?
> 
> We can wrap this in a helper like:
> 
> static bool is_final_frame(struct unwind state *state)
> {
> 	return state->fp == state->final_fp;
> }
> 
> ... and use that in the main loop.
> 

OK. I will make these changes.

Thanks.

Madhavan