Move the restart syscall logic into a separate generic entry helper,
and handle that part separately from signal checking and delivery.
This is in preparation for being able to do syscall restarting
independently from handling signals.
Signed-off-by: Jens Axboe <[email protected]>
---
arch/x86/kernel/signal.c | 32 ++++++++++++++++++--------------
include/linux/entry-common.h | 14 ++++++++++++--
kernel/entry/common.c | 11 ++++++++---
3 files changed, 38 insertions(+), 19 deletions(-)
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index be0d7d4152ec..5dc1eeaf0866 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -799,21 +799,8 @@ static inline unsigned long get_nr_restart_syscall(const struct pt_regs *regs)
#endif
}
-/*
- * Note that 'init' is a special process: it doesn't get signals it doesn't
- * want to handle. Thus you cannot kill init even with a SIGKILL even by
- * mistake.
- */
-void arch_do_signal(struct pt_regs *regs)
+void arch_restart_syscall(struct pt_regs *regs)
{
- struct ksignal ksig;
-
- if (get_signal(&ksig)) {
- /* Whee! Actually deliver the signal. */
- handle_signal(&ksig, regs);
- return;
- }
-
/* Did we come from a system call? */
if (syscall_get_nr(current, regs) >= 0) {
/* Restart the system call - no handlers present */
@@ -831,12 +818,29 @@ void arch_do_signal(struct pt_regs *regs)
break;
}
}
+}
+
+/*
+ * Note that 'init' is a special process: it doesn't get signals it doesn't
+ * want to handle. Thus you cannot kill init even with a SIGKILL even by
+ * mistake.
+ */
+bool arch_do_signal(struct pt_regs *regs)
+{
+ struct ksignal ksig;
+
+ if (get_signal(&ksig)) {
+ /* Whee! Actually deliver the signal. */
+ handle_signal(&ksig, regs);
+ return true;
+ }
/*
* If there's no signal to deliver, we just put the saved sigmask
* back.
*/
restore_saved_sigmask();
+ return false;
}
void signal_fault(struct pt_regs *regs, void __user *frame, char *where)
diff --git a/include/linux/entry-common.h b/include/linux/entry-common.h
index 159c7476b11b..ccfcc4769925 100644
--- a/include/linux/entry-common.h
+++ b/include/linux/entry-common.h
@@ -262,9 +262,19 @@ static __always_inline void arch_exit_to_user_mode(void) { }
* arch_do_signal - Architecture specific signal delivery function
* @regs: Pointer to currents pt_regs
*
- * Invoked from exit_to_user_mode_loop().
+ * Invoked from exit_to_user_mode_loop(). Returns true if a signal was
+ * handled.
*/
-void arch_do_signal(struct pt_regs *regs);
+bool arch_do_signal(struct pt_regs *regs);
+
+/**
+ * arch_restart_syscall - Architecture specific syscall restarting
+ * @regs: Pointer to currents pt_regs
+ *
+ * Invoked from exit_to_user_mode_loop(), if we need to restart the current
+ * system call.
+ */
+void arch_restart_syscall(struct pt_regs *regs);
/**
* arch_syscall_exit_tracehook - Wrapper around tracehook_report_syscall_exit()
diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index d20ab4ac7183..0c0cc3cf3eba 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -135,11 +135,13 @@ static __always_inline void exit_to_user_mode(void)
}
/* Workaround to allow gradual conversion of architecture code */
-void __weak arch_do_signal(struct pt_regs *regs) { }
+bool __weak arch_do_signal(struct pt_regs *regs) { return true; }
static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
unsigned long ti_work)
{
+ bool restart_sys = ti_work & _TIF_SIGPENDING;
+
/*
* Before returning to user space ensure that all pending work
* items have been completed.
@@ -157,8 +159,8 @@ static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
if (ti_work & _TIF_PATCH_PENDING)
klp_update_patch_state(current);
- if (ti_work & _TIF_SIGPENDING)
- arch_do_signal(regs);
+ if ((ti_work & _TIF_SIGPENDING) && arch_do_signal(regs))
+ restart_sys = false;
if (ti_work & _TIF_NOTIFY_RESUME) {
tracehook_notify_resume(regs);
@@ -177,6 +179,9 @@ static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
ti_work = READ_ONCE(current_thread_info()->flags);
}
+ if (restart_sys)
+ arch_restart_syscall(regs);
+
/* Return the latest work state for arch_exit_to_user_mode() */
return ti_work;
}
--
2.28.0
On 10/05, Jens Axboe wrote:
>
> Move the restart syscall logic into a separate generic entry helper,
> and handle that part separately from signal checking and delivery.
>
> This is in preparation for being able to do syscall restarting
> independently from handling signals.
>
> Signed-off-by: Jens Axboe <[email protected]>
> ---
> arch/x86/kernel/signal.c | 32 ++++++++++++++++++--------------
> include/linux/entry-common.h | 14 ++++++++++++--
> kernel/entry/common.c | 11 ++++++++---
> 3 files changed, 38 insertions(+), 19 deletions(-)
Can't we avoid this patch and the and simplify the change in
exit_to_user_mode_loop() from the next patch? Can't the much more simple
patch below work?
Then later we can even change arch_do_signal() to accept the additional
argument, ti_work, so that it can use ti_work & TIF_NOTIFY_SIGNAL/SIGPENDING
instead of test_thread_flag/task_sigpending.
Oleg.
--- x/arch/x86/kernel/signal.c
+++ x/arch/x86/kernel/signal.c
@@ -808,7 +808,10 @@ void arch_do_signal(struct pt_regs *regs
{
struct ksignal ksig;
- if (get_signal(&ksig)) {
+ if (test_thread_flag(TIF_NOTIFY_SIGNAL))
+ tracehook_notify_signal();
+
+ if (task_sigpending(current) && get_signal(&ksig)) {
/* Whee! Actually deliver the signal. */
handle_signal(&ksig, regs);
return;
--- x/kernel/entry/common.c
+++ x/kernel/entry/common.c
@@ -155,7 +155,7 @@ static unsigned long exit_to_user_mode_l
if (ti_work & _TIF_PATCH_PENDING)
klp_update_patch_state(current);
- if (ti_work & _TIF_SIGPENDING)
+ if (ti_work & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL)
arch_do_signal(regs);
if (ti_work & _TIF_NOTIFY_RESUME) {
On 10/8/20 8:31 AM, Jens Axboe wrote:
> On 10/8/20 8:21 AM, Oleg Nesterov wrote:
>> On 10/05, Jens Axboe wrote:
>>>
>>> Move the restart syscall logic into a separate generic entry helper,
>>> and handle that part separately from signal checking and delivery.
>>>
>>> This is in preparation for being able to do syscall restarting
>>> independently from handling signals.
>>>
>>> Signed-off-by: Jens Axboe <[email protected]>
>>> ---
>>> arch/x86/kernel/signal.c | 32 ++++++++++++++++++--------------
>>> include/linux/entry-common.h | 14 ++++++++++++--
>>> kernel/entry/common.c | 11 ++++++++---
>>> 3 files changed, 38 insertions(+), 19 deletions(-)
>>
>> Can't we avoid this patch and the and simplify the change in
>> exit_to_user_mode_loop() from the next patch? Can't the much more simple
>> patch below work?
>>
>> Then later we can even change arch_do_signal() to accept the additional
>> argument, ti_work, so that it can use ti_work & TIF_NOTIFY_SIGNAL/SIGPENDING
>> instead of test_thread_flag/task_sigpending.
>
> Yeah I guess that would be a bit simpler, maybe I'm too focused on
> decoupling the two. But if we go this route, and avoid sighand->lock for
> just having TIF_NOTIFY_SIGNAL set, then that should be functionally
> equivalent as far as I'm concerned.
>
> I'll make the reduction, I'd prefer to keep this as small/simple as
> possible initially.
FWIW, then we should also just integrate the x86 define change into
that patch, so we can drop patches 3 + 5 with this change.
--
Jens Axboe
On 10/8/20 8:21 AM, Oleg Nesterov wrote:
> On 10/05, Jens Axboe wrote:
>>
>> Move the restart syscall logic into a separate generic entry helper,
>> and handle that part separately from signal checking and delivery.
>>
>> This is in preparation for being able to do syscall restarting
>> independently from handling signals.
>>
>> Signed-off-by: Jens Axboe <[email protected]>
>> ---
>> arch/x86/kernel/signal.c | 32 ++++++++++++++++++--------------
>> include/linux/entry-common.h | 14 ++++++++++++--
>> kernel/entry/common.c | 11 ++++++++---
>> 3 files changed, 38 insertions(+), 19 deletions(-)
>
> Can't we avoid this patch and the and simplify the change in
> exit_to_user_mode_loop() from the next patch? Can't the much more simple
> patch below work?
>
> Then later we can even change arch_do_signal() to accept the additional
> argument, ti_work, so that it can use ti_work & TIF_NOTIFY_SIGNAL/SIGPENDING
> instead of test_thread_flag/task_sigpending.
Yeah I guess that would be a bit simpler, maybe I'm too focused on
decoupling the two. But if we go this route, and avoid sighand->lock for
just having TIF_NOTIFY_SIGNAL set, then that should be functionally
equivalent as far as I'm concerned.
I'll make the reduction, I'd prefer to keep this as small/simple as
possible initially.
--
Jens Axboe
On 10/08, Jens Axboe wrote:
>
> On 10/8/20 8:21 AM, Oleg Nesterov wrote:
> >
> > Can't we avoid this patch and the and simplify the change in
> > exit_to_user_mode_loop() from the next patch? Can't the much more simple
> > patch below work?
> >
> > Then later we can even change arch_do_signal() to accept the additional
> > argument, ti_work, so that it can use ti_work & TIF_NOTIFY_SIGNAL/SIGPENDING
> > instead of test_thread_flag/task_sigpending.
>
> Yeah I guess that would be a bit simpler, maybe I'm too focused on
> decoupling the two. But if we go this route, and avoid sighand->lock for
> just having TIF_NOTIFY_SIGNAL set, then that should be functionally
> equivalent as far as I'm concerned.
Not sure I understand... I think that the change I propose is functionally
equivalent or I missed something.
> I'll make the reduction, I'd prefer to keep this as small/simple as
> possible initially.
Great, thanks.
Oleg.
On 10/8/20 8:45 AM, Oleg Nesterov wrote:
> On 10/08, Jens Axboe wrote:
>>
>> On 10/8/20 8:21 AM, Oleg Nesterov wrote:
>>>
>>> Can't we avoid this patch and the and simplify the change in
>>> exit_to_user_mode_loop() from the next patch? Can't the much more simple
>>> patch below work?
>>>
>>> Then later we can even change arch_do_signal() to accept the additional
>>> argument, ti_work, so that it can use ti_work & TIF_NOTIFY_SIGNAL/SIGPENDING
>>> instead of test_thread_flag/task_sigpending.
>>
>> Yeah I guess that would be a bit simpler, maybe I'm too focused on
>> decoupling the two. But if we go this route, and avoid sighand->lock for
>> just having TIF_NOTIFY_SIGNAL set, then that should be functionally
>> equivalent as far as I'm concerned.
>
> Not sure I understand... I think that the change I propose is functionally
> equivalent or I missed something.
Sorry, maybe my phrasing wasn't good, I'm totally agreeing with you :-)
Was just noting that the task_sigpending() is key for not calling
get_signal(), to avoid hitting the sighand->lock again.
--
Jens Axboe