Re: [PATCH v5] mm,hwpoison: send SIGBUS to PF_MCE_EARLY processes on action required events

[Oscar Salvador <osalvador@suse.de>]

On Wed, Jan 20, 2021 at 04:24:22PM +0800, Aili Yao wrote:
> When a memory uncorrected error is triggered by process who accessed
> the address with error, It's Action Required Case for only current
> process which triggered this; This Action Required case means Action
> optional to other process who share the same page. Usually killing
> current process will be sufficient, other processes sharing the same
> page will get be signaled when they really touch the poisoned page.
> 
> But there is another scenario that other processes
> sharing the same page want to be signaled early with PF_MCE_EARLY set,
> In this case, we should get them into kill list and signal
> BUS_MCEERR_AO to them.
> 
> So in this patch, task_early_kill will check current process if
> force_early is set, and if not current,the code will fallback to
> find_early_kill_thread() to check if there is PF_MCE_EARLY process
> who cares the error.
> 
> In kill_proc(), BUS_MCEERR_AR is only send to current, other processes in
> kill list will be signaled with BUS_MCEERR_AO.
> 
> Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> Signed-off-by: Aili Yao <yaoaili@kingsoft.com>

Looks good to me, a few nits below.

Reviewed-by: Oscar Salvador <osalvador@suse.de>


> @@ -243,9 +243,12 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags)
>  			pfn, t->comm, t->pid);
>  
>  	if (flags & MF_ACTION_REQUIRED) {
> -		WARN_ON_ONCE(t != current);
> -		ret = force_sig_mceerr(BUS_MCEERR_AR,
> +		if (tk->tsk == current)
You can re-use "t" here.

> +			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);

I would place a brief comment above explaining why we are sending BUS_MCEER_AO
to non-current tasks.
E.g: "Signal other processes sharing the page if they have PF_MCE_EARLY set"

> @@ -457,8 +463,6 @@ static struct task_struct *task_early_kill(struct task_struct *tsk,
>  		 */
>  		if (tsk->mm == current->mm)
>  			return current;
> -		else
> -			return NULL;

 if (force_early && task->mm == current->mm)
         return current;
 

-- 
Oscar Salvador
SUSE L3