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

[Oleg Nesterov <oleg@redhat.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.