Haren Myneni <haren@xxxxxxxxxxxxx> writes:
> 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.
I see a lot that doesn't seem to match that description.
Consider forking a child and having that child exit, but still have
the file descriptor open. The release method will be called.
If you consider file descriptor passing there are much more interesting
cases involved.
> - 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.
Make that change at a minimum, please.
>> 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.
An async signal the process can recover from is fine (caveats about
exec, exit, and file descripotr passing aside).
You want something to mark this as an asynchronous event so that
userspace can tell that signal from other signals.
If only a single coprocessor is possible it could be as simple
as using a special si_code for this case.
>> 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().
The thing ins get/put_task_struct is about a reference count. It is not
a lock. So the task can still set PF_EXITING after you have tested it
and before you call copy_to_user. So testing PF_EXITING gets you
practically nothing.
Further the signal delivery code can already cop with attempting to send
a signal to a task that is in PF_EXITING so I don't see the point of
trying to avoid that situation.
> Also
> kill_pid_info() sends signal if pid_task() is available. Since we are
> taken mm reference, it can not be freed.
But task->mm can be different if the task has called exec.
> So the task has to be present until all NX windows are closed.
I don't see how that follows from anything the code does or you have
said. The lifetime of an mm and the lifetime of a pid are independent
of the lifetime of a task.
>> 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
Eric
