Messages in this thread | | | Date | Thu, 14 Mar 2013 15:40:19 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 1/2] task_work: make FIFO task_work list |
| |
On 03/14, liguang wrote: > > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
Changelog please...
> --- > kernel/task_work.c | 15 +++------------ > 1 files changed, 3 insertions(+), 12 deletions(-) > > diff --git a/kernel/task_work.c b/kernel/task_work.c > index 65bd3c9..0bf4258 100644 > --- a/kernel/task_work.c > +++ b/kernel/task_work.c > @@ -13,11 +13,12 @@ task_work_add(struct task_struct *task, struct callback_head *work, bool notify) > head = ACCESS_ONCE(task->task_works); > if (unlikely(head == &work_exited)) > return -ESRCH; > - work->next = head; > - } while (cmpxchg(&task->task_works, head, work) != head); > + head = head->next; > + } while (cmpxchg(&head, NULL, work) == head);
I simply can't understand how this can work... The patch assumes that head->next == NULL after head = head->next, why? And then compares the result with head and succeeds if not equal.
Could you please explain how it was supposed to work? If nothing else, Suppose we have task->task_works -> W1 -> W2 -> W3. How this code can add W4 after W3?
And cmpxchg(&head) should be cmpxchg(&head->next)....
Anyway, whatever I missed this is racy.
head = head->next;
nothing protects "head" after this. Say, it can be task_work_cancel'ed and freed. So,
cmpxchg(&head, ...)
can modify the freed and reused memory.
Oleg.
| |