Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] kernel: make TIF_NOTIFY_SIGNAL and core dumps co-exist
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Oleg Nesterov <oleg@redhat.com>,
        Olivier Langlois <olivier@trillion01.com>,
        Tony Battersby <tonyb@cybernetics.com>
References: <76d3418c-e9ba-4392-858a-5da8028e3526@kernel.dk>
 <CAHk-=wgBMNC1ePTgqM6f0iBH94KE5_oHQYD2sqCbjev0KaZ6Kw@mail.gmail.com>
From:   Jens Axboe <axboe@kernel.dk>
Message-ID: <7fb2d8a6-951c-092c-ccaa-15522ae2ed01@kernel.dk>
Date:   Thu, 19 Aug 2021 08:59:42 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <CAHk-=wgBMNC1ePTgqM6f0iBH94KE5_oHQYD2sqCbjev0KaZ6Kw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 8/18/21 8:57 PM, Linus Torvalds wrote:
> On Tue, Aug 17, 2021 at 8:06 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> task_work being added with notify == TWA_SIGNAL will utilize
>> TIF_NOTIFY_SIGNAL for signaling the targeted task that work is available.
>> If this happens while a task is going through a core dump, it'll
>> potentially disturb and truncate the dump as a signal interruption.
> 
> This patch seems (a) buggy and (b) hacky.
> 
>> --- a/kernel/task_work.c
>> +++ b/kernel/task_work.c
>> @@ -41,6 +41,12 @@ int task_work_add(struct task_struct *task, struct callback_head *work,
>>                 head = READ_ONCE(task->task_works);
>>                 if (unlikely(head == &work_exited))
>>                         return -ESRCH;
>> +               /*
>> +                * TIF_NOTIFY_SIGNAL notifications will interfere with
>> +                * a core dump in progress, reject them.
>> +                */
>> +               if (notify == TWA_SIGNAL && (task->flags & PF_SIGNALED))
>> +                       return -ESRCH;
> 
> This basically seems to check task->flags with no serialization.
> 
> I'm sure it works 99.9% of the time in practice, since you'd be really
> unlucky to hit any races, but I really don't see what the
> serialization logic is.
> 
> Also, the main user that actually triggered the problem already has
> 
>         if (unlikely(tsk->flags & PF_EXITING))
>                 goto fail;
> 
> just above the call to task_work_add(), so this all seems very hacky indeed.
> 
> Of course, I don't see what the serialization for _that_ one is either.
> 
> Pls explain. You can't just randomly add tests for random flags that
> get modified by other random code.

You're absolutely right. On the io_uring side, in the current tree,
there's only one check where current != task being checked - and that's
in the poll rewait arming. That one should likely just go away. It may
be fine as it is, as it just pertains to ring exit cancelations. We want
to ensure that we don't rearm poll requests if the process is canceling
and going away. I'll take a closer look at that one.

For this particular patch, I agree it's racy. I'll see if I can come up
with something better...

-- 
Jens Axboe