Re: [PATCH] hid: logitech: check the return value of create_singlethread_workqueue

[Benjamin Tissoires]

On Sat, Mar 9, 2019 at 5:43 AM Kangjie Lu <kjlu@xxxxxxx> wrote:
>
> create_singlethread_workqueue may fail and return NULL. The fix
> checks if it is NULL to avoid NULL pointer dereference.
>
> Signed-off-by: Kangjie Lu <kjlu@xxxxxxx>
> ---
>  drivers/hid/hid-logitech-hidpp.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 15ed6177a7a3..efbc39b92aa2 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -2156,6 +2156,9 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
>
>         /* init the hardware command queue */
>         data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
> +       if (!data->wq)
> +               return -ENOMEM;
> +

I agree there is something wrong with the current code, but I think
you are not fixing the issue properly.
The code just above allocates data with kzalloc and stores it in the
hid_device and set up everything. If create_singlethread_workqueue()
fails now, then we are screwed and you need to reverse all of the FF
init.

I think we should move the thread creation above so we are not leaking
memory and not create the FF elements if there is something wrong.

Cheers,
Benjamin

>         atomic_set(&data->workqueue_size, 0);
>
>         /* initialize with zero autocenter to get wheel in usable state */
> --
> 2.17.1
>