Re: [PATCH] secure unlock_task_sighand() call

[naveen yadav]

Happy Christmas !!!


Thanks Oleg and Linus for your reply.


We are facing OOPS during core dump on kernel 3.8.x on ARM target.

So we were doing core review and found this. We do not know whether
its big issue but thought to share this considering the problem


 Also I think in zap_process() there is no need to send SIGKILL to
ZOMBIE or DEAD process.

--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -271,17 +271,19 @@ static int zap_process(struct task_struct
*start, int exit_code)

-               if (t != current && t->mm) {
+               if (t->exit_state) {
+                       nr++;
+               } else if (t != current && t->mm) {
                        sigaddset(&t->pending.signal, SIGKILL);
                        signal_wake_up(t, 1);

Regards
Naveen



On Sun, Dec 22, 2013 at 8:04 PM, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> Naveen,
>
> sorry for the terse and neglectful reply yesterday.
>
> Actually, when I re-read the Linus's email, I think he already explained
> everything, so let me repeat:
>
> On 12/21, Linus Torvalds wrote:
>>
>> Did you actually *see* the problem, or was this just from looking at the code?
>
> Yes. Because this code assumes that lock_task_sighand() must not fail.
> If it fails, we have a problem which should be fixed.
>
>> We have coredump serialization in exit_mm() that I think *should* make
>> this all ok - if we still see p->mm matching our mm, I don't think it
>> should be able to get to __exit_signal() and make the sighand go away,
>> so the lock_task_sighand() shouldn't ever fail.
>
> Yes, exactly.
>
> Note that if we ignore exec, we do not need lock_task_sighand() at all,
> we could simply do spin_lock_irq(p->sighand->siglock).
>
> The caller holds mm->mmap_sem for writing, if we see p->mm == mm it
> simply can not pass exit_mm() which does down_read(&mm->mmap_sem), so
> this task can not exit.
>
> The problem is, this task can change its ->sighand in de_thread(), that
> is why we need lock_task_sighand(). But if it does exec, it can't pass
> exec_mmap() by the same reason, we hold mmap_sem.
>
>> >                         if (p->mm) {
>> >                                 if (unlikely(p->mm == mm)) {
>> > -                                       lock_task_sighand(p, &flags);
>> > -                                       nr += zap_process(p, exit_code);
>> > -                                       unlock_task_sighand(p, &flags);
>> > +                                       if (lock_task_sighand(p, &flags) {
>> > +                                               nr += zap_process(p, exit_code);
>
> But we can't silently skip a process with the same ->mm. We can't even
> skip the execing thread task if it is going to change its ->mm, even if
> it is single-threaded. Note that exec_mmap() will notice mm->core_state
> and fail. So every task with the same mm should be accounted because it
> will play with core_state->nr_threads in exit_mm(). And it should be
> killed because otherwise coredump_wait() can sleep "forever".
>
> So this is not the right change in any case. If lock_task_sighand() can
> fail we should fix something else.
>
> Oleg.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/