On Tue, Aug 11, 2020 at 08:45:16AM +0200, Oleg Nesterov wrote:
> 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() ?
In theory, yes. The compiler doesn't know about locks, it can tear
writes whenever it feels like it. In practise it doesn't happen much,
but...
