Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Miroslav Benes]

On Thu, 8 Oct 2020, Oleg Nesterov wrote:

> On 10/05, Jens Axboe wrote:
> >
> > Hi,
> >
> > The goal is this patch series is to decouple TWA_SIGNAL based task_work
> > from real signals and signal delivery.
> 
> I think TIF_NOTIFY_SIGNAL can have more users. Say, we can move
> try_to_freeze() from get_signal() to tracehook_notify_signal(), kill
> fake_signal_wake_up(), and remove freezing() from recalc_sigpending().
> 
> Probably the same for TIF_PATCH_PENDING, klp_send_signals() can use
> set_notify_signal() rather than signal_wake_up().

Yes, that was my impression from the patch set too, when I accidentally 
noticed it.

Jens, could you CC our live patching ML when you submit v4, please? It 
would be a nice cleanup.

Thanks
Miroslav

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

On 10/9/20 2:01 AM, Miroslav Benes wrote:
> On Thu, 8 Oct 2020, Oleg Nesterov wrote:
> 
>> On 10/05, Jens Axboe wrote:
>>>
>>> Hi,
>>>
>>> The goal is this patch series is to decouple TWA_SIGNAL based task_work
>>> from real signals and signal delivery.
>>
>> I think TIF_NOTIFY_SIGNAL can have more users. Say, we can move
>> try_to_freeze() from get_signal() to tracehook_notify_signal(), kill
>> fake_signal_wake_up(), and remove freezing() from recalc_sigpending().
>>
>> Probably the same for TIF_PATCH_PENDING, klp_send_signals() can use
>> set_notify_signal() rather than signal_wake_up().
> 
> Yes, that was my impression from the patch set too, when I accidentally 
> noticed it.
> 
> Jens, could you CC our live patching ML when you submit v4, please? It 
> would be a nice cleanup.

Definitely, though it'd be v5 at this point. But we really need to get
all archs supporting TIF_NOTIFY_SIGNAL first. Once we have that, there's
a whole slew of cleanups that'll fall out naturally:

- Removal of JOBCTL_TASK_WORK
- Removal of special path for TWA_SIGNAL in task_work
- TIF_PATCH_PENDING can be converted and then removed
- try_to_freeze() cleanup that Oleg mentioned

And probably more I'm not thinking of right now :-)

-- 
Jens Axboe

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

On 10/9/20 9:21 AM, Jens Axboe wrote:
> On 10/9/20 2:01 AM, Miroslav Benes wrote:
>> On Thu, 8 Oct 2020, Oleg Nesterov wrote:
>>
>>> On 10/05, Jens Axboe wrote:
>>>>
>>>> Hi,
>>>>
>>>> The goal is this patch series is to decouple TWA_SIGNAL based task_work
>>>> from real signals and signal delivery.
>>>
>>> I think TIF_NOTIFY_SIGNAL can have more users. Say, we can move
>>> try_to_freeze() from get_signal() to tracehook_notify_signal(), kill
>>> fake_signal_wake_up(), and remove freezing() from recalc_sigpending().
>>>
>>> Probably the same for TIF_PATCH_PENDING, klp_send_signals() can use
>>> set_notify_signal() rather than signal_wake_up().
>>
>> Yes, that was my impression from the patch set too, when I accidentally 
>> noticed it.
>>
>> Jens, could you CC our live patching ML when you submit v4, please? It 
>> would be a nice cleanup.
> 
> Definitely, though it'd be v5 at this point. But we really need to get
> all archs supporting TIF_NOTIFY_SIGNAL first. Once we have that, there's
> a whole slew of cleanups that'll fall out naturally:
> 
> - Removal of JOBCTL_TASK_WORK
> - Removal of special path for TWA_SIGNAL in task_work
> - TIF_PATCH_PENDING can be converted and then removed
> - try_to_freeze() cleanup that Oleg mentioned
> 
> And probably more I'm not thinking of right now :-)

Here's the current series, I took a stab at converting all archs to
support TIF_NOTIFY_SIGNAL so we have a base to build on top of. Most
of them were straight forward, but I need someone to fixup powerpc,
verify arm and s390.

But it's a decent start I think, and means that we can drop various
bits as is done at the end of the series. I could swap things around
a bit and avoid having the intermediate step, but I envision that
getting this in all archs will take a bit longer than just signing off
on the generic/x86 bits. So probably best to keep the series as it is
for now, and work on getting the arch bits verified/fixed/tested.

https://git.kernel.dk/cgit/linux-block/log/?h=tif-task_work

-- 
Jens Axboe

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Miroslav Benes]

On Sat, 10 Oct 2020, Jens Axboe wrote:

