Re: [PATCH v14 7/8] signal: define the field siginfo.si_faultflags

Haren Myneni [off-list ref] writes:

On Mon, 2020-11-09 at 19:54 -0600, Eric W. Biederman wrote:

quoted

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.

quoted

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. 

quoted

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.

quoted

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.

quoted

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.  

quoted

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