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

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

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