linux-kernel - Re: [PATCH RFC] kernel: decouple TASK_WORK TWA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <875z7uezys.fsf@nanos.tec.linutronix.de>
Date:   Thu, 01 Oct 2020 17:19:07 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Jens Axboe <axboe@...nel.dk>, io-uring <io-uring@...r.kernel.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Oleg Nesterov <oleg@...hat.com>
Subject: Re: [PATCH RFC] kernel: decouple TASK_WORK TWA_SIGNAL handling from signals

On Thu, Oct 01 2020 at 08:29, Jens Axboe wrote:
> This adds TIF_TASKWORK for x86, which if set, will return true on
> checking for pending signals. That in turn causes tasks to restart the
> system call, which will run the added task_work.

Huch? The syscall restart does not cause the task work to run.

> If TIF_TASKWORK is available, we'll use that for notification when
> TWA_SIGNAL is specified.  If it isn't available, the existing
> TIF_SIGPENDING path is used.

Bah. Yet another TIF flag just because.

> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1767,7 +1767,7 @@ static int io_req_task_work_add(struct io_kiocb *req, struct callback_head *cb,
>  		notify = TWA_SIGNAL;
>  
>  	ret = task_work_add(tsk, cb, notify);
> -	if (!ret)
> +	if (!ret && !notify)

!notify assumes that TWA_RESUME == 0. Fun to debug if that ever changes.

>  		wake_up_process(tsk);
> --- a/kernel/task_work.c
> +++ b/kernel/task_work.c
> @@ -28,7 +28,6 @@ int
>  task_work_add(struct task_struct *task, struct callback_head *work, int notify)
>  {
>  	struct callback_head *head;
> -	unsigned long flags;
>  
>  	do {
>  		head = READ_ONCE(task->task_works);
> @@ -41,7 +40,10 @@ task_work_add(struct task_struct *task, struct callback_head *work, int notify)
>  	case TWA_RESUME:
>  		set_notify_resume(task);
>  		break;
> -	case TWA_SIGNAL:
> +	case TWA_SIGNAL: {
> +#ifndef TIF_TASKWORK
> +		unsigned long flags;
> +
>  		/*
>  		 * Only grab the sighand lock if we don't already have some
>  		 * task_work pending. This pairs with the smp_store_mb()
> @@ -53,7 +55,12 @@ task_work_add(struct task_struct *task, struct callback_head *work, int notify)
>  			signal_wake_up(task, 0);
>  			unlock_task_sighand(task, &flags);
>  		}
> +#else
> +		set_tsk_thread_flag(task, TIF_TASKWORK);
> +		wake_up_process(task);
> +#endif

This is really a hack. TWA_SIGNAL is a misnomer with the new
functionality and combined with the above

         if (!ret && !notify)
  		wake_up_process(tsk);

there is not really a big difference between TWA_RESUME and TWA_SIGNAL
anymore. Just the delivery mode and the syscall restart magic.

>  static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
>  					    unsigned long ti_work)
>  {
> +	bool restart_sys = false;
> +
>  	/*
>  	 * Before returning to user space ensure that all pending work
>  	 * items have been completed.
> @@ -157,8 +159,13 @@ static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
>  		if (ti_work & _TIF_PATCH_PENDING)
>  			klp_update_patch_state(current);
>  
> +		if (ti_work & _TIF_TASKWORK) {
> +			task_work_run();
> +			restart_sys = true;
> +		}
> +
>  		if (ti_work & _TIF_SIGPENDING)
> -			arch_do_signal(regs);
> +			restart_sys |= !arch_do_signal(regs);
>  
>  		if (ti_work & _TIF_NOTIFY_RESUME) {
>  			clear_thread_flag(TIF_NOTIFY_RESUME);
> @@ -178,6 +185,9 @@ static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
>  		ti_work = READ_ONCE(current_thread_info()->flags);
>  	}
>  
> +	if (restart_sys)
> +		arch_restart_syscall(regs);
> +

How is that supposed to work?

Assume that both TIF_TASKWORK and TIF_SIGPENDING are set, i.e. after
running task work and requesting syscall restart there is an actual
signal to be delivered.

This needs a lot more thoughts.

Thanks,

        tglx