lkml.org 
[lkml]   [2013]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 5/5] coredump: ignore non-fatal signals when core dumping to a pipe
From
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.
>>


\
 
 \ /
  Last update: 2013-02-21 01:02    [W:1.100 / U:0.436 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site