Re: [PATCH 5/5] Input: goodix - Fix race on driver unbind

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

Hi Hans,

On Sun, Dec 12, 2021 at 01:42:42PM +0100, Hans de Goede wrote:
> Because there is no way to detect if the touchscreen has pen support,
> the driver is allocating and registering the input_pen input_dev on
> receiving the first pen event.
> 
> But this means that the input_dev gets allocated after the request_irq()
> call which means that the devm framework will free it before disabling
> the irq, leaving a window where the irq handler may run and reference the
> free-ed input_dev.
> 
> To fix this move the allocation of the input_pen input_dev to before
> the request_irq() call, while still only registering it on the first pen
> event so that the driver does not advertise pen capability on touchscreens
> without it (most goodix touchscreens do not have pen support).
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/input/touchscreen/goodix.c | 32 ++++++++++++++++++------------
>  drivers/input/touchscreen/goodix.h |  1 +
>  2 files changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index 04baf5a770f5..1ada40fa6de6 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -297,14 +297,14 @@ static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
>  	return -ENOMSG;
>  }
>  
> -static struct input_dev *goodix_create_pen_input(struct goodix_ts_data *ts)
> +static int goodix_create_pen_input(struct goodix_ts_data *ts)
>  {
>  	struct device *dev = &ts->client->dev;
>  	struct input_dev *input;
>  
>  	input = devm_input_allocate_device(dev);
>  	if (!input)
> -		return NULL;
> +		return -ENOMEM;
>  
>  	input_copy_abs(input, ABS_X, ts->input_dev, ABS_MT_POSITION_X);
>  	input_copy_abs(input, ABS_Y, ts->input_dev, ABS_MT_POSITION_Y);
> @@ -331,23 +331,18 @@ static struct input_dev *goodix_create_pen_input(struct goodix_ts_data *ts)
>  		input->id.product = 0x1001;
>  	input->id.version = ts->version;
>  
> -	if (input_register_device(input) != 0) {
> -		input_free_device(input);
> -		return NULL;
> -	}
> -
> -	return input;
> +	ts->input_pen = input;
> +	return 0;
>  }
>  
>  static void goodix_ts_report_pen_down(struct goodix_ts_data *ts, u8 *data)
>  {
> -	int input_x, input_y, input_w;
> +	int input_x, input_y, input_w, error;
>  	u8 key_value;
>  
> -	if (!ts->input_pen) {
> -		ts->input_pen = goodix_create_pen_input(ts);
> -		if (!ts->input_pen)
> -			return;
> +	if (!ts->pen_input_registered) {
> +		error = input_register_device(ts->input_pen);
> +		ts->pen_input_registered = error == 0;

I do not think you want to try and re-register input device if first
registration failed. You probably also don't want to waste time and
return early from here in case of failures.

Thanks.

-- 
Dmitry