Re: [PATCH] atmel: fix an error handle in mxt_probe

[Dmitry Torokhov]

Hi,

On Wed, Apr 22, 2015 at 06:46:58PM +0800, Pan Xinhui wrote:
> mxt_probe() may fail at last step, and the queue_work scheduled by request_firmware_nowait
> may run later and then access some data which is freed.
> To handle this error, add one mutex_lock to cover such case. It may cause module load delay only when the probe fails.
> 
> here is the detail.
> 
> module load:                                                             worker_thread:
> mxt_probe -> mxt_initialize -> request_firmware_nowait (schedule_work)
>                    |
>             sysfs_create_group (fails)                                    mxt_config_cb -> mxt_configure_objects (may access data freed)
>                    |
>             err_free_object: some cleanup work, like free(data).
> 
> Signed-off-by: xinhuix.pan <xinhuix.pan@xxxxxxxxx>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 2875ddf..af057c0 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -1978,10 +1978,19 @@ err_free_mem:
>  static int mxt_configure_objects(struct mxt_data *data,
>  				 const struct firmware *cfg);
> +static DEFINE_MUTEX(err_probe_lock);
> +static int err_probe;

While you are right that bad things will happen if we let
request_firmware_nowait() run after driver fails to bind to the device
using statics to indicate success or failure is not good idea since you
may have several such devices in your unit. Also it still doe snot help
if you decide to unbind the device quickly or unlock the module.

I guess the best way is to signal a completion from callback and wait
for it in error path and in remove().

Thanks.

> +
>  static void mxt_config_cb(const struct firmware *cfg, void *ctx)
>  {
> +	mutex_lock(&err_probe_lock);
> +	if (err_probe) {
> +		mutex_unlock(&err_probe_lock);
> +		return;
> +	}
>  	mxt_configure_objects(ctx, cfg);
>  	release_firmware(cfg);
> +	mutex_unlock(&err_probe_lock);
>  }
>  static int mxt_initialize(struct mxt_data *data)
> @@ -2423,6 +2432,8 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	const struct mxt_platform_data *pdata;
>  	int error;
> +	err_probe = 0;
> +
>  	pdata = dev_get_platdata(&client->dev);
>  	if (!pdata) {
>  		pdata = mxt_parse_dt(client);
> @@ -2472,6 +2483,9 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	return 0;
>  err_free_object:
> +	mutex_lock(&err_probe_lock);
> +	err_probe = -1;
> +	mutex_unlock(&err_probe_lock);
>  	mxt_free_input_device(data);
>  	mxt_free_object_table(data);
>  err_free_irq:
> -- 
> 1.9.1

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/