Message-ID: <2a6642a1-d20d-4588-9e5c-a4693f96fe38@linux.alibaba.com>
Date: Tue, 12 Mar 2024 14:05:48 +0800
From: Shuai Xue <xueshuai@...ux.alibaba.com>
To: Borislav Petkov <bp@...en8.de>
Cc: rafael@...nel.org, wangkefeng.wang@...wei.com, tanxiaofei@...wei.com,
 mawupeng1@...wei.com, tony.luck@...el.com, linmiaohe@...wei.com,
 naoya.horiguchi@....com, james.morse@....com, gregkh@...uxfoundation.org,
 will@...nel.org, jarkko@...nel.org, linux-acpi@...r.kernel.org,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
 linux-edac@...r.kernel.org, x86@...nel.org, justin.he@....com,
 ardb@...nel.org, ying.huang@...el.com, ashish.kalra@....com,
 baolin.wang@...ux.alibaba.com, tglx@...utronix.de, mingo@...hat.com,
 dave.hansen@...ux.intel.com, lenb@...nel.org, hpa@...or.com,
 robert.moore@...el.com, lvying6@...wei.com, xiexiuqi@...wei.com,
 zhuo.song@...ux.alibaba.com
Subject: Re: [PATCH v11 3/3] ACPI: APEI: handle synchronous exceptions in task
 work to send correct SIGBUS si_code



