linux-kernel - Re: [PATCH v10 54/55] Input: atmel_mxt_ts: Implement synchronization during various operation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <271b1013-63e4-cd05-f859-07032c0e8a85@mentor.com>
Date:   Fri, 3 Apr 2020 17:39:07 +0900
From:   "Wang, Jiada" <jiada_wang@...tor.com>
To:     Dmitry Osipenko <digetx@...il.com>, <nick@...anahar.org>,
        <dmitry.torokhov@...il.com>, <jikos@...nel.org>,
        <benjamin.tissoires@...hat.com>, <bsz@...ihalf.com>
CC:     <linux-input@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <erosca@...adit-jv.com>, <Andrew_Gabbasov@...tor.com>
Subject: Re: [PATCH v10 54/55] Input: atmel_mxt_ts: Implement synchronization
 during various operation

Hi Dmitry

On 2020/04/02 22:24, Dmitry Osipenko wrote:
> 02.04.2020 14:50, Wang, Jiada пишет:
>> Hi Dmitry
>>
>> On 2020/04/02 1:04, Dmitry Osipenko wrote:
>>> 31.03.2020 13:50, Jiada Wang пишет:
>>>> From: Sanjeev Chugh <sanjeev_chugh@...tor.com>
>>>>
>>>> There could be scope of race conditions when sysfs is being handled
>>>> and at the same time, device removal is occurring. For example,
>>>> we don't want the device removal to begin if the Atmel device
>>>> cfg update is going on or firmware update is going on. In such
>>>> cases, wait for device update to be completed before the removal
>>>> continues.
>>>>
>>>>       Thread                                          Thread 2:
>>>> =========================
>>>> =========================
>>>> mxt_update_fw_store()                           mxt_remove()
>>>> mutex_lock(&data->lock)                         ...
>>>> mxt_initialize()                                //Tries to acquire lock
>>>>     request_firmware_nowait()                     mutex_lock(&data->lock)
>>>> ...                                             ==>waits for lock()
>>>> ...                                             .
>>>> ...                                             .
>>>> mutex_unlock(&data->lock)                       .
>>>>                                                   //Gets lock and
>>>> proceeds
>>>>                                                  
>>>> mxt_free_input_device();
>>>>                                                   ...
>>>>                                                  
>>>> mutex_unlock(&data->lock)
>>>>                                                   //Frees atmel driver
>>>> data
>>>>                                                   kfree(data)
>>>>
>>>> If the request_firmware_nowait() completes after the driver removal,
>>>> and callback is triggered. But kernel crashes since the module is
>>>> already removed.
>>>>
>>>> This commit adds state machine to serialize such scenarios.
>>>
>>> Won't it be easier to bump driver's module use-count by __module_get()
>>> while firmware is updating? Or remove sysfs during of mxt_remove()? >
>>
>> thanks for your inspiration, I will replace state machine with module
>> use-count.
> 
> I'm actually now thinking that the suggestion about the module-count
> wasn't very correct because this won't really help in regards to
> mxt_update_fw_store() / mxt_remove() racing.
> 
> I see that mxt_remove() already invokes the mxt_sysfs_remove(), which
> should block until mxt_update_fw_store() is completed, shouldn't it?

Yes, you are correct,
this commit isn't addressing the real root cause

> 
> I guess the kfree(data) isn't the real cause of the problem and
> something like this should help:
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c
> b/drivers/input/touchscreen/atmel_mxt_ts.c
> index b2edf51e1595..4e66106feeb9 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -4254,6 +4254,7 @@ static void mxt_sysfs_remove(struct mxt_data *data)
>   	struct i2c_client *client = data->client;
> 
>   	sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> +	sysfs_remove_group(&client->dev.kobj, &mxt_fw_attr_group);
>   }
> 
>   static void mxt_reset_slots(struct mxt_data *data)
> @@ -4649,31 +4650,19 @@ static int mxt_remove(struct i2c_client *client)
>   {
>   	struct mxt_data *data = i2c_get_clientdata(client);
> 
> -	mutex_lock(&data->lock);
> -	if (data->e_state == MXT_STATE_UPDATING_CONFIG_ASYNC ||
> -	    data->e_state == MXT_STATE_UPDATING_CONFIG) {
> -		data->e_state = MXT_STATE_GOING_AWAY;
> -		mutex_unlock(&data->lock);
> -		mxt_wait_for_completion(data, &data->update_cfg_completion,
> -					MXT_CONFIG_TIMEOUT);
> -	} else {
> -		data->e_state = MXT_STATE_GOING_AWAY;
> -		mutex_unlock(&data->lock);
> -	}
> +	mxt_sysfs_remove(data);
> 
> -	disable_irq(data->irq);
> -	sysfs_remove_group(&client->dev.kobj, &mxt_fw_attr_group);
>   	if (data->reset_gpio) {
>   		sysfs_remove_link(&client->dev.kobj, "reset");
>   		gpiod_unexport(data->reset_gpio);
>   	}
> +
>   	mxt_debug_msg_remove(data);
> -	mxt_sysfs_remove(data);
>   	mxt_free_input_device(data);
>   	mxt_free_object_table(data);
> 
> 	if (debug_state)
> 		cancel_delayed_work_sync(&data->watchdog_work);
> +	disable_irq(data->irq);
> 
>   	return 0;
>   }
> 
yes, I confirmed, the root cause is because irq is disabled while 
firmware is being updated, this cause update of firmware can't proceed.
by move disable irq after sysfs entry removal can fix the issue

Thanks,
Jiada