Date:   Mon, 28 Mar 2022 17:07:25 -0500
From:   "Eric W. Biederman" <ebiederm@...ssion.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org,
        Oleg Nesterov <oleg@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andy Lutomirski <luto@...nel.org>,
        Ben Segall <bsegall@...gle.com>,
        Borislav Petkov <bp@...en8.de>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Ingo Molnar <mingo@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Mel Gorman <mgorman@...e.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Vincent Guittot <vincent.guittot@...aro.org>
Subject: Re: [PATCH] signal/x86: Delay calling signals in atomic

Sebastian Andrzej Siewior <bigeasy@...utronix.de> writes:

> On 2022-03-28 09:25:06 [-0500], Eric W. Biederman wrote:
>> Sebastian Andrzej Siewior <bigeasy@...utronix.de> writes:
>> 
>> Folks I really would have appreciated being copied on a signal handling
>> patch like this.
>
> Sorry for that. For the whole ptrace/signal part is no maintainer listed
> and I got the feeling that Oleg knows these bits.

Oleg and I maybe a couple of others have been doing the work.

I happen to know the fine differences between TIF_SIGPENDING and
TIF_NOTIFY_RESUME because I have just been doing some cleanup work in
that area.  Plus cleanup in the force_sig_info_to_task area.

>> It is too late to nack, but I think this buggy patch deserved one.  Can
>> we please fix PREEMPT_RT instead?
>
> Sure.
>
>> As far as I can tell this violates all of rules from
>> implementing/maintaining the RT kernel.  Instead of coming up with new
>> abstractions that makes sense and can use by everyone this introduces
>> a hack only for PREEMPT_RT and a pretty horrible one at that.
>>
>> This talks about int3, but the code looks for in_atomic().  Which means
>> that essentially every call of force_sig will take this path as they
>> almost all come from exception handlers.  It is the nature of signals
>> that report on faults.  An exception is raised and the kernel reports it
>> to userspace with a fault signal (aka force_sig_xxx).
>
> The int3 is invoked with disabled interrupts. There are also a few
> others path which are explicit with disabled interrupts or with a
> raw_spinlock_t which lead to an atomic section on PREEMPT_RT. Call
> chains with spinlock_t or a rwlock_t don't lead to a atomic section on
> PREEMPT_RT. Therefore I don't think this is "essentially every call of
> force_sig" that is going to use that.

I was assuming that exception handlers would be like int3 and are in
general cases that would be places that are atomic and don't allow
scheduling.  I admit I don't know the PREEMPT_RT rules though.


>> Further this code is buggy.  TIF_NOTIFY_RESUME is not the correct
>> flag to set to enter into exit_to_usermode_loop.  TIF_NOTIFY_RESUME is
>> about that happens after signal handling.  This very much needs to be
>> TIF_SIGPENDING with recalc_sigpending and friends updated to know about
>> "task->force_info".
>> 
>> Does someone own this problem?  Can that person please fix this
>> properly?
>
> Sure. Instead setting TIF_NOTIFY_RESUME you want the code updated to use
> recalc_sigpending() only. Or do you have other suggestions regarding
> fixing this properly?

The rule with TIF_SIGPENDING (which should be used if this technique is
used at all), is that recalc_sigpending needs to know how to test if
the signal is still pending.

>> I really don't think it is going to be maintainable for PREEMPT_RT to
>> maintain a separate signal delivery path for faults from the rest of
>> linux.
>
> Okay.

We have a few other cases where we deliver signals from interrupts.
Off the top of my head there is SAK and magic sysrq, but I think there
are more.  So I am also not convinced that all signals you care about
will go through force_sig_info_to_task.

What I really don't know and this is essentially a PREEMPT_RT question
is what makes int3 special?  Why don't other faults have this problem?

I remember there was a change where we had threaded interrupt handlers
to get around this for interrupt service routines.  I wonder if we need
to do something similar with faults.  Have a fast part and a threaded
part that runs in a schedulable way.  Although given that for a fault
you need to be fundamentally bound to the task/thread you faulted from
it probably just means having a way to switch to a kernel stack that you
can schedule from, and not use a reserved per cpu stack.  The
task_struct would certainly need to stay the same for all of the pieces.

Or maybe for PREMPT_RT you pick the i386 mechanism.  How does PREEMPT_RT
deal with page faults, or general protection faults?

This is my long winded way of saying that I rather expect that if
PREEMPT_RT is going to call code it has modified to be sleeping that it
would also make it safe for that code to sleep.

Further (and this is probably my ignorance) I just don't see what makes
any of this specific to just int3.  Why aren't other faults affected?

Eric

Powered by blists - more mailing lists