Messages in this thread | | | Date | Tue, 19 Feb 2013 12:20:04 -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 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();
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. >
| |