Re: [PATCH 2/2] io_uring: use TWA_SIGNAL for task_work if the task isn't running

From: Oleg Nesterov <oleg@redhat.com>
To: Jann Horn <jannh@google.com>
Cc: Jens Axboe <axboe@kernel.dk>,
	Peter Zijlstra <peterz@infradead.org>,
	io-uring <io-uring@vger.kernel.org>,
	stable <stable@vger.kernel.org>, Josef <josef.grieb@gmail.com>
Subject: Re: [PATCH 2/2] io_uring: use TWA_SIGNAL for task_work if the task isn't running
Date: Tue, 11 Aug 2020 08:45:16 +0200	[thread overview]
Message-ID: <20200811064516.GA21797@redhat.com> (raw)
In-Reply-To: <CAG48ez2EzOpWZbhnuBxVBXjRbLZULJJeeTBsdbL6Hzh9-1YYhA@mail.gmail.com>

On 08/11, Jann Horn wrote:
>
> > --- a/kernel/task_work.c
> > +++ b/kernel/task_work.c
> > @@ -42,7 +42,8 @@ task_work_add(struct task_struct *task, struct callback_head *work, int notify)
> >                 set_notify_resume(task);
> >                 break;
> >         case TWA_SIGNAL:
> > -               if (lock_task_sighand(task, &flags)) {
> > +               if (!(task->jobctl & JOBCTL_TASK_WORK) &&
> > +                   lock_task_sighand(task, &flags)) {
> >                         task->jobctl |= JOBCTL_TASK_WORK;
> >                         signal_wake_up(task, 0);
> >                         unlock_task_sighand(task, &flags);
> 
> I think that should work in theory, but if you want to be able to do a
> proper unlocked read of task->jobctl here, then I think you'd have to
> use READ_ONCE() here

Agreed,

> and make all existing writes to ->jobctl use
> WRITE_ONCE().

->jobctl is always modified with ->siglock held, do we really need
WRITE_ONCE() ?

> Also, I think that to make this work, stuff like get_signal() will
> need to use memory barriers to ensure that reads from ->task_works are
> ordered after ->jobctl has been cleared

Why? I don't follow.

Afaics, we only need to ensure that task_work_add() checks JOBCTL_TASK_WORK
after it adds the new work to the ->task_works list, and we can rely on
cmpxchg() ?

Oleg.