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 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. It seems to me that this driver could use a lot of love to get it into better shape, which I could work on, but I'm not actually sure where to begin. Especially if we don't want to break ABI. > > If there's a better way to handle the suspend case let me know, but this is not a hypothetical issue. > >> >> Thanks. >> > > Vicki Vici
