Re: [PATCH] stm class: Fix double add issue when store source_link

[Jinlong Mao]

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@xxxxxxxxxxx>
> > Signed-off-by: Mao Jinlong <quic_jinlmao@xxxxxxxxxxx>
> > ---
> >   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;