Hi Dmitry,
On 12/8/23 11:32, Dmitry Torokhov wrote:
Hi Vicki,
On Wed, Dec 06, 2023 at 10:34:05PM -0800, Vicki Pfau wrote:
Currently, uinput_request_submit will only fail if the request wait times out.
However, in other places this wait is interruptable, and in this specific
location it can lead to issues, such as causing system suspend to hang until
the request times out.
Could you please explain how a sleeping process can cause suspend to
hang?
While I'm not 100% sure how it happens, given I found this by reproducing it before I came up with a theory for why it happened, my guess is that as it's trying to suspend all of userspace programs, it suspends the process that owns the uinput handle, so it can't continue to service requests, while the other process hangs in the uninterruptable call, blocking suspend for 30 seconds until the call times out.
Since the timeout is so long, this can cause the
appearance of a total system freeze. Making the wait interruptable resolves
this and possibly further issues.
I think you are trying to find a justification too hard and it does not
make sense, however I agree that allowing to kill the process issuing
the request without waiting for the timeout to expire if the other side
is stuck might be desirable.
This isn't reaching. As I said above, I discovered the patched line of code *after* observing suspend hanging for 30 seconds while trying to reproduce another bug. I wrote this patch, retested, and found that it now suspended immediately, leading to a visible -ERESTARTSYS in strace on coming back from suspend.
I can post the reproduction case somewhere, but the test program is only the evdev client end, with the uinput side being Steam, which I don't have source code for.
I think the best way to use wait_for_completion_killable_timeout()
so that stray signals do not disturb userspace, but the processes can
still be terminated.
There's already a mutex_lock_interruptable in uinput_request_send that could cause this to fall back to userspace under similar circumstances. The only difference I can find, which is admittedly a bug in this patch now that I look at it again, is that uinput_dev_event would get called twice, leading to the request getting duplicated.
