Re: [PATCH v3 2/3] mm: only interrupt taking all mm locks on fatal signal

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Mikulas Patocka <mpatocka@redhat.com> [260104 16:17]:
> If a process sets up a timer that periodically sends a signal in short
> intervals and if it executes some kernel code that calls
> mm_take_all_locks, we get random -EINTR failures.
> 
> The function mm_take_all_locks fails with -EINTR if there is pending
> signal. The -EINTR is propagated up the call stack to userspace and
> userspace fails if it gets this error.
> 
> In order to fix these failures, this commit changes
> signal_pending(current) to fatal_signal_pending(current) in
> mm_take_all_locks, so that it is interrupted only if the signal is
> actually killing the process.

I may be missing context because I didn't get 1/3 of this patch set,
nor can I find it in my ML searching.  Nor did I get the cover letter,
or find it.  Is this series threaded?

What you are doing is changing a really horrible loop across all VMAs,
that happens 4 times, to a less interruptible method.

I'm not sure I'm okay with this.  Everyone else does seem fine with it,
because userspace basically never checks error codes for a retry (or
really anything, and sometimes not even for an error at all).

But this could potentially have larger consequences for those
applications that register signal handlers, couldn't it?  That is, they
expect to get a return based on some existing code but now it won't
return and the application is forced to wait for all locks to be taken
regardless of how long it takes - or to force kill the application?

We regularly get people requesting the default number of vmas be
increased.  This means that processes that approach max_map_count will
wait until 4 loops through the vmas before they can be interrupted, or
they have to kill the process.

> 
> For example, this bug happens when using OpenCL on AMDGPU. Sometimes,
> probing the OpenCL device fails (strace shows that open("/dev/kfd")
> failed with -EINTR). Sometimes we get the message "amdgpu:
> init_user_pages: Failed to register MMU notifier: -4" in the syslog.

If you only get the error message sometimes, does that mean there is
another signal check that isn't covered by this change - or another call
path?

> 
> The bug can be reproduced with the following program.
> 
> To run this program, you need AMD graphics card and the package
> "rocm-opencl" installed. You must not have the package "mesa-opencl-icd"
> installed, because it redirects the default OpenCL implementation to
> itself.

I'm not saying it's wrong to change the signal handling, but this is
very much working around a bug in userspace constantly hammering a task
with signals and then is surprised there is a response that the kernel
was interrupted.

This seems to imply that all signal handling should only happen on fatal
signals?

...
> 
> I'm submitting this patch for the stable kernels, because this bug may
> cause random failures in any code that calls mm_take_all_locks.

They aren't random failures, they are a response to a signal sent to the
process that may be taking a very long time to do something.

I really don't see how continuously sending signals at a short interval
interrupting system calls can be considered random failures, especially
when the return is -EINTR which literally means "Interrupted system
call".  How else would you interrupt a system call, if not a signal?

I feel like we are making the code less versatile to work around the
fact that userspace didn't realise that system calls could be
interrupted without a fatal signal.

And from that view, I consider this a functional change and not a bug
fix.

Thanks,
Liam