Re: [PATCH 1/2] Input: uinput - Allow uinput_request_submit wait interrupting

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Vicki,

On Thu, Dec 14, 2023 at 07:04:09PM -0800, Vicki Pfau wrote:
> Hi Dmitry
> 
> On 12/8/23 19:24, Vicki Pfau wrote:
> > 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 must apologize, you indeed weren't reaching. As far as I can see,
putting tasks into the freezer (which happens during system suspend) is
done via delivering a fake signal to the task. So the task indeed needs
to be in an interruptible state, uninterruptible tasks result in system
failing to 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.
> 
> After further investigation, it seems this would still be the case
> even if the request times out--an invalid request would get left in
> the buffer, which means that while this is a new way to trigger the
> issue, it's not actually a new issue.

No, I disagree that it is the same issue. The timeout condition is
pretty much fatal, I expect the caller to exit or stop using the device
if request times out (because either the real device is not responding,
or userspace is not responding, and there is no indication that they
will start responding any time soon). That is why the timeout value is
so generous (30 seconds). In this case we definitely not expect the
request to be re-submitted, either automatically, or explicitly by
userspace.

If we make waiting on the request interruptible we may get interrupted
by a stray signal, and I do not know how both producer (the process
issuing the uinput request) and consumer of the request, will react to
essentially duplicate requests being sent.

I believe we can split this into 2 separate issues:

1. The fact that it is not possible terminate the producer process while
it is waiting for request to be handled (for 30 seconds). I think this
can be safely resolved by switching to
wait_for_completion_killable_timeout(). This will allow fatal signals to
break the wait, and for the process to exit.

2. Producer task failing to enter refrigerator and breaking suspend.
I wonder if the best way to handle that is for uinput to create and
register wakeup source, and then use __pm_stay_awake() and __pm_relax()
to indicate to the rest of the system that suspend is blocked. I believe
userspace should be able to handle this and repeat suspend attempt when
the condition clears...

Rafael, do you have any suggestions here? And I wonder, could we make
killable tasks also enter refrigerator?


Also, now that I think about it more, we should not use slot number for
request->id, as I expect the common situation is to have 1 outstanding
request, so all requests have id 0. Instead we should have a counter and
increase it. This way timeout handling will be more robust, we will not
mistake delayed response to the previous request as response to the
current one.

Thanks.

-- 
Dmitry