linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] atmel: fix an error handle in mxt_probe
@ 2015-04-22 10:46 Pan Xinhui
  2015-05-13 17:41 ` Dmitry Torokhov
  0 siblings, 1 reply; 3+ messages in thread
From: Pan Xinhui @ 2015-04-22 10:46 UTC (permalink / raw)
  To: linux-kernel
  Cc: dmitry.torokhov, nick.dyer, miletus, bleung, djkurtz, mnipxh,
	yanmin_zhang

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@intel.com>
---
  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;
+
  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

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] atmel: fix an error handle in mxt_probe
  2015-04-22 10:46 [PATCH] atmel: fix an error handle in mxt_probe Pan Xinhui
@ 2015-05-13 17:41 ` Dmitry Torokhov
  2015-05-15  2:42   ` Pan Xinhui
  0 siblings, 1 reply; 3+ messages in thread
From: Dmitry Torokhov @ 2015-05-13 17:41 UTC (permalink / raw)
  To: Pan Xinhui
  Cc: linux-kernel, nick.dyer, miletus, bleung, djkurtz, mnipxh, yanmin_zhang

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@intel.com>
> ---
>  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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] atmel: fix an error handle in mxt_probe
  2015-05-13 17:41 ` Dmitry Torokhov
@ 2015-05-15  2:42   ` Pan Xinhui
  0 siblings, 0 replies; 3+ messages in thread
From: Pan Xinhui @ 2015-05-15  2:42 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: linux-kernel, nick.dyer, miletus, bleung, djkurtz, mnipxh, yanmin_zhang

HI, Dmitry
	thanks for your reply :)

On 2015年05月14日 01:41, Dmitry Torokhov wrote:
> 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@intel.com>
>> ---
>>   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.
>

yes, statics is not good.
It's also a good point that do some work both in err patch and in_remove().

thanks for your nice advices. I will work out patch V2.

thanks

xinhui

>> +
>>   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
>

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-05-14  2:44 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-22 10:46 [PATCH] atmel: fix an error handle in mxt_probe Pan Xinhui
2015-05-13 17:41 ` Dmitry Torokhov
2015-05-15  2:42   ` Pan Xinhui

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).