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.
