On Tue, 3 Jun 2014 19:26:32 +0200 Oleg Nesterov <o...@redhat.com> wrote:
> On 06/03, Steven Rostedt wrote: > > > > We were able to trigger this bug in -rt, and by review, I'm thinking > > that this could very well be a mainline bug too. I had our QA team add > > a trace patch to the kernel to prove my analysis, and it did. > > > > Here's the patch: > > > > http://rostedt.homelinux.com/private/sighand-trace.patch > > > > Let me try to explain the bug: > > > > > > CPU0 CPU1 > > ---- ---- > > [ read of /proc/<pid>/stat ] > > get_task_struct(); > > [...] > > [ <pid> exits ] > > [ parent does wait on <pid> ] > > wait_task_zombie() > > release_task() > > proc_flush_task() > > /* the above removes new access > > to the /proc system */ > > __exit_signal() > > __cleanup_sighand(sighand); > > atomic_dec_and_test(sighand->count); > > do_task_stat() > > lock_task_sighand(task); > > sighand = rcu_dereference(tsk->sighand); > > > > kmem_cache_free(sighand); > > > > if (sighand != NULL) > > spin_lock(sighand->siglock); > > > > ** BOOM! use after free ** > > Yes, ->sighand can be already freed at this point, but this should be > fine because sighand_cachep is SLAB_DESTROY_BY_RCU. Ah, I didn't notice that. This makes this even more bazaar. You know, this code could use some comments. I may send you a patch, because that __lock_task_sighand() is doing a lot of subtle things and there's not a single comment explaining it :-( > > That is why lock_task_sighand() does rcu_read_lock() and re-checks > sighand == tsk->sighand after it takes ->siglock. It is fine if it was > already freed or even reallocated via kmem_cache_alloc(sighand_cachep). > We only need to ensure that (SLAB_DESTROY_BY_RCU should ensure this) > this memory won't be returned to system, so this peace of memory must > be "struct sighand" with the properly initialized ->siglock until > rcu_read_unlock(). OK, this makes __lock_task_sighand() make some more sense. > > > Seems there is no protection between reading the sighand from proc and > > freeing it. The sighand->count is not updated, and the sighand is not > > freed via rcu. > > See above. > > > One, the spinlock in -rt is an rtmutex. The list_del_entry() bug is the > > task trying to remove itself from sighand->lock->wait_list. As the lock > > has been freed, the list head of the rtmutex is corrupted. > > looks like, SLAB_DESTROY_BY_RCU logic is broken? Could be. I'll look to see if we didn't break something. Thanks! -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
