On Tue, 2021-06-15 at 17:08 -0500, Eric W. Biederman wrote: > Oleg Nesterov <oleg@xxxxxxxxxx> writes: > > > > --- a/fs/coredump.c > > > +++ b/fs/coredump.c > > > @@ -519,7 +519,7 @@ static bool dump_interrupted(void) > > > * but then we need to teach dump_write() to restart and > > > clear > > > * TIF_SIGPENDING. > > > */ > > > - return signal_pending(current); > > > + return fatal_signal_pending(current) || > > > freezing(current); > > > } > > > > > > Well yes, this is what the comment says. > > > > But note that there is another reason why dump_interrupted() > > returns true > > if signal_pending(), it assumes thagt __dump_emit()- > > >__kernel_write() may > > fail anyway if signal_pending() is true. Say, pipe_write(), or iirc > > nfs, > > perhaps something else... > > > > That is why zap_threads() clears TIF_SIGPENDING. Perhaps it should > > clear > > TIF_NOTIFY_SIGNAL as well and we should change io-uring to not > > abuse the > > dumping threads? > > > > Or perhaps we should change __dump_emit() to clear signal_pending() > > and > > restart __kernel_write() if it fails or returns a short write. > > > > Otherwise the change above doesn't look like a full fix to me. > > Agreed. The coredump to a pipe will still be short. That needs > something additional. > > The problem Olivier Langlois <olivier@xxxxxxxxxxxxxx> reported was > core dumps coming up short because TIF_NOTIFY_SIGNAL was being > set during a core dump. > > We can see this with pipe_write returning -ERESTARTSYS > on a full pipe if signal_pending which includes TIF_NOTIFY_SIGNAL > is true. > Eric, I redid my test but this time instead of dumping directly into a file, I did let the coredump be piped to the systemd coredump module and the coredump generation isn't working as expected when piping. So your code review conclusions are correct.
