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]

Date:   Mon, 21 Mar 2022 14:20:50 -0400
From:   Tony Battersby <tonyb@...ernetics.com>
To:     Jens Axboe <axboe@...nel.dk>,
        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>
Subject: Re: [PATCH] kernel: make TIF_NOTIFY_SIGNAL and core dumps co-exist

On 8/19/21 10:59, Jens Axboe wrote:
> 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...
>

Continuing this thread from August 2021:

I previously tested a version of Jens' patch backported to 5.10 and it
fixed my problem.  Now I am trying to upgrade kernels, and 5.17 still
has the same problem - coredumps from an io_uring program to a pipe are
truncated.  Jens' patch applied to 5.17 again fixes the problem.  Has
there been any progress with fixing the problem upstream?

Reference:

https://lore.kernel.org/all/8af373ec-9609-35a4-f185-f9bdc63d39b7@cybernetics.com/
https://lore.kernel.org/all/76d3418c-e9ba-4392-858a-5da8028e3526@kernel.dk/

Tony Battersby
Cybernetics