* [PATCH] uinput: Fix module crash on driver exit with force feedback effects still loaded
@ 2016-05-22 18:22 Manuel Reimer
2016-06-05 13:01 ` Manuel Reimer
2016-06-25 15:44 ` Dmitry Torokhov
0 siblings, 2 replies; 3+ messages in thread
From: Manuel Reimer @ 2016-05-22 18:22 UTC (permalink / raw)
To: linux-input, dmitry.torokhov
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.
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 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 <mail@m-reimer.de>
--- 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;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] uinput: Fix module crash on driver exit with force feedback effects still loaded
2016-05-22 18:22 [PATCH] uinput: Fix module crash on driver exit with force feedback effects still loaded Manuel Reimer
@ 2016-06-05 13:01 ` Manuel Reimer
2016-06-25 15:44 ` Dmitry Torokhov
1 sibling, 0 replies; 3+ messages in thread
From: Manuel Reimer @ 2016-06-05 13:01 UTC (permalink / raw)
To: linux-input, dmitry.torokhov
On 05/22/2016 08:22 PM, 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.
>
> 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 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 <mail@m-reimer.de>
>
> --- 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;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello,
did I use the correct list of recipients? This is my first try to fix
something in kernel code, so it would really help me to get some
suggestions about the right way to get something reviewed.
Thanks in advance
Manuel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] uinput: Fix module crash on driver exit with force feedback effects still loaded
2016-05-22 18:22 [PATCH] uinput: Fix module crash on driver exit with force feedback effects still loaded Manuel Reimer
2016-06-05 13:01 ` Manuel Reimer
@ 2016-06-25 15:44 ` Dmitry Torokhov
1 sibling, 0 replies; 3+ messages in thread
From: Dmitry Torokhov @ 2016-06-25 15:44 UTC (permalink / raw)
To: Manuel Reimer; +Cc: linux-input
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.
>
> 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 <mail@m-reimer.de>
>
> --- 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-06-25 15:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-22 18:22 [PATCH] uinput: Fix module crash on driver exit with force feedback effects still loaded Manuel Reimer
2016-06-05 13:01 ` Manuel Reimer
2016-06-25 15:44 ` Dmitry Torokhov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.