Re: [PATCH] coredump: Limit what can interrupt coredumps

[Jens Axboe <axboe@kernel.dk>]

On 6/12/21 8:36 AM, Olivier Langlois wrote:
> On Thu, 2021-06-10 at 15:11 -0500, Eric W. Biederman wrote:
>>
>> Olivier Langlois has been struggling with coredumps being incompletely
>> written in
>> processes using io_uring.
>>
>> Olivier Langlois <olivier@trillion01.com> writes:
>>> io_uring is a big user of task_work and any event that io_uring made
>>> a
>>> task waiting for that occurs during the core dump generation will
>>> generate a TIF_NOTIFY_SIGNAL.
>>>
>>> Here are the detailed steps of the problem:
>>> 1. io_uring calls vfs_poll() to install a task to a file wait queue
>>>    with io_async_wake() as the wakeup function cb from
>>> io_arm_poll_handler()
>>> 2. wakeup function ends up calling task_work_add() with TWA_SIGNAL
>>> 3. task_work_add() sets the TIF_NOTIFY_SIGNAL bit by calling
>>>    set_notify_signal()
>>
>> The coredump code deliberately supports being interrupted by SIGKILL,
>> and depends upon prepare_signal to filter out all other signals.   Now
>> that signal_pending includes wake ups for TIF_NOTIFY_SIGNAL this hack
>> in dump_emitted by the coredump code no longer works.
>>
>> Make the coredump code more robust by explicitly testing for all of
>> the wakeup conditions the coredump code supports.  This prevents
>> new wakeup conditions from breaking the coredump code, as well
>> as fixing the current issue.
>>
>> The filesystem code that the coredump code uses already limits
>> itself to only aborting on fatal_signal_pending.  So it should
>> not develop surprising wake-up reasons either.
>>
>> v2: Don't remove the now unnecessary code in prepare_signal.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 12db8b690010 ("entry: Add support for TIF_NOTIFY_SIGNAL")
>> Reported-by: Olivier Langlois <olivier@trillion01.com>
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
>>  fs/coredump.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/coredump.c b/fs/coredump.c
>> index 2868e3e171ae..c3d8fc14b993 100644
>> --- 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);
>>  }
>>  
>>  static void wait_for_dump_helpers(struct file *file)
> 
> Tested-by: Olivier Langlois <olivier@trillion01.com>

Thanks Olivier and Eric for taking care of this. I've been mostly
offline for more than a week, back at it next week.

-- 
Jens Axboe