Messages in this thread | | | Date | Wed, 20 Feb 2013 15:30:41 -0800 | Subject | Re: [PATCH 5/5] coredump: ignore non-fatal signals when core dumping to a pipe | From | Mandeep Singh Baines <> |
| |
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(¤t->sighand->siglock, flags); recalc_sigpending(); spin_unlock_irqrestore(¤t->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. >>
| |