linux-kernel - Re: [PATCH 1/2] task_work: make FIFO task

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 15 Mar 2013 08:16:31 +0800
From:	li guang <lig.fnst@...fujitsu.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	viro@...iv.linux.org.uk, edumazet@...gle.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] task_work: make FIFO task_work list

在 2013-03-14四的 15:40 +0100，Oleg Nesterov写道：
> On 03/14, liguang wrote:
> >
> > Signed-off-by: liguang <lig.fnst@...fujitsu.com>
> 
> Changelog please...
> 

OK.

> > ---
> >  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.
> 

then ->next filed was not initialized, so I think it will
be 0'ed by compiler, is it unreliable?.

> 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?
> 

1. head = task_works
2. head = head->next
3. if head == NULL
	/* it's next node of list tail (w3->next) */
	head = work
   else
  	goto 1
	


> 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.
> 

Thanks Oleg,
Hmm, at first, I think even it was changed, it can't happened to be
NULL, but ... maybe it need more deliberation.

The motivation it make the list FIFO at task_work_add, so you don't
need to reverse it at task_work_run, and it's a time-saver if the list
doesn't have too many nodes I think.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/