linux-kernel - Re: [PATCH] fs: process fput task_work with TWA

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: <2cdd6d47-7eb1-3ab1-7aa8-80c54819009b@kernel.dk>
Date:   Fri, 8 Jan 2021 09:26:40 -0700
From:   Jens Axboe <axboe@...nel.dk>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Oleg Nesterov <oleg@...hat.com>,
        Song Liu <songliubraving@...com>
Subject: Re: [PATCH] fs: process fput task_work with TWA_SIGNAL

On 1/8/21 9:10 AM, Jens Axboe wrote:
> On 1/8/21 8:58 AM, Al Viro wrote:
>> On Fri, Jan 08, 2021 at 08:13:25AM -0700, Jens Axboe wrote:
>>>> Anyway, bedtime for me; right now it looks like at least for task ==
>>>> current we always want TWA_SIGNAL.  I'll look into that more tomorrow
>>>> when I get up, but so far it smells like switching everything to
>>>> TWA_SIGNAL would be the right thing to do, if not going back to bool
>>>> notify for task_work_add()...
>>>
>>> Before the change, the fact that we ran task_work off get_signal() and
>>> thus processed even non-notify work in that path was a bit of a mess,
>>> imho. If you have work that needs processing now, in the same manner as
>>> signals, then you really should be using TWA_SIGNAL. For this pipe case,
>>> and I'd need to setup and reproduce it again, the task must have a
>>> signal pending and that would have previously caused the task_work to
>>> run, and now it does not. TWA_RESUME technically didn't change its
>>> behavior, it's still the same notification type, we just don't run
>>> task_work unconditionally (regardless of notification type) from
>>> get_signal().
>>
>> It sure as hell did change behaviour.  Think of the effect of getting
>> hit with SIGSTOP.  That's what that "bit of a mess" had been about.
>> Work done now vs. possibly several days later when SIGCONT finally
>> gets sent.
>>
>>> I think the main question here is if we want to re-instate the behavior
>>> of running task_work off get_signal(). I'm leaning towards not doing
>>> that and ensuring that callers that DO need that are using TWA_SIGNAL.
>>
>> Can you show the callers that DO NOT need it?
> 
> OK, so here's my suggestion:
> 
> 1) For 5.11, we just re-instate the task_work run in get_signal(). This
>    will make TWA_RESUME have the exact same behavior as before.
> 
> 2) For 5.12, I'll prepare a patch that collapses TWA_RESUME and TWA_SIGNAL,
>    turning it into a bool again (notify or no notify).
> 
> How does that sound?

Attached the patches - #1 is proposed for 5.11 to fix the current issue,
and then 2-4 can get queued for 5.12 to totally remove the difference
between TWA_RESUME and TWA_SIGNAL.

Totally untested, but pretty straight forward.

-- 
Jens Axboe


View attachment "0003-task_work-use-true-false-for-task_work_add-notificat.patch" of type "text/x-patch" (9299 bytes)

View attachment "0002-task_work-always-use-signal-work-if-notification-is-.patch" of type "text/x-patch" (4039 bytes)

View attachment "0001-task_work-unconditionally-run-task_work-from-get_sig.patch" of type "text/x-patch" (1789 bytes)

View attachment "0004-task_work-kill-enum-task_work_notify_mode.patch" of type "text/x-patch" (828 bytes)