Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RFC PATCH] kprobes/arm64: Blacklist functions called in
 '_sdei_handler'
To:     Xiongfeng Wang <wangxiongfeng2@huawei.com>
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
References: <1555751217-54310-1-git-send-email-wangxiongfeng2@huawei.com>
From:   James Morse <james.morse@arm.com>
Message-ID: <d9f89a0d-bb73-7162-9fcf-2bcc2ea4bc66@arm.com>
Date:   Thu, 25 Apr 2019 17:39:33 +0100
User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.1
MIME-Version: 1.0
In-Reply-To: <1555751217-54310-1-git-send-email-wangxiongfeng2@huawei.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Xiongfeng Wang,

On 20/04/2019 10:06, Xiongfeng Wang wrote:
> Functions called in '_sdei_handler' are needed to be marked as
> 'nokprobe'.

+ "Because these functions are called in NMI context and neither the arch-code's debug
infrastructure nor kprobes core supports this."


> ---
> When I kprobe 'sdei_smccc_smc',

Heh, when debugging this I put printk()s on the other side.


> the cpu hungs. I am not sure if it is
> becase when SDEI events interrupt EL0, '__sdei_asm_entry_trampoline'
> didn't restore vbar_el1 with kernel vectors.

Yes, we don't expect exceptions from the NMI handler, so VBAR_EL1 is left as whatever its
current state is. Today it could be the EL0 trampoline, the kernel vectors proper, or KVMs
vectors. This may change in the future, all SDEI does with VBAR_EL1 is emulate the
architecture: read it in order to fake up an interrupt on exit.


> Or is it becasue brk
> exception corrupt elr_el1 and trust firmware didn't preserve it for us?

Yes, this too! "5.2.1.1 Client responsibilities" of [0]
| The handler may modify the accessible System registers, but these must be restored
| before the handler completes.

We don't do this, because its extra work, and we don't ever anticipate it being necessary.


> drivers/firmware/arm_sdei.c | 3 +++
> 1 file changed, 3 insertions(+)

Nit: Because this patch only touches this file, the subject prefix should really be
"firmware: arm_sdei:". Running 'git log --online' over a file should give you a hint at
what the expected style is. This lets people spot the patches they need to look at.


> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index e6376f9..9cd70d1 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -165,6 +165,7 @@ static int invoke_sdei_fn(unsigned long function_id, unsigned long arg0,
>  
>  	return err;
>  }
> +NOKPROBE_SYMBOL(invoke_sdei_fn);

Ah! These are called via sdei_api_event_context(). Oops.


>  static struct sdei_event *sdei_event_find(u32 event_num)
>  {
> @@ -879,6 +880,7 @@ static void sdei_smccc_smc(unsigned long function_id,
>  {
>  	arm_smccc_smc(function_id, arg0, arg1, arg2, arg3, arg4, 0, 0, res);
>  }
> +NOKPROBE_SYMBOL(sdei_smccc_smc);
>  
>  static void sdei_smccc_hvc(unsigned long function_id,
>  			   unsigned long arg0, unsigned long arg1,
> @@ -887,6 +889,7 @@ static void sdei_smccc_hvc(unsigned long function_id,
>  {
>  	arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4, 0, 0, res);
>  }
> +NOKPROBE_SYMBOL(sdei_smccc_hvc);
>  
>  int sdei_register_ghes(struct ghes *ghes, sdei_event_callback *normal_cb,
>  		       sdei_event_callback *critical_cb)


Thanks for making this more robust!

Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James


[0]
http://infocenter.arm.com/help/topic/com.arm.doc.den0054a/ARM_DEN0054A_Software_Delegated_Exception_Interface.pdf