diff mbox series

stm class: Fix double add issue when store source_link

Message ID 20220418114658.6491-1-quic_jinlmao@quicinc.com
State New
Headers show
Series stm class: Fix double add issue when store source_link | expand

Commit Message

Mao Jinlong April 18, 2022, 11:46 a.m. UTC 
If two threads store the same stm device to stm_source_link
at the same time when stm->link_list is empty, it is possible
that stm_source_link_add will be called for both of these two
threads. Then double add issue below will happen. Add mutex
lock for stm_source_link drop and stm_source_link add to avoid
this race condition.

[ 12.386579][ T1024] list_add double add: new=ffffff87b73ebd90,
prev=ffffff87b73ebd90, next=ffffffc012737700.
[ 12.386657][ T1024] -----------[ cut here ]-----------
[ 12.386671][ T1024] kernel BUG at lib/list_debug.c:31!
[ 12.388845][ T1024] CPU: 2 PID: 1024 Comm: sh
[ 12.389162][ T1024] Call trace:
[ 12.389174][ T1024] __list_add_valid+0x68/0x98
[ 12.389199][ T1024] stm_source_link_store+0xcc/0x314 [stm_core]
[ 12.389213][ T1024] dev_attr_store+0x38/0x8c
[ 12.389228][ T1024] sysfs_kf_write+0xa0/0x100
[ 12.389239][ T1024] kernfs_fop_write_iter+0x1b0/0x2f8
[ 12.389253][ T1024] vfs_write+0x300/0x37c
[ 12.389264][ T1024] ksys_write+0x84/0x12c

Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
---
 drivers/hwtracing/stm/core.c | 7 ++++++-
 drivers/hwtracing/stm/stm.h  | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

Comments

Mao Jinlong May 16, 2022, 7:14 a.m. UTC | #1 
Hi Reviewers,

Could you please help to review this patch ?

Thanks

Jinlong Mao

