[PATCH] uinput: Fix module crash on driver exit with force feedback effects still loaded

[PATCH] uinput: Fix module crash on driver exit with force feedback effects still loaded

[Manuel Reimer]

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;

Re: [PATCH] uinput: Fix module crash on driver exit with force feedback effects still loaded

[Manuel Reimer]

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

Re: [PATCH] uinput: Fix module crash on driver exit with force feedback effects still loaded

[Dmitry Torokhov]

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