On Mon, 2020-11-09 at 19:54 -0600, Eric W. Biederman wrote:
> Peter you are patching buggy code for your siginfo extension can
> you please ignore vas-fault.c. The code in vas-fault.c should
> be fixed separately. Futher it uses clear_siginfo so you should
> get well defined behavior even if your new field is not initialized.
>
> I have copied the powerpc folks so hopefully this buggy code
> can be fixed.
>
> > diff --git a/arch/powerpc/platforms/powernv/vas-fault.c
> > b/arch/powerpc/platforms/powernv/vas-fault.c
> > index 3d21fce254b7..877e7d5fb4a2 100644
> > --- a/arch/powerpc/platforms/powernv/vas-fault.c
> > +++ b/arch/powerpc/platforms/powernv/vas-fault.c
> > @@ -154,6 +154,7 @@ static void update_csb(struct vas_window
> > *window,
> > info.si_errno = EFAULT;
> > info.si_code = SEGV_MAPERR;
> > info.si_addr = csb_addr;
> > + info.si_faultflags = 0;
>
Thanks Eric for your comments and pointing possible issues.
Here is the NX coprocessor interaction with user space and kernel:
- Process opens NX window / channel. The user space sends requests to
NX without kernel involvement. This request contains data buffer and
status block called coprocessor status block (CSB).
- if NX sees fault on the request buffer or on CSB address, issue an
interrupt to kernel
- kernel updates the CSB with the fault information and then the
process can reissue request.
- If the fault is on CSB address and is not a valid address, sending
SEGV signal so that the process assign proper CSB and reissue new
request
- We are not seeing the invalid CSB address in the process context, but
during handling the fault later. So thought about sending SEGV signal
instead of killing the process since it is not a standard segfault.
- All these windows will be closed upon process exit, but waits
until all pending requests are completed. So process will not exit with
pending requests, means after all faults handled if any.
- In the case of multithread applications, NX windows will be closed
with the last thread. Means other threads can still issue requests with
these windows. So to support in these applications, take PID and MM
references during window open and release them later in close.
> Powerpc folks. This code was introduced in c96c4436aba4
> ("powerpc/vas:
> Update CSB and notify process for fault CRBs") and is badly buggy.
>
> Let me count the bugs:
>
> a) Using kill_pid_info. That performs a permission check that
> does not make sense from a kernel thread.
>
> b) Manually filling in struct siginfo. Everyone gets it wrong
> and the powerpc code is no exception setting si_errno when
> that is something Linux as a rule does not do.
>
> Technically we have send_sig_fault to handle sending
> a fault from a non-sychrnous context but I am not convinced
> it make sense in this case.
Yes, kill_pid_info() -> group_send_sig_info() checks permissions which
is an extra step. I think send_sig_fault may be used to replace the
above steps.
>
> c) Sending an asynchronous SIGSEGV with the si_code set to
> SEGV_MAPERR.
> How can userspace detect it is an asynchronous signal? What can
> userspace do if it detects an asynchronous signal? If userspace
> is
> so buggered as to give your kernel thread a bogus address I
> suspect
> uncerimonious sending SIGKILL is probably the best you can do.
Application can assign new CSB and send new request when it catches the
signal. For example it can use csb_addr passed in si_addr amd decide
whether this SEGV is due to to CSB fault. Since it is an async signal,
was thinking for the application to recover instead of killing theprocess.
>
> There are some additional questionable things in that code like
> taking a
> task_struct reference simply to be able to test tsk->flags but no
> locks are held to ensure that tsk->flags are meaningful. Nor are
> any tests performed to see if the task being tested still uses
> the designated mm. I suspect exec could have been called.
tsk->flags is used to make sure not to send a signal if the task is in
exiting. We access this task under get/put_task_struct(). Also
kill_pid_info() sends signal if pid_task() is available. Since we are
taken mm reference, it can not be freed.
So the task has to be present until all NX windows are closed.
>
> In which case the code needs to check the mm, or at least play with
> exec_id to ensure you are not improperly signaling a process after
> exec.
>
> None of this is to say that update_csb is fundmentally bad or hard to
> correct just that it has some significant defects in it's
> implementation
> right now that need to be corrected. I am hoping a detailed
> accounting
> and pointing out those defects will allow the bug to be fixed.
>
> Thank you,
> Eric
