| Message ID | 20220418114658.6491-1-quic_jinlmao@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | stm class: Fix double add issue when store source_link | expand |
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;
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 --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;