linux-kernel - Re: [PATCH] kernel: make TIF_NOTIFY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7fb2d8a6-951c-092c-ccaa-15522ae2ed01@kernel.dk>
Date:   Thu, 19 Aug 2021 08:59:42 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Oleg Nesterov <oleg@...hat.com>,
        Olivier Langlois <olivier@...llion01.com>,
        Tony Battersby <tonyb@...ernetics.com>
Subject: Re: [PATCH] kernel: make TIF_NOTIFY_SIGNAL and core dumps co-exist

On 8/18/21 8:57 PM, Linus Torvalds wrote:
> On Tue, Aug 17, 2021 at 8:06 PM Jens Axboe <axboe@...nel.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