Message-ID: <9d194813-ecb1-2fe4-70aa-75faf4e144ad@kernel.dk>
Date:   Wed, 11 Aug 2021 19:55:44 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Tony Battersby <tonyb@...ernetics.com>,
        Olivier Langlois <olivier@...llion01.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Oleg Nesterov <oleg@...hat.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        io-uring <io-uring@...r.kernel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        "Pavel Begunkov>" <asml.silence@...il.com>
Subject: Re: [PATCH] coredump: Limit what can interrupt coredumps

On 8/10/21 3:48 PM, Tony Battersby wrote:
> On 8/5/21 9:06 AM, Olivier Langlois wrote:
>> On Tue, 2021-06-15 at 17:08 -0500, Eric W. Biederman wrote:
>>> Oleg Nesterov <oleg@...hat.com> 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@...llion01.com> 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.
>>>
>>> Looking further if the thread that is core dumping initiated
>>> any io_uring work then io_ring_exit_work will use task_work_add
>>> to request that thread clean up it's io_uring state.
>>>
>>> Perhaps we can put a big comment in dump_emit and if we
>>> get back -ERESTARTSYS run tracework_notify_signal.  I am not
>>> seeing any locks held at that point in the coredump, so it
>>> should be safe.  The coredump is run inside of file_start_write
>>> which is the only potential complication.
>>>
>>>
>>>
>>> The code flow is complicated but it looks like the entire
>>> point of the exercise is to call io_uring_del_task_file
>>> on the originating thread.  I suppose that keeps the
>>> locking of the xarray in io_uring_task simple.
>>>
>>>
>>> Hmm.   All of this comes from io_uring_release.
>>> How do we get to io_uring_release?  The coredump should
>>> be catching everything in exit_mm before exit_files?
>>>
>>> Confused and hopeful someone can explain to me what is going on,
>>> and perhaps simplify it.
>>>
>>> Eric
>> Hi all,
>>
>> I didn't forgot about this remaining issue and I have kept thinking
>> about it on and off.
>>
>> I did try the following on 5.12.19:
>>
>> diff --git a/fs/coredump.c b/fs/coredump.c
>> index 07afb5ddb1c4..614fe7a54c1a 100644
>> --- a/fs/coredump.c
>> +++ b/fs/coredump.c
>> @@ -41,6 +41,7 @@
>>  #include <linux/fs.h>
>>  #include <linux/path.h>
>>  #include <linux/timekeeping.h>
>> +#include <linux/io_uring.h>
>>  
>>  #include <linux/uaccess.h>
>>  #include <asm/mmu_context.h>
>> @@ -625,6 +626,8 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>>  		need_suid_safe = true;
>>  	}
>>  
>> +	io_uring_files_cancel(current->files);
>> +
>>  	retval = coredump_wait(siginfo->si_signo, &core_state);
>>  	if (retval < 0)
>>  		goto fail_creds;
>> --
>> 2.32.0
>>
>> with my current understanding, io_uring_files_cancel is supposed to
>> cancel everything that might set the TIF_NOTIFY_SIGNAL.
>>
>> I must report that in my testing with generating a core dump through a
>> pipe with the modif above, I still get truncated core dumps.
>>
>> systemd is having a weird error:
>> [ 2577.870742] systemd-coredump[4056]: Failed to get COMM: No such
>> process
>>
>> and nothing is captured
>>
>> so I have replaced it with a very simple shell:
>> $ cat /proc/sys/kernel/core_pattern 
>> |/home/lano1106/bin/pipe_core.sh %e %p
>>
>> ~/bin $ cat pipe_core.sh 
>> #!/bin/sh
>>
>> cat > /home/lano1106/core/core.$1.$2
>>
>> BFD: warning: /home/lano1106/core/core.test.10886 is truncated:
>> expected core file size >= 24129536, found: 61440
>>
>> I conclude from my attempt that maybe io_uring_files_cancel is not 100%
>> cleaning everything that it should clean.
>>
>>
>>
> I just ran into this problem also - coredumps from an io_uring program
> to a pipe are truncated.  But I am using kernel 5.10.57, which does NOT
> have commit 12db8b690010 ("entry: Add support for TIF_NOTIFY_SIGNAL") or
> commit 06af8679449d ("coredump: Limit what can interrupt coredumps"). 
> Kernel 5.4 works though, so I bisected the problem to commit
> f38c7e3abfba ("io_uring: ensure async buffered read-retry is setup
> properly") in kernel 5.9.  Note that my io_uring program uses only async
> buffered reads, which may be why this particular commit makes a
> difference to my program.
> 
> My io_uring program is a multi-purpose long-running program with many
> threads.  Most threads don't use io_uring but a few of them do. 
> Normally, my core dumps are piped to a program so that they can be
> compressed before being written to disk, but I can also test writing the
> core dumps directly to disk.  This is what I have found:
> 
> *) Unpatched 5.10.57: if a thread that doesn't use io_uring triggers a
> coredump, the core file is written correctly, whether it is written to
> disk or piped to a program, even if another thread is using io_uring at
> the same time.
> 
> *) Unpatched 5.10.57: if a thread that uses io_uring triggers a
> coredump, the core file is truncated, whether written directly to disk
> or piped to a program.
> 
> *) 5.10.57+backport 06af8679449d: if a thread that uses io_uring
> triggers a coredump, and the core is written directly to disk, then it
> is written correctly.
> 
> *) 5.10.57+backport 06af8679449d: if a thread that uses io_uring
> triggers a coredump, and the core is piped to a program, then it is
> truncated.
> 
> *) 5.10.57+revert f38c7e3abfba: core dumps are written correctly,
> whether written directly to disk or piped to a program.

That is very interesting. Like Olivier mentioned, it's not that actual
commit, but rather the change of behavior implemented by it. Before that
commit, we'd hit the async workers more often, whereas after we do the
correct retry method where it's driven by the wakeup when the page is
unlocked. This is purely speculation, but perhaps the fact that the
process changes state potentially mid dump is why the dump ends up being
truncated?

I'd love to dive into this and try and figure it out. Absent a test
case, at least the above gives me an idea of what to try out. I'll see
if it makes it easier for me to create a case that does result in a
truncated core dump.

-- 
Jens Axboe

Powered by blists - more mailing lists