> On 10/9/20 9:21 AM, Jens Axboe wrote:
> > On 10/9/20 2:01 AM, Miroslav Benes wrote:
> >> On Thu, 8 Oct 2020, Oleg Nesterov wrote:
> >>
> >>> On 10/05, Jens Axboe wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> The goal is this patch series is to decouple TWA_SIGNAL based task_work
> >>>> from real signals and signal delivery.
> >>>
> >>> I think TIF_NOTIFY_SIGNAL can have more users. Say, we can move
> >>> try_to_freeze() from get_signal() to tracehook_notify_signal(), kill
> >>> fake_signal_wake_up(), and remove freezing() from recalc_sigpending().
> >>>
> >>> Probably the same for TIF_PATCH_PENDING, klp_send_signals() can use
> >>> set_notify_signal() rather than signal_wake_up().
> >>
> >> Yes, that was my impression from the patch set too, when I accidentally 
> >> noticed it.
> >>
> >> Jens, could you CC our live patching ML when you submit v4, please? It 
> >> would be a nice cleanup.
> > 
> > Definitely, though it'd be v5 at this point. But we really need to get
> > all archs supporting TIF_NOTIFY_SIGNAL first. Once we have that, there's
> > a whole slew of cleanups that'll fall out naturally:
> > 
> > - Removal of JOBCTL_TASK_WORK
> > - Removal of special path for TWA_SIGNAL in task_work
> > - TIF_PATCH_PENDING can be converted and then removed
> > - try_to_freeze() cleanup that Oleg mentioned
> > 
> > And probably more I'm not thinking of right now :-)
> 
> Here's the current series, I took a stab at converting all archs to
> support TIF_NOTIFY_SIGNAL so we have a base to build on top of. Most
> of them were straight forward, but I need someone to fixup powerpc,
> verify arm and s390.
> 
> But it's a decent start I think, and means that we can drop various
> bits as is done at the end of the series. I could swap things around
> a bit and avoid having the intermediate step, but I envision that
> getting this in all archs will take a bit longer than just signing off
> on the generic/x86 bits. So probably best to keep the series as it is
> for now, and work on getting the arch bits verified/fixed/tested.
> 
> https://git.kernel.dk/cgit/linux-block/log/?h=tif-task_work

Thanks, Jens.

Crude diff for live patching on top of the series is below. Tested only on 
x86_64, but it passes the tests without an issue.

Miroslav

---
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index f6310f848f34..3a4beb9395c4 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -9,6 +9,7 @@
 
 #include <linux/cpu.h>
 #include <linux/stacktrace.h>
+#include <linux/tracehook.h>
 #include "core.h"
 #include "patch.h"
 #include "transition.h"
@@ -369,9 +370,7 @@ static void klp_send_signals(void)
                         * Send fake signal to all non-kthread tasks which are
                         * still not migrated.
                         */
-                       spin_lock_irq(&task->sighand->siglock);
-                       signal_wake_up(task, 0);
-                       spin_unlock_irq(&task->sighand->siglock);
+                       set_notify_signal(task);
                }
        }
        read_unlock(&tasklist_lock);
diff --git a/kernel/signal.c b/kernel/signal.c
index a15c584a0455..b7cf4eda8611 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -181,8 +181,7 @@ void recalc_sigpending_and_wake(struct task_struct *t)
 
 void recalc_sigpending(void)
 {
-       if (!recalc_sigpending_tsk(current) && !freezing(current) &&
-           !klp_patch_pending(current))
+       if (!recalc_sigpending_tsk(current) && !freezing(current))
                clear_thread_flag(TIF_SIGPENDING);
 
 }

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

