Re: [PATCH] uinput: Fix module crash on driver exit with force feedback effects still loaded
flat view
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2016-06-25 15:44:09
On Sun, May 22, 2016 at 08:22:23PM +0200, Manuel Reimer wrote:
Hello, I had a deeper look into the uinput kernel module crash problem, which happens when a game loads effects and the driver exits while the game is still running. The actual problem is, that "input_unregister_device", which is called in "uinput_destroy_device", seems to include some "cleanup routine" which includes erasing loaded force feedback effects. This cleanup routine causes the "uinput_dev_erase_effect" routine to be called, which will wait for the, already exited, daemon to answer this request.
There are 2 cases when we destroy the device: - when daemon is exiting. In this case you are absolutely right, we have no hope of talking to the device and should not be trying to send requests to it; - when userspace explicitly requested the device to be destroyed via UI_DEV_DESTROY ioctl. In this case I believe the kernel should attempt orderly device shutdown which includes stopping all FF effects. I guess we'll need to introduce another bit of state and adjust behavior accordingly.
The existing code contains a device state test, but this test is placed wrong. My patch moves the device state test, which was in "uinput_request_send", to the "uinput_request_submit" function. This
The device state is supposed to be protected by udev->mutex, your change violates this constraint.
quoted hunk ↗ jump to hunk
way the erase requests, triggered by "input_unregister_device", are rejected immediately, so they no longer cause any trouble. Should I have added a one-line comment above the device state test, mentioning the cleanup routine in "input_unregister_device"? Signed-off-by: Manuel Reimer <redacted>--- a/drivers/input/misc/uinput.c 2016-05-13 16:06:31.919351656 +0200 +++ b/drivers/input/misc/uinput.c 2016-05-22 19:33:30.814403482 +0200@@ -117,11 +117,6 @@ static int uinput_request_send(struct ui if (retval) return retval; - if (udev->state != UIST_CREATED) { - retval = -ENODEV; - goto out; - } - init_completion(&request->done); /*@@ -130,7 +125,6 @@ static int uinput_request_send(struct ui */ uinput_dev_event(udev->dev, EV_UINPUT, request->code, request->id); - out: mutex_unlock(&udev->mutex); return retval; }@@ -140,6 +134,9 @@ static int uinput_request_submit(struct { int error; + if (udev->state != UIST_CREATED) + return -ENODEV; + error = uinput_request_reserve_slot(udev, request); if (error) return error;
Thanks. -- Dmitry