Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v10 08/11] arm64: Rename unwinder functions, prevent them
 from being traced and kprobed
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, jmorris@namei.org,
        linux-arm-kernel@lists.infradead.org,
        live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
References: <c05ce30dcc9be1bd6b5e24a2ca8fe1d66246980b>
 <20211015025847.17694-1-madvenka@linux.microsoft.com>
 <20211015025847.17694-9-madvenka@linux.microsoft.com>
 <20211027175325.GC58503@C02TD0UTHF1T.local>
From:   "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
Message-ID: <88b9f9fb-155f-da97-b8ef-755eaf2a4af9@linux.microsoft.com>
Date:   Wed, 27 Oct 2021 15:07:09 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <20211027175325.GC58503@C02TD0UTHF1T.local>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 10/27/21 12:53 PM, Mark Rutland wrote:
> On Thu, Oct 14, 2021 at 09:58:44PM -0500, madvenka@linux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>
>> Rename unwinder functions for consistency and better naming.
>>
>> 	- Rename start_backtrace() to unwind_start().
>> 	- Rename unwind_frame() to unwind_next().
>> 	- Rename walk_stackframe() to unwind().
> 
> This looks good to me.
> 

Thanks.

> Could we split this from the krpbes/tracing changes? I think this stands
> on it's own, and (as below) the kprobes/tracing changes need some more
> explanation, and would make sense as a separate patch.
> 

OK. I will split the patches.

>> Prevent the following unwinder functions from being traced:
>>
>> 	- unwind_start()
>> 	- unwind_next()
>>
>> 	unwind() is already prevented from being traced.
> 
> This could do with an explanation in the commis message as to why we
> need to do this. If this is fixing a latent issue, it should be in a
> preparatory patch that we can backport.
> 
> I dug into this a bit, and from taking a look, we prohibited ftrace in commit:
> 
>   0c32706dac1b0a72 ("arm64: stacktrace: avoid tracing arch_stack_walk()")
> 
> ... which is just one special case of graph return stack unbalancing,
> and should be addressed by using HAVE_FUNCTION_GRAPH_RET_ADDR_PTR, so
> with the patch making us use HAVE_FUNCTION_GRAPH_RET_ADDR_PTR, that's
> no longer necessary.
> 
> So we no longer seem to have a specific reason to prohibit ftrace
> here.
> 

OK, I will think about this and add a comment.

>> Prevent the following unwinder functions from being kprobed:
>>
>> 	- unwind_start()
>>
>> 	unwind_next() and unwind() are already prevented from being kprobed.
> 
> Likewise, I think this needs some explanation. From diggin, we
> prohibited kprobes in commit:
> 
>   ee07b93e7721ccd5 ("arm64: unwind: Prohibit probing on return_address()")
> 
> ... and the commit message says we need to do this because this is
> (transitively) called by trace_hardirqs_off(), which is kprobes
> blacklisted, but doesn't explain the actual problem this results in.
> 

OK. I will think about this and add a comment.

> AFAICT x86 directly uses __builtin_return_address() here, but that won't
> recover rewritten addresses, which seems like a bug (or at least a
> limitation) on x86, assuming I've read that correctly.
> 

OK.

Thanks,

Madhavan