On 10/12/20 11:27 AM, Miroslav Benes wrote:
> On Sat, 10 Oct 2020, Jens Axboe wrote:
> 
>> On 10/9/20 9:21 AM, Jens Axboe wrote:
>>> On 10/9/20 2:01 AM, Miroslav Benes wrote:
>>>> On Thu, 8 Oct 2020, Oleg Nesterov wrote:
>>>>
>>>>> On 10/05, Jens Axboe wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> The goal is this patch series is to decouple TWA_SIGNAL based task_work
>>>>>> from real signals and signal delivery.
>>>>>
>>>>> I think TIF_NOTIFY_SIGNAL can have more users. Say, we can move
>>>>> try_to_freeze() from get_signal() to tracehook_notify_signal(), kill
>>>>> fake_signal_wake_up(), and remove freezing() from recalc_sigpending().
>>>>>
>>>>> Probably the same for TIF_PATCH_PENDING, klp_send_signals() can use
>>>>> set_notify_signal() rather than signal_wake_up().
>>>>
>>>> Yes, that was my impression from the patch set too, when I accidentally 
>>>> noticed it.
>>>>
>>>> Jens, could you CC our live patching ML when you submit v4, please? It 
>>>> would be a nice cleanup.
>>>
>>> Definitely, though it'd be v5 at this point. But we really need to get
>>> all archs supporting TIF_NOTIFY_SIGNAL first. Once we have that, there's
>>> a whole slew of cleanups that'll fall out naturally:
>>>
>>> - Removal of JOBCTL_TASK_WORK
>>> - Removal of special path for TWA_SIGNAL in task_work
>>> - TIF_PATCH_PENDING can be converted and then removed
>>> - try_to_freeze() cleanup that Oleg mentioned
>>>
>>> And probably more I'm not thinking of right now :-)
>>
>> Here's the current series, I took a stab at converting all archs to
>> support TIF_NOTIFY_SIGNAL so we have a base to build on top of. Most
>> of them were straight forward, but I need someone to fixup powerpc,
>> verify arm and s390.
>>
>> But it's a decent start I think, and means that we can drop various
>> bits as is done at the end of the series. I could swap things around
>> a bit and avoid having the intermediate step, but I envision that
>> getting this in all archs will take a bit longer than just signing off
>> on the generic/x86 bits. So probably best to keep the series as it is
>> for now, and work on getting the arch bits verified/fixed/tested.
>>
>> https://git.kernel.dk/cgit/linux-block/log/?h=tif-task_work
> 
> Thanks, Jens.
> 
> Crude diff for live patching on top of the series is below. Tested only on 
> x86_64, but it passes the tests without an issue.

Nice, thanks!

I'm continuing to hone the series, what's really missing so far is arch
review. Most conversions are straight forward, some I need folks to
definitely take a look at (arm, s390). powerpc is also a bit hair right
now, but I'm told that 5.10 will kill a TIF flag there, so that'll make
it trivial once I rebase on that.

Did a few more cleanups on top, series is in the same spot. I'll repost
once the merge window settles down.

-- 
Jens Axboe

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Thomas Gleixner]

Jens,

On Tue, Oct 13 2020 at 13:39, Jens Axboe wrote:
> On 10/12/20 11:27 AM, Miroslav Benes wrote:
> I'm continuing to hone the series, what's really missing so far is arch
> review. Most conversions are straight forward, some I need folks to
> definitely take a look at (arm, s390). powerpc is also a bit hair right
> now, but I'm told that 5.10 will kill a TIF flag there, so that'll make
> it trivial once I rebase on that.

can you pretty please not add that to anything which is not going
through kernel/entry/ ?

The amount of duplicated and differently buggy, inconsistent and
incomplete code in syscall and exception handling is just annoying.

It's perfectly fine if we keep that #ifdeffery around for a while and
encourage arch folks to move over to the generic infrastructure instead
of proliferating the status quo by adding this to their existing pile.

The #ifdef guarding this in set_notify_signal() and other core code
places wants to be:

    #if defined(CONFIG_GENERIC_ENTRY) && defined(TIF_NOTIFY_SIGNAL)

Thanks,

        tglx

Re: [PATCHSET RFC v3 0/6] Add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

On 10/13/20 5:34 PM, Thomas Gleixner wrote:
> Jens,
> 
> On Tue, Oct 13 2020 at 13:39, Jens Axboe wrote:
>> On 10/12/20 11:27 AM, Miroslav Benes wrote:
>> I'm continuing to hone the series, what's really missing so far is arch
>> review. Most conversions are straight forward, some I need folks to
>> definitely take a look at (arm, s390). powerpc is also a bit hair right
>> now, but I'm told that 5.10 will kill a TIF flag there, so that'll make
>> it trivial once I rebase on that.
> 
> can you pretty please not add that to anything which is not going
> through kernel/entry/ ?

Certainly, tif-task_work is just a holding ground for all of it so
far. Once the core bits are agreed upon and merged, then I'll bounce
the rest through the arch maintainers. So please don't view it as
on cohesive series, it only is up until (and including):

commit 8dc14b576a9bf13dba4993e4ddb4068d39dee1e9
Author: Jens Axboe <axboe@kernel.dk>
Date:   Thu Oct 1 13:29:21 2020 -0600

    task_work: use TIF_NOTIFY_SIGNAL if available


> The amount of duplicated and differently buggy, inconsistent and
> incomplete code in syscall and exception handling is just annoying.
> 
> It's perfectly fine if we keep that #ifdeffery around for a while and
> encourage arch folks to move over to the generic infrastructure instead
> of proliferating the status quo by adding this to their existing pile.

Agree

> The #ifdef guarding this in set_notify_signal() and other core code
> places wants to be:
> 
>     #if defined(CONFIG_GENERIC_ENTRY) && defined(TIF_NOTIFY_SIGNAL)

OK no problem, I'll fix that up.

-- 
Jens Axboe