On 4/18/2022 7:46 PM, Mao Jinlong wrote:
> If two threads store the same stm device to stm_source_link
> at the same time when stm->link_list is empty, it is possible
> that stm_source_link_add will be called for both of these two
> threads. Then double add issue below will happen. Add mutex
> lock for stm_source_link drop and stm_source_link add to avoid
> this race condition.
>
> [ 12.386579][ T1024] list_add double add: new=ffffff87b73ebd90,
> prev=ffffff87b73ebd90, next=ffffffc012737700.
> [ 12.386657][ T1024] -----------[ cut here ]-----------
> [ 12.386671][ T1024] kernel BUG at lib/list_debug.c:31!
> [ 12.388845][ T1024] CPU: 2 PID: 1024 Comm: sh
> [ 12.389162][ T1024] Call trace:
> [ 12.389174][ T1024] __list_add_valid+0x68/0x98
> [ 12.389199][ T1024] stm_source_link_store+0xcc/0x314 [stm_core]
> [ 12.389213][ T1024] dev_attr_store+0x38/0x8c
> [ 12.389228][ T1024] sysfs_kf_write+0xa0/0x100
> [ 12.389239][ T1024] kernfs_fop_write_iter+0x1b0/0x2f8
> [ 12.389253][ T1024] vfs_write+0x300/0x37c
> [ 12.389264][ T1024] ksys_write+0x84/0x12c
>
> Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
> ---
>   drivers/hwtracing/stm/core.c | 7 ++++++-
>   drivers/hwtracing/stm/stm.h  | 1 +
>   2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index 2712e699ba08..e73ac961acb2 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -1171,11 +1171,14 @@ static ssize_t stm_source_link_store(struct device *dev,
>   	struct stm_device *link;
>   	int err;
>   
> +	mutex_lock(&src->link_mutex);
>   	stm_source_link_drop(src);
>   
>   	link = stm_find_device(buf);
> -	if (!link)
> +	if (!link) {
> +		mutex_lock(&src->link_mutex);
>   		return -EINVAL;
> +	}
>   
>   	pm_runtime_get(&link->dev);
>   
> @@ -1185,6 +1188,7 @@ static ssize_t stm_source_link_store(struct device *dev,
>   		/* matches the stm_find_device() above */
>   		stm_put_device(link);
>   	}
> +	mutex_unlock(&src->link_mutex);
>   
>   	return err ? : count;
>   }
> @@ -1251,6 +1255,7 @@ int stm_source_register_device(struct device *parent,
>   
>   	stm_output_init(&src->output);
>   	spin_lock_init(&src->link_lock);
> +	mutex_init(&src->link_mutex);
>   	INIT_LIST_HEAD(&src->link_entry);
>   	src->data = data;
>   	data->src = src;
> diff --git a/drivers/hwtracing/stm/stm.h b/drivers/hwtracing/stm/stm.h
> index a9be49fc7a6b..60b814cc00e0 100644
> --- a/drivers/hwtracing/stm/stm.h
> +++ b/drivers/hwtracing/stm/stm.h
> @@ -79,6 +79,7 @@ void stm_put_device(struct stm_device *stm);
>   struct stm_source_device {
>   	struct device		dev;
>   	struct stm_source_data	*data;
> +	struct mutex		link_mutex;
>   	spinlock_t		link_lock;
>   	struct stm_device __rcu	*link;
>   	struct list_head	link_entry;
Mao Jinlong Nov. 2, 2022, 9:34 a.m. UTC | #2 
Hi all,

Please help to review this patch.

Thanks

Jinlong Mao

On 5/16/2022 3:14 PM, Jinlong Mao wrote:
> Hi Reviewers,
>
> Could you please help to review this patch ?
>
> Thanks
>
> Jinlong Mao
>
> On 4/18/2022 7:46 PM, Mao Jinlong wrote:
>> If two threads store the same stm device to stm_source_link
>> at the same time when stm->link_list is empty, it is possible
>> that stm_source_link_add will be called for both of these two
>> threads. Then double add issue below will happen. Add mutex
>> lock for stm_source_link drop and stm_source_link add to avoid
>> this race condition.
>>
>> [ 12.386579][ T1024] list_add double add: new=ffffff87b73ebd90,
>> prev=ffffff87b73ebd90, next=ffffffc012737700.
>> [ 12.386657][ T1024] -----------[ cut here ]-----------
>> [ 12.386671][ T1024] kernel BUG at lib/list_debug.c:31!
>> [ 12.388845][ T1024] CPU: 2 PID: 1024 Comm: sh
>> [ 12.389162][ T1024] Call trace:
>> [ 12.389174][ T1024] __list_add_valid+0x68/0x98
>> [ 12.389199][ T1024] stm_source_link_store+0xcc/0x314 [stm_core]
>> [ 12.389213][ T1024] dev_attr_store+0x38/0x8c
>> [ 12.389228][ T1024] sysfs_kf_write+0xa0/0x100
>> [ 12.389239][ T1024] kernfs_fop_write_iter+0x1b0/0x2f8
>> [ 12.389253][ T1024] vfs_write+0x300/0x37c
>> [ 12.389264][ T1024] ksys_write+0x84/0x12c
>>
>> Signed-off-by: Yuanfang Zhang <quic_yuanfang@quicinc.com>
>> Signed-off-by: Mao Jinlong <quic_jinlmao@quicinc.com>
>> ---
>>   drivers/hwtracing/stm/core.c | 7 ++++++-
>>   drivers/hwtracing/stm/stm.h  | 1 +
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
>> index 2712e699ba08..e73ac961acb2 100644
>> --- a/drivers/hwtracing/stm/core.c
>> +++ b/drivers/hwtracing/stm/core.c
>> @@ -1171,11 +1171,14 @@ static ssize_t stm_source_link_store(struct 
>> device *dev,
>>       struct stm_device *link;
>>       int err;
>>   +    mutex_lock(&src->link_mutex);
>>       stm_source_link_drop(src);
>>         link = stm_find_device(buf);
>> -    if (!link)
>> +    if (!link) {
>> +        mutex_lock(&src->link_mutex);
>>           return -EINVAL;
>> +    }
>>         pm_runtime_get(&link->dev);
>>   @@ -1185,6 +1188,7 @@ static ssize_t stm_source_link_store(struct 
>> device *dev,
>>           /* matches the stm_find_device() above */
>>           stm_put_device(link);
>>       }
>> +    mutex_unlock(&src->link_mutex);
>>         return err ? : count;
>>   }
>> @@ -1251,6 +1255,7 @@ int stm_source_register_device(struct device 
>> *parent,
>>         stm_output_init(&src->output);
>>       spin_lock_init(&src->link_lock);
>> +    mutex_init(&src->link_mutex);
>>       INIT_LIST_HEAD(&src->link_entry);
>>       src->data = data;
>>       data->src = src;
>> diff --git a/drivers/hwtracing/stm/stm.h b/drivers/hwtracing/stm/stm.h
>> index a9be49fc7a6b..60b814cc00e0 100644
>> --- a/drivers/hwtracing/stm/stm.h
>> +++ b/drivers/hwtracing/stm/stm.h
>> @@ -79,6 +79,7 @@ void stm_put_device(struct stm_device *stm);
>>   struct stm_source_device {
>>       struct device        dev;
>>       struct stm_source_data    *data;
>> +    struct mutex        link_mutex;
>>       spinlock_t        link_lock;
>>       struct stm_device __rcu    *link;
>>       struct list_head    link_entry;
diff mbox series

Patch

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 2712e699ba08..e73ac961acb2 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -1171,11 +1171,14 @@  static ssize_t stm_source_link_store(struct device *dev,
 	struct stm_device *link;
 	int err;
 
+	mutex_lock(&src->link_mutex);
 	stm_source_link_drop(src);
 
 	link = stm_find_device(buf);
-	if (!link)
+	if (!link) {
+		mutex_lock(&src->link_mutex);
 		return -EINVAL;
+	}
 
 	pm_runtime_get(&link->dev);
 
@@ -1185,6 +1188,7 @@  static ssize_t stm_source_link_store(struct device *dev,
 		/* matches the stm_find_device() above */
 		stm_put_device(link);
 	}
+	mutex_unlock(&src->link_mutex);
 
 	return err ? : count;
 }
@@ -1251,6 +1255,7 @@  int stm_source_register_device(struct device *parent,
 
 	stm_output_init(&src->output);
 	spin_lock_init(&src->link_lock);
+	mutex_init(&src->link_mutex);
 	INIT_LIST_HEAD(&src->link_entry);
 	src->data = data;
 	data->src = src;
diff --git a/drivers/hwtracing/stm/stm.h b/drivers/hwtracing/stm/stm.h
index a9be49fc7a6b..60b814cc00e0 100644
--- a/drivers/hwtracing/stm/stm.h
+++ b/drivers/hwtracing/stm/stm.h
@@ -79,6 +79,7 @@  void stm_put_device(struct stm_device *stm);
 struct stm_source_device {
 	struct device		dev;
 	struct stm_source_data	*data;
+	struct mutex		link_mutex;
 	spinlock_t		link_lock;
 	struct stm_device __rcu	*link;
 	struct list_head	link_entry;