Messages in this thread | | | Date | Sat, 30 Jun 2012 19:41:33 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 0/4] Was: deferring __fput() |
| |
On 06/30, Al Viro wrote: > > On Thu, Jun 28, 2012 at 06:45:39PM +0200, Oleg Nesterov wrote: > > Forgot to mention... > > > > And I still think that task_work_add() should not succeed unconditionally, > > it synchronize with exit_task_work(). Otherwise keyctl_session_to_parent() > > is broken. > > Hmm... Look: if nothing else, we have > /* the parent mustn't be init and mustn't be a kernel thread */ > if (parent->pid <= 1 || !parent->mm) > goto unlock; > in the caller. OTOH, on the exit side we have exit_mm() done first. And > that will have ->mm set to NULL. So we are closing a very narrow race to start > with. So why not do the following and be done with that?
Of course we can fix keyctl_session_to_parent(). But why? I mean, why do you dislike the idea to put this synchronization into add/run ?
IMO, this makes task_work much less useful/convenient. Every caller has to fight with this race somehow. And the lock it can take depends on the context, say, you can't use task_lock() in irq.
IOW, what is wrong with
[PATCH 2/4] task_work: don't rely on PF_EXITING http://marc.info/?l=linux-kernel&m=134082265321691
and [PATCH 3/4] task_work: deal with task_work callbacks adding more work http://marc.info/?t=134082275400004
?
perhaps you do not like the fact that the exiting task takes pi_lock unconditionally?
and in fact I think that probably it makes sense to change fput,
- task_work_add(current, ...); + BUG_ON(task_work_add(current, ...) != 0);
Oleg.
| |