Re: [PATCH 5/5] coredump: ignore non-fatal signals when core dumping to a pipe

[Mandeep Singh Baines <msb@chromium.org>]

On Tue, Feb 19, 2013 at 12:20 PM, Mandeep Singh Baines <msb@chromium.org> wrote:
> On Tue, Feb 19, 2013 at 11:45 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> On 02/19, Mandeep Singh Baines wrote:
>>>
>>> On Tue, Feb 19, 2013 at 6:27 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>>> > Please look at 1-3 I sent. Btw, I slightly tested this series, seems
>>> > to work...
>>> >
>>>
>>> They look good to me. I plan on applying them to our tree since we
>>> need a fix ASAP.
>>
>> Great!
>>
>>> >> You'd need to prevent the fake signal from freeezer from setting
>>> >> TIF_SIGPENDING. Maybe just add a SIGNAL_GROUP_EXIT check in freezer.c.
>>> >
>>> > I am thinking about checking SIGNAL_GROUP_COREDUMP but I am not sure,
>>> > perhaps we can make a simpler solution. As for wait_for_dump_helper()
>>> > we do not need any check at all, but we should either fix
>>> > wait_event_freezable (it is actually not right) or change pipe_release()
>>>
>>> Is the bug that it will exit on the fake_signal.
>>
>> Yes, I understand, but
>>
>>> I don't think that bug will affects this patch though.  I think this
>>> should all work if we add a check to freezer.c (or something similar
>>> that is cleaner).
>>>
>>> If you add SIGNAL_GROUP_COREDUMP check to freezer.c:
>>>
>>> static void fake_signal_wake_up(struct task_struct *p)
>>> {
>>>         unsigned long flags;
>>>
>>>         if (lock_task_sighand(p, &flags)) {
>>> -                signal_wake_up(p, 0);
>>> +              if (!p->signal->flags & SIGNAL_GROUP_COREDUMP)
>>> +                              signal_wake_up(p, 0);
>>>                 unlock_task_sighand(p, &flags);
>>>         }
>>> }
>>>
>>> And change the wait_event_freezekillable() in this patch to just
>>> wait_event_freezable(), shouldn't that just work.
>>
>> I doubt,
>>
>>> The fake signal will never get sent.
>>
>> Yes but try_to_freeze_tasks() can fail.
>>
>
> Ah. Good point. How about this then:
>
> /* can't use wait_event_freezable since we suppress the fake signal on
> SIGNAL_GROUP_COREDUMP */
> freezer_do_not_count();
> wait_event_interruptible(pipe->wait, pipe->readers == 1);
> freezer_count();
>

No that won't work either. You can't block the fake signal. Since the
dump catcher can keep pipe_write blocked for unbounded time,
user-space can block suspend for unbounded time.

Even worse than bad core dumps is unreliable suspend/resume.

Here the patch I eventually applied to out tree:

{
        struct pipe_inode_info *pipe;

        pipe = file->f_path.dentry->d_inode->i_pipe;

        pipe_lock(pipe);
        pipe->readers++;
        pipe->writers--;

        while (pipe->readers > 1) {
                unsigned long flags;

                wake_up_interruptible_sync(&pipe->wait);
                kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
                pipe_wait(pipe);

                pipe_unlock(pipe);
                try_to_freeze();
                pipe_lock(pipe);

                if (fatal_signal_pending(current))
                        break;

                /* Clear fake signal from freeze_task(). */
                spin_lock_irqsave(&current->sighand->siglock, flags);
                recalc_sigpending();
                spin_unlock_irqrestore(&current->sighand->siglock, flags);
        }

        pipe->readers--;
        pipe->writers++;
        pipe_unlock(pipe);

}

On suspend, you might truncate a core-dump but at least you can
reliably suspend.

Regards,
Mandeep

> Regards,
> Mandeep
>
>> And once again, if wait_event_freezable() was correct we do not care
>> about the fake signal (in wait_for_dump_helper), so we do not need
>> to change fake_signal_wake_up. So perhaps we should fix it but this
>> needs some discussion.
>>
>> Sorry again for the terse reply (and perhaps I misunderstood you),
>> I'll try to return to this problem asap. In any case I still think
>> we should do the freezer fixes on top of signal fixes I sent, and
>> you seem to agree. Good ;)
>>
>> Oleg.
>>