2015-05-14 11:21:05

by Pan Xinhui

Subject: [PATCH V2] atmel: fix a race between fw_load and data free

mxt_probe() may fail at last step, or we jsut unload mxt module soon.
the queue_work scheduled by request_firmware_nowait may run later,
and then access some data which is freed.
To handle this issue, add fw_load_completion field in mxt_data.
then we wait for it complete both in probe error path and mxt_remove().

here is the detail in probe, similar in remove.

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: xinhui.pan <[email protected]>
---
change in V2:
use fw_load_completion instead of statics.
fix a race both in mxt_remove and mxt_probe.

drivers/input/touchscreen/atmel_mxt_ts.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 40b98dd..3da040d 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -313,6 +313,9 @@ struct mxt_data {

/* for config update handling */
struct completion crc_completion;
+
+ /* for fw load handling */
+ struct completion fw_load_completion;
};

static size_t mxt_obj_size(const struct mxt_object *obj)
@@ -1982,8 +1985,10 @@ static int mxt_configure_objects(struct mxt_data *data,

static void mxt_config_cb(const struct firmware *cfg, void *ctx)
{
+ struct mxt_data *data = ctx;
mxt_configure_objects(ctx, cfg);
release_firmware(cfg);
+ complete(&data->fw_load_completion);
}

static int mxt_initialize(struct mxt_data *data)
@@ -2556,6 +2561,7 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
init_completion(&data->bl_completion);
init_completion(&data->reset_completion);
init_completion(&data->crc_completion);
+ init_completion(&data->fw_load_completion);

error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
pdata->irqflags | IRQF_ONESHOT,
@@ -2581,6 +2587,8 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id)
return 0;

err_free_object:
+ mxt_wait_for_completion(data, &data->fw_load_completion,
+ MXT_FW_RESET_TIME);
mxt_free_input_device(data);
mxt_free_object_table(data);
err_free_irq:
@@ -2594,6 +2602,8 @@ static int mxt_remove(struct i2c_client *client)
{
struct mxt_data *data = i2c_get_clientdata(client);

+ mxt_wait_for_completion(data, &data->fw_load_completion,
+ MXT_FW_RESET_TIME);
sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
free_irq(data->irq, data);
mxt_free_input_device(data);
--
1.9.1

2015-05-14 11:31:24

by Nick Dyer

Subject: Re: [PATCH V2] atmel: fix a race between fw_load and data free

Hi-

This looks like a good tidy up, one minor comment:

On 15/05/15 12:19, Pan Xinhui wrote:
> + mxt_wait_for_completion(data, &data->fw_load_completion,
> + MXT_FW_RESET_TIME);

this should probably just be

wait_for_completion(&data->fw_load_completion);

rather than timing out (the MXT_FW_RESET_TIME may not be an appropriate
length anyway)

cheers

Nick

2015-05-14 12:39:09

by Pan Xinhui

Subject: Re: [PATCH V2] atmel: fix a race between fw_load and data free

hi,
yes, we can't guarantee MXT_FW_RESET_TIME is enough. patch v3 is sent out :)
thanks
xinhui

On 2015年05月14日 19:31, Nick Dyer wrote:
> Hi-
>
> This looks like a good tidy up, one minor comment:
>
> On 15/05/15 12:19, Pan Xinhui wrote:
>> + mxt_wait_for_completion(data, &data->fw_load_completion,
>> + MXT_FW_RESET_TIME);
>
> this should probably just be
>
> wait_for_completion(&data->fw_load_completion);
>
> rather than timing out (the MXT_FW_RESET_TIME may not be an appropriate
> length anyway)
>
> cheers
>
> Nick
>