On 2024/3/8 18:18, Borislav Petkov wrote:
> On Sun, Feb 04, 2024 at 04:01:44PM +0800, Shuai Xue wrote:
>> Hardware errors could be signaled by asynchronous interrupt, e.g. when an
>> error is detected by a background scrubber, or signaled by synchronous
>> exception, e.g. when a CPU tries to access a poisoned cache line. Since
>> commit a70297d22132 ("ACPI: APEI: set memory failure flags as
>> MF_ACTION_REQUIRED on synchronous events")', the flag MF_ACTION_REQUIRED
>> could be used to determine whether a synchronous exception occurs on ARM64
>> platform. When a synchronous exception is detected, the kernel should
>> terminate the current process which accessing the poisoned page. This is
> 
> "which has accessed poison data"

Thank you. Will fix the grammer.

> 
>> done by sending a SIGBUS signal with an error code BUS_MCEERR_AR,
>> indicating an action-required machine check error on read.
>>
>> However, the memory failure recovery is incorrectly sending a SIGBUS
>> with wrong error code BUS_MCEERR_AO for synchronous errors in early kill
>> mode, even MF_ACTION_REQUIRED is set. The main problem is that
> 
> "even if"

Thank you. Will fix the grammer.


> 
>> synchronous errors are queued as a memory_failure() work, and are
>> executed within a kernel thread context, not the user-space process that
>> encountered the corrupted memory on ARM64 platform. As a result, when
>> kill_proc() is called to terminate the process, it sends the incorrect
>> SIGBUS error code because the context in which it operates is not the
>> one where the error was triggered.
>>
>> To this end, queue memory_failure() as a task_work so that it runs in
>> the context of the process that is actually consuming the poisoned data,
>> and it will send SIBBUS with si_code BUS_MCEERR_AR.
> 
> SIGBUS

Sorry, will fix the typo.
> 
>>
>> Signed-off-by: Shuai Xue <xueshuai@...ux.alibaba.com>
>> Tested-by: Ma Wupeng <mawupeng1@...wei.com>
>> Reviewed-by: Kefeng Wang <wangkefeng.wang@...wei.com>
>> Reviewed-by: Xiaofei Tan <tanxiaofei@...wei.com>
>> Reviewed-by: Baolin Wang <baolin.wang@...ux.alibaba.com>
>> ---
>>  drivers/acpi/apei/ghes.c | 77 +++++++++++++++++++++++-----------------
>>  include/acpi/ghes.h      |  3 --
>>  mm/memory-failure.c      | 13 -------
>>  3 files changed, 44 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 0892550732d4..e5086d795bee 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -465,28 +465,41 @@ static void ghes_clear_estatus(struct ghes *ghes,
>>  }
>>  
>>  /*
>> - * Called as task_work before returning to user-space.
>> - * Ensure any queued work has been done before we return to the context that
>> - * triggered the notification.
>> + * struct sync_task_work - for synchronous RAS event
> 
> What's so special about it being a "sync_"?
> 
> task_work is just fine and something else could use it too.

You are right, the `sync_task_work` is only use for synchronous RAS event right, but
it could be also use for other purpose in the future. The purpose can be specified
through flags.

I will remove the `sync_` prefix.

> 
>> + *
>> + * @twork:                callback_head for task work
>> + * @pfn:                  page frame number of corrupted page
>> + * @flags:                fine tune action taken
> 
> s/fine tune action taken/work control flags/
> 

Will fix it.

>> + *
>> + * Structure to pass task work to be handled before
>> + * ret_to_user via task_work_add().
> 
> What is "ret_to_user"?
> 
> If this is an ARM thing, then make sure you explain stuff properly and
> detailed. This driver is used by multiple architectures.

It is not ARM specific thing. I mean it is used by task_work before returning to user-space.

	+ * Structure to pass task work to be handled before
	+ * returning to user-space via task_work_add().

> 
>>   */
>> -static void ghes_kick_task_work(struct callback_head *head)
>> +struct sync_task_work {
>> +	struct callback_head twork;
>> +	u64 pfn;
>> +	int flags;
>> +};
>> +
>> +static void memory_failure_cb(struct callback_head *twork)
>>  {
>> -	struct acpi_hest_generic_status *estatus;
>> -	struct ghes_estatus_node *estatus_node;
>> -	u32 node_len;
>> +	int ret;
>> +	struct sync_task_work *twcb =
>> +		container_of(twork, struct sync_task_work, twork);
> 
> Ugly linebreak - no need for it.

Will fix it.
> 
>> -	estatus_node = container_of(head, struct ghes_estatus_node, task_work);
>> -	if (IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
>> -		memory_failure_queue_kick(estatus_node->task_work_cpu);
>> +	ret = memory_failure(twcb->pfn, twcb->flags);
>> +	gen_pool_free(ghes_estatus_pool, (unsigned long)twcb, sizeof(*twcb));
>>  
>> -	estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
>> -	node_len = GHES_ESTATUS_NODE_LEN(cper_estatus_len(estatus));
>> -	gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node, node_len);
>> +	if (!ret || ret == -EHWPOISON || ret == -EOPNOTSUPP)
>> +		return;
>> +
>> +	pr_err("Sending SIGBUS to current task due to memory error not recovered");
>> +	force_sig(SIGBUS);
>>  }
>>  
>>  static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>>  {
>>  	unsigned long pfn;
>> +	struct sync_task_work *twcb;
>>  
>>  	if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
>>  		return false;
>> @@ -499,6 +512,18 @@ static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>>  		return false;
>>  	}
>>  
>> +	if (flags == MF_ACTION_REQUIRED && current->mm) {
>> +		twcb = (void *)gen_pool_alloc(ghes_estatus_pool, sizeof(*twcb));
>> +		if (!twcb)
>> +			return false;
>> +
>> +		twcb->pfn = pfn;
>> +		twcb->flags = flags;
>> +		init_task_work(&twcb->twork, memory_failure_cb);
>> +		task_work_add(current, &twcb->twork, TWA_RESUME);
>> +		return true;
>> +	}
>> +
>>  	memory_failure_queue(pfn, flags);
>>  	return true;
>>  }
>> @@ -746,7 +771,7 @@ int cxl_cper_unregister_callback(cxl_cper_callback callback)
>>  }
>>  EXPORT_SYMBOL_NS_GPL(cxl_cper_unregister_callback, CXL);
>>  
>> -static bool ghes_do_proc(struct ghes *ghes,
>> +static void ghes_do_proc(struct ghes *ghes,
>>  			 const struct acpi_hest_generic_status *estatus)
>>  {
>>  	int sev, sec_sev;
>> @@ -814,8 +839,6 @@ static bool ghes_do_proc(struct ghes *ghes,
>>  		pr_err("Sending SIGBUS to current task due to memory error not recovered");
>>  		force_sig(SIGBUS);
>>  	}
>> -
>> -	return queued;
>>  }
>>  
>>  static void __ghes_print_estatus(const char *pfx,
>> @@ -1117,9 +1140,7 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
>>  	struct ghes_estatus_node *estatus_node;
>>  	struct acpi_hest_generic *generic;
>>  	struct acpi_hest_generic_status *estatus;
>> -	bool task_work_pending;
>>  	u32 len, node_len;
>> -	int ret;
>>  
>>  	llnode = llist_del_all(&ghes_estatus_llist);
>>  	/*
>> @@ -1134,25 +1155,16 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
>>  		estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
>>  		len = cper_estatus_len(estatus);
>>  		node_len = GHES_ESTATUS_NODE_LEN(len);
>> -		task_work_pending = ghes_do_proc(estatus_node->ghes, estatus);
>> +
>> +		ghes_do_proc(estatus_node->ghes, estatus);
>> +
>>  		if (!ghes_estatus_cached(estatus)) {
>>  			generic = estatus_node->generic;
>>  			if (ghes_print_estatus(NULL, generic, estatus))
>>  				ghes_estatus_cache_add(generic, estatus);
>>  		}
>> -
>> -		if (task_work_pending && current->mm) {
>> -			estatus_node->task_work.func = ghes_kick_task_work;
>> -			estatus_node->task_work_cpu = smp_processor_id();
>> -			ret = task_work_add(current, &estatus_node->task_work,
>> -					    TWA_RESUME);
>> -			if (ret)
>> -				estatus_node->task_work.func = NULL;
>> -		}
>> -
>> -		if (!estatus_node->task_work.func)
>> -			gen_pool_free(ghes_estatus_pool,
>> -				      (unsigned long)estatus_node, node_len);
> 
> I have no clue why this is being removed.

Before this patch, a memory_failure() work is queued into workqueue for
both the asynchronous interrupt and synchronous exception. So
memory_failure() will be executed asynchronously.  For NMIlike
notifications, commit 7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure()
queue for synchronous errors") keeps track of whether memory_failure() work
was queued, and makes task_work pending to flush out the queue.  It ensures
any queued work has been done before we return to the context that
triggered the notification.

In this patch:

	- a memory_failure() work is queued into workqueue for asynchronous interrupt
	- a memory_failure() task_work is queued by task_work_add for synchronous exception

The memory_failure() task_work will be handled before returning to user
space, so we does not need to queue a flushing task_work any anymore.

> 
> Why doesn't a synchronous exception on ARM call into ghes_proc_in_irq()?

	/*
	 * SEA can interrupt SError, mask it and describe this as an NMI so
	 * that APEI defers the handling.
	 */
	local_daif_restore(DAIF_ERRCTX);
	nmi_enter();
	 => ghes_notify_sea
		=> ghes_in_nmi_spool_from_list
			=> ghes_in_nmi_queue_one_entry	// also called in __ghes_sdei_callback
			=> irq_work_queue(&ghes_proc_irq_work);
	nmi_exit();
> 
> That SDEI thing certainly does.
> 
> Well looka here:
> 
>   7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure() queue for synchronous errors")
> 
> that thing does exactly what you're trying to "fix". So why doesn't that
> work for you?
> 

Commit a70297d22132 (ACPI: APEI: set memory failure flags as MF_ACTION_REQUIRED on synchronous events)
set MF_ACTION_REQUIRED for synchronous events.

	/*
	 * Send all the processes who have the page mapped a signal.
	 * ``action optional'' if they are not immediately affected by the error
	 * ``action required'' if error happened in current execution context
	 */
	static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags)
	{
		...
		if ((flags & MF_ACTION_REQUIRED) && (t == current))
			ret = force_sig_mceerr(BUS_MCEERR_AR,
					 (void __user *)tk->addr, addr_lsb);
		else
			ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr,
					      addr_lsb, t);
		...
	}

Because the memory_failure() running in a kthread context, the false branch in kill_proc()
will send SIGBUS with BUS_MCEERR_AO. But we except it as a BUS_MCEERR_AR.

Thank you for valuable comments :)

Best Regards,
Shuai

Powered by blists - more mailing lists