[PATCH v4] coresight: prevent deactivate active config while enabling the config

[PATCH v4] coresight: prevent deactivate active config while enabling the config

[Yeoreum Yun]

While enable active config via cscfg_csdev_enable_active_config(),
active config could be deactivated via configfs' sysfs interface.
This could make UAF issue in below scenario:

CPU0                                          CPU1
(sysfs enable)                                load module
                                              cscfg_load_config_sets()
                                              activate config. // sysfs
                                              (sys_active_cnt == 1)
...
cscfg_csdev_enable_active_config()
  lock(csdev->cscfg_csdev_lock)
  // here load config activate by CPU1
  unlock(csdev->cscfg_csdev_lock)

                                              deactivate config // sysfs
                                              (sys_activec_cnt == 0)
                                              cscfg_unload_config_sets()
                                              unload module

  // access to config_desc which freed
  // while unloading module.
  cfs_csdev_enable_config

To address this, use cscfg_config_desc's active_cnt as a reference count
which will be holded when
    - activate the config.
    - enable the activated config.
and put the module reference when config_active_cnt == 0.

Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
Since v3:
  - Remove enable arguments in cscfg_config_desc_get() (from Mike).
  - https://lore.kernel.org/all/20250109171956.3535294-1-yeoreum.yun@arm.com/
---
 .../hwtracing/coresight/coresight-config.h    |  2 +-
 .../coresight/coresight-etm4x-core.c          |  3 ++
 .../hwtracing/coresight/coresight-syscfg.c    | 52 +++++++++++++------
 3 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
index b9ebc9fcfb7f..90fd937d3bd8 100644
--- a/drivers/hwtracing/coresight/coresight-config.h
+++ b/drivers/hwtracing/coresight/coresight-config.h
@@ -228,7 +228,7 @@ struct cscfg_feature_csdev {
  * @feats_csdev:references to the device features to enable.
  */
 struct cscfg_config_csdev {
-	const struct cscfg_config_desc *config_desc;
+	struct cscfg_config_desc *config_desc;
 	struct coresight_device *csdev;
 	bool enabled;
 	struct list_head node;
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index e5972f16abff..ef96028fa56b 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1020,6 +1020,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
 	smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);

 	raw_spin_unlock(&drvdata->spinlock);
+
+	cscfg_csdev_disable_active_config(csdev);
+
 	cpus_read_unlock();

 	/*
diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
index a70c1454b410..6d8c212ad434 100644
--- a/drivers/hwtracing/coresight/coresight-syscfg.c
+++ b/drivers/hwtracing/coresight/coresight-syscfg.c
@@ -391,14 +391,17 @@ static void cscfg_owner_put(struct cscfg_load_owner_info *owner_info)
 static void cscfg_remove_owned_csdev_configs(struct coresight_device *csdev, void *load_owner)
 {
 	struct cscfg_config_csdev *config_csdev, *tmp;
+	unsigned long flags;

 	if (list_empty(&csdev->config_csdev_list))
 		return;

+	raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
 	list_for_each_entry_safe(config_csdev, tmp, &csdev->config_csdev_list, node) {
 		if (config_csdev->config_desc->load_owner == load_owner)
 			list_del(&config_csdev->node);
 	}
+	raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
 }

 static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
@@ -867,6 +870,25 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
 }
 EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);

+static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc)
+{
+	if (!atomic_fetch_inc(&config_desc->active_cnt)) {
+		/* must ensure that config cannot be unloaded in use */
+		if (unlikely(cscfg_owner_get(config_desc->load_owner))) {
+			atomic_dec(&config_desc->active_cnt);
+			return false;
+		}
+	}
+
+	return true;
+}
+
+static void cscfg_config_desc_put(struct cscfg_config_desc *config_desc)
+{
+	if (!atomic_dec_return(&config_desc->active_cnt))
+		cscfg_owner_put(config_desc->load_owner);
+}
+
 /*
  * This activate configuration for either perf or sysfs. Perf can have multiple
  * active configs, selected per event, sysfs is limited to one.
@@ -890,22 +912,17 @@ static int _cscfg_activate_config(unsigned long cfg_hash)
 			if (config_desc->available == false)
 				return -EBUSY;

-			/* must ensure that config cannot be unloaded in use */
-			err = cscfg_owner_get(config_desc->load_owner);
-			if (err)
+			if (!cscfg_config_desc_get(config_desc)) {
+				err = -EINVAL;
 				break;
+			}
+
 			/*
 			 * increment the global active count - control changes to
 			 * active configurations
 			 */
 			atomic_inc(&cscfg_mgr->sys_active_cnt);

-			/*
-			 * mark the descriptor as active so enable config on a
-			 * device instance will use it
-			 */
-			atomic_inc(&config_desc->active_cnt);
-
 			err = 0;
 			dev_dbg(cscfg_device(), "Activate config %s.\n", config_desc->name);
 			break;
@@ -920,9 +937,8 @@ static void _cscfg_deactivate_config(unsigned long cfg_hash)

 	list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
 		if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
-			atomic_dec(&config_desc->active_cnt);
 			atomic_dec(&cscfg_mgr->sys_active_cnt);
-			cscfg_owner_put(config_desc->load_owner);
+			cscfg_config_desc_put(config_desc);
 			dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
 			break;
 		}
@@ -1047,7 +1063,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
 				     unsigned long cfg_hash, int preset)
 {
 	struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
-	const struct cscfg_config_desc *config_desc;
+	struct cscfg_config_desc *config_desc;
 	unsigned long flags;
 	int err = 0;

@@ -1062,8 +1078,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
 	raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
 	list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
 		config_desc = config_csdev_item->config_desc;
-		if ((atomic_read(&config_desc->active_cnt)) &&
-		    ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
+		if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
+				cscfg_config_desc_get(config_desc)) {
 			config_csdev_active = config_csdev_item;
 			csdev->active_cscfg_ctxt = (void *)config_csdev_active;
 			break;
@@ -1097,7 +1113,11 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
 				err = -EBUSY;
 			raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
 		}
+
+		if (err)
+			cscfg_config_desc_put(config_desc);
 	}
+
 	return err;
 }
 EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
@@ -1136,8 +1156,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev)
 	raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);

 	/* true if there was an enabled active config */
-	if (config_csdev)
+	if (config_csdev) {
 		cscfg_csdev_disable_config(config_csdev);
+		cscfg_config_desc_put(config_csdev->config_desc);
+	}
 }
 EXPORT_SYMBOL_GPL(cscfg_csdev_disable_active_config);

--
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

Re: [PATCH v4] coresight: prevent deactivate active config while enabling the config

[Leo Yan]

Hi Levi,

On Mon, Mar 24, 2025 at 07:17:40PM +0000, Yeoreum Yun wrote:
> While enable active config via cscfg_csdev_enable_active_config(),
> active config could be deactivated via configfs' sysfs interface.
> This could make UAF issue in below scenario:
> 
> CPU0                                          CPU1
> (sysfs enable)                                load module
>                                               cscfg_load_config_sets()
>                                               activate config. // sysfs
>                                               (sys_active_cnt == 1)
> ...
> cscfg_csdev_enable_active_config()
>   lock(csdev->cscfg_csdev_lock)
>   // here load config activate by CPU1
>   unlock(csdev->cscfg_csdev_lock)
> 
>                                               deactivate config // sysfs
>                                               (sys_activec_cnt == 0)
>                                               cscfg_unload_config_sets()
>                                               unload module
> 
>   // access to config_desc which freed
>   // while unloading module.
>   cfs_csdev_enable_config

I am not sure if this flow can happen.  CoreSight configfs feature is
integrated into the CoreSight core layer, and the other CoreSight
modules are dependent on it.

For example, if the ETM4x module is not removed, the kernel module
management will natually prevent the CoreSight core module from being
removed.

> To address this, use cscfg_config_desc's active_cnt as a reference count
> which will be holded when
>     - activate the config.
>     - enable the activated config.
> and put the module reference when config_active_cnt == 0.
> 
> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> ---
> Since v3:
>   - Remove enable arguments in cscfg_config_desc_get() (from Mike).
>   - https://lore.kernel.org/all/20250109171956.3535294-1-yeoreum.yun@arm.com/
> ---
>  .../hwtracing/coresight/coresight-config.h    |  2 +-
>  .../coresight/coresight-etm4x-core.c          |  3 ++
>  .../hwtracing/coresight/coresight-syscfg.c    | 52 +++++++++++++------
>  3 files changed, 41 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> index b9ebc9fcfb7f..90fd937d3bd8 100644
> --- a/drivers/hwtracing/coresight/coresight-config.h
> +++ b/drivers/hwtracing/coresight/coresight-config.h
> @@ -228,7 +228,7 @@ struct cscfg_feature_csdev {
>   * @feats_csdev:references to the device features to enable.
>   */
>  struct cscfg_config_csdev {
> -	const struct cscfg_config_desc *config_desc;
> +	struct cscfg_config_desc *config_desc;
>  	struct coresight_device *csdev;
>  	bool enabled;
>  	struct list_head node;
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index e5972f16abff..ef96028fa56b 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -1020,6 +1020,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
>  	smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);
> 
>  	raw_spin_unlock(&drvdata->spinlock);
> +
> +	cscfg_csdev_disable_active_config(csdev);
> +

In general, we need to split changes into several patches if each
addresses a different issue.  From my understanding, the change above is
to fix missing to disable config when disable Sysfs mode.

If so, could we use a seperate patch for this change?

>  	cpus_read_unlock();
> 
>  	/*
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> index a70c1454b410..6d8c212ad434 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> @@ -391,14 +391,17 @@ static void cscfg_owner_put(struct cscfg_load_owner_info *owner_info)
>  static void cscfg_remove_owned_csdev_configs(struct coresight_device *csdev, void *load_owner)
>  {
>  	struct cscfg_config_csdev *config_csdev, *tmp;
> +	unsigned long flags;
> 
>  	if (list_empty(&csdev->config_csdev_list))
>  		return;
> 
> +	raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);

I think we should use spinlock to guard the condition checking
list_empty().

Here the race condition is the 'config_csdev_list' list and
configurations on the list.  For atomicity, we should use lock to
protect any operations on the list (read, add, delete, etc).

A side topic, as here it adds locks for protecting 'config_csdev_list',
I am wandering why we do not do the same thing for
'feature_csdev_list' (See cscfg_remove_owned_csdev_features() and
cscfg_get_feat_csdev()).

>  	list_for_each_entry_safe(config_csdev, tmp, &csdev->config_csdev_list, node) {
>  		if (config_csdev->config_desc->load_owner == load_owner)
>  			list_del(&config_csdev->node);
>  	}
> +	raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
>  }
> 
>  static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> @@ -867,6 +870,25 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
>  }
>  EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
> 
> +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc)
> +{
> +	if (!atomic_fetch_inc(&config_desc->active_cnt)) {
> +		/* must ensure that config cannot be unloaded in use */
> +		if (unlikely(cscfg_owner_get(config_desc->load_owner))) {
> +			atomic_dec(&config_desc->active_cnt);
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
> +static void cscfg_config_desc_put(struct cscfg_config_desc *config_desc)
> +{
> +	if (!atomic_dec_return(&config_desc->active_cnt))
> +		cscfg_owner_put(config_desc->load_owner);
> +}
> +
>  /*
>   * This activate configuration for either perf or sysfs. Perf can have multiple
>   * active configs, selected per event, sysfs is limited to one.
> @@ -890,22 +912,17 @@ static int _cscfg_activate_config(unsigned long cfg_hash)
>  			if (config_desc->available == false)
>  				return -EBUSY;
> 
> -			/* must ensure that config cannot be unloaded in use */
> -			err = cscfg_owner_get(config_desc->load_owner);
> -			if (err)
> +			if (!cscfg_config_desc_get(config_desc)) {
> +				err = -EINVAL;
>  				break;
> +			}
> +
>  			/*
>  			 * increment the global active count - control changes to
>  			 * active configurations
>  			 */
>  			atomic_inc(&cscfg_mgr->sys_active_cnt);

Seems to me, it is more reasonable to use 'sys_active_cnt' to acquire
the module reference instead of 'config_desc->active_cnt'.  The reason
is 'sys_active_cnt' is a global counter.

> -			/*
> -			 * mark the descriptor as active so enable config on a
> -			 * device instance will use it
> -			 */
> -			atomic_inc(&config_desc->active_cnt);
> -
>  			err = 0;
>  			dev_dbg(cscfg_device(), "Activate config %s.\n", config_desc->name);
>  			break;
> @@ -920,9 +937,8 @@ static void _cscfg_deactivate_config(unsigned long cfg_hash)
> 
>  	list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
>  		if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> -			atomic_dec(&config_desc->active_cnt);
>  			atomic_dec(&cscfg_mgr->sys_active_cnt);
> -			cscfg_owner_put(config_desc->load_owner);
> +			cscfg_config_desc_put(config_desc);
>  			dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
>  			break;
>  		}
> @@ -1047,7 +1063,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
>  				     unsigned long cfg_hash, int preset)
>  {
>  	struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
> -	const struct cscfg_config_desc *config_desc;
> +	struct cscfg_config_desc *config_desc;
>  	unsigned long flags;
>  	int err = 0;
> 
> @@ -1062,8 +1078,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
>  	raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
>  	list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
>  		config_desc = config_csdev_item->config_desc;
> -		if ((atomic_read(&config_desc->active_cnt)) &&
> -		    ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
> +		if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
> +				cscfg_config_desc_get(config_desc)) {

This seems to me not right.  Why a config descriptor is get in multiple
places?  One time getting a config descriptor is in
_cscfg_activate_config(), another is at here.

To be honest, I am not clear what is the difference between 'activate'
config and 'enable active' config.  Literally, I think we only need to
acquire the config at its creating phase (maybe match to activate
config?).

>  			config_csdev_active = config_csdev_item;
>  			csdev->active_cscfg_ctxt = (void *)config_csdev_active;
>  			break;
> @@ -1097,7 +1113,11 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
>  				err = -EBUSY;
>  			raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
>  		}
> +
> +		if (err)
> +			cscfg_config_desc_put(config_desc);
>  	}
> +
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
> @@ -1136,8 +1156,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev)
>  	raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> 
>  	/* true if there was an enabled active config */
> -	if (config_csdev)
> +	if (config_csdev) {
>  		cscfg_csdev_disable_config(config_csdev);
> +		cscfg_config_desc_put(config_csdev->config_desc);
> +	}
>  }
>  EXPORT_SYMBOL_GPL(cscfg_csdev_disable_active_config);
> 
> --
> LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
> 

Re: [PATCH v4] coresight: prevent deactivate active config while enabling the config

[Yeoreum Yun]

Hi Leo,

> > While enable active config via cscfg_csdev_enable_active_config(),
> > active config could be deactivated via configfs' sysfs interface.
> > This could make UAF issue in below scenario:
> >
> > CPU0                                          CPU1
> > (sysfs enable)                                load module
> >                                               cscfg_load_config_sets()
> >                                               activate config. // sysfs
> >                                               (sys_active_cnt == 1)
> > ...
> > cscfg_csdev_enable_active_config()
> >   lock(csdev->cscfg_csdev_lock)
> >   // here load config activate by CPU1
> >   unlock(csdev->cscfg_csdev_lock)
> >
> >                                               deactivate config // sysfs
> >                                               (sys_activec_cnt == 0)
> >                                               cscfg_unload_config_sets()
> >                                               unload module
> >
> >   // access to config_desc which freed
> >   // while unloading module.
> >   cfs_csdev_enable_config
>
> I am not sure if this flow can happen.  CoreSight configfs feature is
> integrated into the CoreSight core layer, and the other CoreSight
> modules are dependent on it.
>
> For example, if the ETM4x module is not removed, the kernel module
> management will natually prevent the CoreSight core module from being
> removed.
>

No. Suppose some user writes custom config module for etm4x using
cscfg_load_config_sets() cscfg_unload_config_sets() in init/exit of
module function.

Although it's rare case but it can happen while  above 2 interfaces are
EXPORTED.

> > To address this, use cscfg_config_desc's active_cnt as a reference count
> > which will be holded when
> >     - activate the config.
> >     - enable the activated config.
> > and put the module reference when config_active_cnt == 0.
> >
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> > ---
> > Since v3:
> >   - Remove enable arguments in cscfg_config_desc_get() (from Mike).
> >   - https://lore.kernel.org/all/20250109171956.3535294-1-yeoreum.yun@arm.com/
> > ---
> >  .../hwtracing/coresight/coresight-config.h    |  2 +-
> >  .../coresight/coresight-etm4x-core.c          |  3 ++
> >  .../hwtracing/coresight/coresight-syscfg.c    | 52 +++++++++++++------
> >  3 files changed, 41 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> > index b9ebc9fcfb7f..90fd937d3bd8 100644
> > --- a/drivers/hwtracing/coresight/coresight-config.h
> > +++ b/drivers/hwtracing/coresight/coresight-config.h
> > @@ -228,7 +228,7 @@ struct cscfg_feature_csdev {
> >   * @feats_csdev:references to the device features to enable.
> >   */
> >  struct cscfg_config_csdev {
> > -	const struct cscfg_config_desc *config_desc;
> > +	struct cscfg_config_desc *config_desc;
> >  	struct coresight_device *csdev;
> >  	bool enabled;
> >  	struct list_head node;
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index e5972f16abff..ef96028fa56b 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -1020,6 +1020,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
> >  	smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);
> >
> >  	raw_spin_unlock(&drvdata->spinlock);
> > +
> > +	cscfg_csdev_disable_active_config(csdev);
> > +
>
> In general, we need to split changes into several patches if each
> addresses a different issue.  From my understanding, the change above is
> to fix missing to disable config when disable Sysfs mode.
>
> If so, could we use a seperate patch for this change?
>

It's not a differnt issue. Without this line, the active count wouldn't
decrese and it raise another issue -- unloadable moudle for active_cnt :(
So I think it should be included in this patch.

> >  	cpus_read_unlock();
> >
> >  	/*
> > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> > index a70c1454b410..6d8c212ad434 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > @@ -391,14 +391,17 @@ static void cscfg_owner_put(struct cscfg_load_owner_info *owner_info)
> >  static void cscfg_remove_owned_csdev_configs(struct coresight_device *csdev, void *load_owner)
> >  {
> >  	struct cscfg_config_csdev *config_csdev, *tmp;
> > +	unsigned long flags;
> >
> >  	if (list_empty(&csdev->config_csdev_list))
> >  		return;
> >
> > +	raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
>
> I think we should use spinlock to guard the condition checking
> list_empty().
>
> Here the race condition is the 'config_csdev_list' list and
> configurations on the list.  For atomicity, we should use lock to
> protect any operations on the list (read, add, delete, etc).

Interesting... Would you let me know which race it is?
here to check list_empty(), it already guarded with "cscfg_mutex".
However list_del() is special case because iterating config_csdev_list
can be done without cscfg_mutex -- see
cscfg_csdev_enable_active_config().
This gurad with spinlock purpose to guard race unloading and
get the config in cscfg_csdev_enable_active_config()
(Please see my response below...).

the emptiness of config_csdev_list is guarded with cscfg_mutex.
therefore, It seems enough to guard iterating part with spinlock :)

> A side topic, as here it adds locks for protecting 'config_csdev_list',
> I am wandering why we do not do the same thing for
> 'feature_csdev_list' (See cscfg_remove_owned_csdev_features() and
> cscfg_get_feat_csdev()).

In case of feature, It's okay since it couldn't be accessed when it
gets failed to get related config.

When we see cscfg_csdev_enable_active_config(), the config could be
accessed without cscfg_mutex lock. so the config need to be guarded with
spin_lock otherwise it could be acquired while unload module
(after get active_cnt in search logic cscfg_csdev_enable_active_config()
and other running unloading process)

But feature list is depends on config, If config is safe from
load/unload, this is not an issue so we don't need it.

Thanks for your review!

> >  	list_for_each_entry_safe(config_csdev, tmp, &csdev->config_csdev_list, node) {
> >  		if (config_csdev->config_desc->load_owner == load_owner)
> >  			list_del(&config_csdev->node);
> >  	}
> > +	raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> >  }
> >
> >  static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> > @@ -867,6 +870,25 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
> >  }
> >  EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
> >
> > +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc)
> > +{
> > +	if (!atomic_fetch_inc(&config_desc->active_cnt)) {
> > +		/* must ensure that config cannot be unloaded in use */
> > +		if (unlikely(cscfg_owner_get(config_desc->load_owner))) {
> > +			atomic_dec(&config_desc->active_cnt);
> > +			return false;
> > +		}
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static void cscfg_config_desc_put(struct cscfg_config_desc *config_desc)
> > +{
> > +	if (!atomic_dec_return(&config_desc->active_cnt))
> > +		cscfg_owner_put(config_desc->load_owner);
> > +}
> > +
> >  /*
> >   * This activate configuration for either perf or sysfs. Perf can have multiple
> >   * active configs, selected per event, sysfs is limited to one.
> > @@ -890,22 +912,17 @@ static int _cscfg_activate_config(unsigned long cfg_hash)
> >  			if (config_desc->available == false)
> >  				return -EBUSY;
> >
> > -			/* must ensure that config cannot be unloaded in use */
> > -			err = cscfg_owner_get(config_desc->load_owner);
> > -			if (err)
> > +			if (!cscfg_config_desc_get(config_desc)) {
> > +				err = -EINVAL;
> >  				break;
> > +			}
> > +
> >  			/*
> >  			 * increment the global active count - control changes to
> >  			 * active configurations
> >  			 */
> >  			atomic_inc(&cscfg_mgr->sys_active_cnt);
>
> Seems to me, it is more reasonable to use 'sys_active_cnt' to acquire
> the module reference instead of 'config_desc->active_cnt'.  The reason
> is 'sys_active_cnt' is a global counter.
>
> > -			/*
> > -			 * mark the descriptor as active so enable config on a
> > -			 * device instance will use it
> > -			 */
> > -			atomic_inc(&config_desc->active_cnt);
> > -
> >  			err = 0;
> >  			dev_dbg(cscfg_device(), "Activate config %s.\n", config_desc->name);
> >  			break;
> > @@ -920,9 +937,8 @@ static void _cscfg_deactivate_config(unsigned long cfg_hash)
> >
> >  	list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
> >  		if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> > -			atomic_dec(&config_desc->active_cnt);
> >  			atomic_dec(&cscfg_mgr->sys_active_cnt);
> > -			cscfg_owner_put(config_desc->load_owner);
> > +			cscfg_config_desc_put(config_desc);
> >  			dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
> >  			break;
> >  		}
> > @@ -1047,7 +1063,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> >  				     unsigned long cfg_hash, int preset)
> >  {
> >  	struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
> > -	const struct cscfg_config_desc *config_desc;
> > +	struct cscfg_config_desc *config_desc;
> >  	unsigned long flags;
> >  	int err = 0;
> >
> > @@ -1062,8 +1078,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> >  	raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> >  	list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
> >  		config_desc = config_csdev_item->config_desc;
> > -		if ((atomic_read(&config_desc->active_cnt)) &&
> > -		    ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
> > +		if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
> > +				cscfg_config_desc_get(config_desc)) {
>
> This seems to me not right.  Why a config descriptor is get in multiple
> places?  One time getting a config descriptor is in
> _cscfg_activate_config(), another is at here.
>
> To be honest, I am not clear what is the difference between 'activate'
> config and 'enable active' config.  Literally, I think we only need to
> acquire the config at its creating phase (maybe match to activate
> config?).
>
> >  			config_csdev_active = config_csdev_item;
> >  			csdev->active_cscfg_ctxt = (void *)config_csdev_active;
> >  			break;
> > @@ -1097,7 +1113,11 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> >  				err = -EBUSY;
> >  			raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> >  		}
> > +
> > +		if (err)
> > +			cscfg_config_desc_put(config_desc);
> >  	}
> > +
> >  	return err;
> >  }
> >  EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
> > @@ -1136,8 +1156,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev)
> >  	raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> >
> >  	/* true if there was an enabled active config */
> > -	if (config_csdev)
> > +	if (config_csdev) {
> >  		cscfg_csdev_disable_config(config_csdev);
> > +		cscfg_config_desc_put(config_csdev->config_desc);
> > +	}
> >  }
> >  EXPORT_SYMBOL_GPL(cscfg_csdev_disable_active_config);
> >
> > --
> > LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
> >

Re: [PATCH v4] coresight: prevent deactivate active config while enabling the config

[Yeo Reum Yun]

Gentle ping in case of forgotten.

________________________________________
From: Yeo Reum Yun
Sent: 26 March 2025 07:34
To: Leo Yan
Cc: Suzuki Poulose; mike.leach@linaro.org; james.clark@linaro.org; alexander.shishkin@linux.intel.com; coresight@lists.linaro.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] coresight: prevent deactivate active config while enabling the config

Hi Leo,

> > While enable active config via cscfg_csdev_enable_active_config(),
> > active config could be deactivated via configfs' sysfs interface.
> > This could make UAF issue in below scenario:
> >
> > CPU0                                          CPU1
> > (sysfs enable)                                load module
> >                                               cscfg_load_config_sets()
> >                                               activate config. // sysfs
> >                                               (sys_active_cnt == 1)
> > ...
> > cscfg_csdev_enable_active_config()
> >   lock(csdev->cscfg_csdev_lock)
> >   // here load config activate by CPU1
> >   unlock(csdev->cscfg_csdev_lock)
> >
> >                                               deactivate config // sysfs
> >                                               (sys_activec_cnt == 0)
> >                                               cscfg_unload_config_sets()
> >                                               unload module
> >
> >   // access to config_desc which freed
> >   // while unloading module.
> >   cfs_csdev_enable_config
>
> I am not sure if this flow can happen.  CoreSight configfs feature is
> integrated into the CoreSight core layer, and the other CoreSight
> modules are dependent on it.
>
> For example, if the ETM4x module is not removed, the kernel module
> management will natually prevent the CoreSight core module from being
> removed.
>

No. Suppose some user writes custom config module for etm4x using
cscfg_load_config_sets() cscfg_unload_config_sets() in init/exit of
module function.

Although it's rare case but it can happen while  above 2 interfaces are
EXPORTED.

> > To address this, use cscfg_config_desc's active_cnt as a reference count
> > which will be holded when
> >     - activate the config.
> >     - enable the activated config.
> > and put the module reference when config_active_cnt == 0.
> >
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> > ---
> > Since v3:
> >   - Remove enable arguments in cscfg_config_desc_get() (from Mike).
> >   - https://lore.kernel.org/all/20250109171956.3535294-1-yeoreum.yun@arm.com/
> > ---
> >  .../hwtracing/coresight/coresight-config.h    |  2 +-
> >  .../coresight/coresight-etm4x-core.c          |  3 ++
> >  .../hwtracing/coresight/coresight-syscfg.c    | 52 +++++++++++++------
> >  3 files changed, 41 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> > index b9ebc9fcfb7f..90fd937d3bd8 100644
> > --- a/drivers/hwtracing/coresight/coresight-config.h
> > +++ b/drivers/hwtracing/coresight/coresight-config.h
> > @@ -228,7 +228,7 @@ struct cscfg_feature_csdev {
> >   * @feats_csdev:references to the device features to enable.
> >   */
> >  struct cscfg_config_csdev {
> > -   const struct cscfg_config_desc *config_desc;
> > +   struct cscfg_config_desc *config_desc;
> >     struct coresight_device *csdev;
> >     bool enabled;
> >     struct list_head node;
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index e5972f16abff..ef96028fa56b 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -1020,6 +1020,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
> >     smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);
> >
> >     raw_spin_unlock(&drvdata->spinlock);
> > +
> > +   cscfg_csdev_disable_active_config(csdev);
> > +
>
> In general, we need to split changes into several patches if each
> addresses a different issue.  From my understanding, the change above is
> to fix missing to disable config when disable Sysfs mode.
>
> If so, could we use a seperate patch for this change?
>

It's not a differnt issue. Without this line, the active count wouldn't
decrese and it raise another issue -- unloadable moudle for active_cnt :(
So I think it should be included in this patch.

> >     cpus_read_unlock();
> >
> >     /*
> > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> > index a70c1454b410..6d8c212ad434 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > @@ -391,14 +391,17 @@ static void cscfg_owner_put(struct cscfg_load_owner_info *owner_info)
> >  static void cscfg_remove_owned_csdev_configs(struct coresight_device *csdev, void *load_owner)
> >  {
> >     struct cscfg_config_csdev *config_csdev, *tmp;
> > +   unsigned long flags;
> >
> >     if (list_empty(&csdev->config_csdev_list))
> >             return;
> >
> > +   raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
>
> I think we should use spinlock to guard the condition checking
> list_empty().
>
> Here the race condition is the 'config_csdev_list' list and
> configurations on the list.  For atomicity, we should use lock to
> protect any operations on the list (read, add, delete, etc).

Interesting... Would you let me know which race it is?
here to check list_empty(), it already guarded with "cscfg_mutex".
However list_del() is special case because iterating config_csdev_list
can be done without cscfg_mutex -- see
cscfg_csdev_enable_active_config().
This gurad with spinlock purpose to guard race unloading and
get the config in cscfg_csdev_enable_active_config()
(Please see my response below...).

the emptiness of config_csdev_list is guarded with cscfg_mutex.
therefore, It seems enough to guard iterating part with spinlock :)

> A side topic, as here it adds locks for protecting 'config_csdev_list',
> I am wandering why we do not do the same thing for
> 'feature_csdev_list' (See cscfg_remove_owned_csdev_features() and
> cscfg_get_feat_csdev()).

In case of feature, It's okay since it couldn't be accessed when it
gets failed to get related config.

When we see cscfg_csdev_enable_active_config(), the config could be
accessed without cscfg_mutex lock. so the config need to be guarded with
spin_lock otherwise it could be acquired while unload module
(after get active_cnt in search logic cscfg_csdev_enable_active_config()
and other running unloading process)

But feature list is depends on config, If config is safe from
load/unload, this is not an issue so we don't need it.

Thanks for your review!

> >     list_for_each_entry_safe(config_csdev, tmp, &csdev->config_csdev_list, node) {
> >             if (config_csdev->config_desc->load_owner == load_owner)
> >                     list_del(&config_csdev->node);
> >     }
> > +   raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> >  }
> >
> >  static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> > @@ -867,6 +870,25 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
> >  }
> >  EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
> >
> > +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc)
> > +{
> > +   if (!atomic_fetch_inc(&config_desc->active_cnt)) {
> > +           /* must ensure that config cannot be unloaded in use */
> > +           if (unlikely(cscfg_owner_get(config_desc->load_owner))) {
> > +                   atomic_dec(&config_desc->active_cnt);
> > +                   return false;
> > +           }
> > +   }
> > +
> > +   return true;
> > +}
> > +
> > +static void cscfg_config_desc_put(struct cscfg_config_desc *config_desc)
> > +{
> > +   if (!atomic_dec_return(&config_desc->active_cnt))
> > +           cscfg_owner_put(config_desc->load_owner);
> > +}
> > +
> >  /*
> >   * This activate configuration for either perf or sysfs. Perf can have multiple
> >   * active configs, selected per event, sysfs is limited to one.
> > @@ -890,22 +912,17 @@ static int _cscfg_activate_config(unsigned long cfg_hash)
> >                     if (config_desc->available == false)
> >                             return -EBUSY;
> >
> > -                   /* must ensure that config cannot be unloaded in use */
> > -                   err = cscfg_owner_get(config_desc->load_owner);
> > -                   if (err)
> > +                   if (!cscfg_config_desc_get(config_desc)) {
> > +                           err = -EINVAL;
> >                             break;
> > +                   }
> > +
> >                     /*
> >                      * increment the global active count - control changes to
> >                      * active configurations
> >                      */
> >                     atomic_inc(&cscfg_mgr->sys_active_cnt);
>
> Seems to me, it is more reasonable to use 'sys_active_cnt' to acquire
> the module reference instead of 'config_desc->active_cnt'.  The reason
> is 'sys_active_cnt' is a global counter.
>
> > -                   /*
> > -                    * mark the descriptor as active so enable config on a
> > -                    * device instance will use it
> > -                    */
> > -                   atomic_inc(&config_desc->active_cnt);
> > -
> >                     err = 0;
> >                     dev_dbg(cscfg_device(), "Activate config %s.\n", config_desc->name);
> >                     break;
> > @@ -920,9 +937,8 @@ static void _cscfg_deactivate_config(unsigned long cfg_hash)
> >
> >     list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
> >             if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> > -                   atomic_dec(&config_desc->active_cnt);
> >                     atomic_dec(&cscfg_mgr->sys_active_cnt);
> > -                   cscfg_owner_put(config_desc->load_owner);
> > +                   cscfg_config_desc_put(config_desc);
> >                     dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
> >                     break;
> >             }
> > @@ -1047,7 +1063,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> >                                  unsigned long cfg_hash, int preset)
> >  {
> >     struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
> > -   const struct cscfg_config_desc *config_desc;
> > +   struct cscfg_config_desc *config_desc;
> >     unsigned long flags;
> >     int err = 0;
> >
> > @@ -1062,8 +1078,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> >     raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> >     list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
> >             config_desc = config_csdev_item->config_desc;
> > -           if ((atomic_read(&config_desc->active_cnt)) &&
> > -               ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
> > +           if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
> > +                           cscfg_config_desc_get(config_desc)) {
>
> This seems to me not right.  Why a config descriptor is get in multiple
> places?  One time getting a config descriptor is in
> _cscfg_activate_config(), another is at here.
>
> To be honest, I am not clear what is the difference between 'activate'
> config and 'enable active' config.  Literally, I think we only need to
> acquire the config at its creating phase (maybe match to activate
> config?).
>
> >                     config_csdev_active = config_csdev_item;
> >                     csdev->active_cscfg_ctxt = (void *)config_csdev_active;
> >                     break;
> > @@ -1097,7 +1113,11 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> >                             err = -EBUSY;
> >                     raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> >             }
> > +
> > +           if (err)
> > +                   cscfg_config_desc_put(config_desc);
> >     }
> > +
> >     return err;
> >  }
> >  EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
> > @@ -1136,8 +1156,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev)
> >     raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> >
> >     /* true if there was an enabled active config */
> > -   if (config_csdev)
> > +   if (config_csdev) {
> >             cscfg_csdev_disable_config(config_csdev);
> > +           cscfg_config_desc_put(config_csdev->config_desc);
> > +   }
> >  }
> >  EXPORT_SYMBOL_GPL(cscfg_csdev_disable_active_config);
> >
> > --
> > LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
> >

Re: [PATCH v4] coresight: prevent deactivate active config while enabling the config

[Yeo Reum Yun]

Gentle ping incase of forgotten.

________________________________________
From: Yeo Reum Yun <YeoReum.Yun@arm.com>
Sent: 02 April 2025 11:23
To: Leo Yan
Cc: Suzuki Poulose; mike.leach@linaro.org; james.clark@linaro.org; alexander.shishkin@linux.intel.com; coresight@lists.linaro.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] coresight: prevent deactivate active config while enabling the config

Gentle ping in case of forgotten.

________________________________________
From: Yeo Reum Yun
Sent: 26 March 2025 07:34
To: Leo Yan
Cc: Suzuki Poulose; mike.leach@linaro.org; james.clark@linaro.org; alexander.shishkin@linux.intel.com; coresight@lists.linaro.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] coresight: prevent deactivate active config while enabling the config

Hi Leo,

> > While enable active config via cscfg_csdev_enable_active_config(),
> > active config could be deactivated via configfs' sysfs interface.
> > This could make UAF issue in below scenario:
> >
> > CPU0                                          CPU1
> > (sysfs enable)                                load module
> >                                               cscfg_load_config_sets()
> >                                               activate config. // sysfs
> >                                               (sys_active_cnt == 1)
> > ...
> > cscfg_csdev_enable_active_config()
> >   lock(csdev->cscfg_csdev_lock)
> >   // here load config activate by CPU1
> >   unlock(csdev->cscfg_csdev_lock)
> >
> >                                               deactivate config // sysfs
> >                                               (sys_activec_cnt == 0)
> >                                               cscfg_unload_config_sets()
> >                                               unload module
> >
> >   // access to config_desc which freed
> >   // while unloading module.
> >   cfs_csdev_enable_config
>
> I am not sure if this flow can happen.  CoreSight configfs feature is
> integrated into the CoreSight core layer, and the other CoreSight
> modules are dependent on it.
>
> For example, if the ETM4x module is not removed, the kernel module
> management will natually prevent the CoreSight core module from being
> removed.
>

No. Suppose some user writes custom config module for etm4x using
cscfg_load_config_sets() cscfg_unload_config_sets() in init/exit of
module function.

Although it's rare case but it can happen while  above 2 interfaces are
EXPORTED.

> > To address this, use cscfg_config_desc's active_cnt as a reference count
> > which will be holded when
> >     - activate the config.
> >     - enable the activated config.
> > and put the module reference when config_active_cnt == 0.
> >
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> > ---
> > Since v3:
> >   - Remove enable arguments in cscfg_config_desc_get() (from Mike).
> >   - https://lore.kernel.org/all/20250109171956.3535294-1-yeoreum.yun@arm.com/
> > ---
> >  .../hwtracing/coresight/coresight-config.h    |  2 +-
> >  .../coresight/coresight-etm4x-core.c          |  3 ++
> >  .../hwtracing/coresight/coresight-syscfg.c    | 52 +++++++++++++------
> >  3 files changed, 41 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-config.h b/drivers/hwtracing/coresight/coresight-config.h
> > index b9ebc9fcfb7f..90fd937d3bd8 100644
> > --- a/drivers/hwtracing/coresight/coresight-config.h
> > +++ b/drivers/hwtracing/coresight/coresight-config.h
> > @@ -228,7 +228,7 @@ struct cscfg_feature_csdev {
> >   * @feats_csdev:references to the device features to enable.
> >   */
> >  struct cscfg_config_csdev {
> > -   const struct cscfg_config_desc *config_desc;
> > +   struct cscfg_config_desc *config_desc;
> >     struct coresight_device *csdev;
> >     bool enabled;
> >     struct list_head node;
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index e5972f16abff..ef96028fa56b 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -1020,6 +1020,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
> >     smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);
> >
> >     raw_spin_unlock(&drvdata->spinlock);
> > +
> > +   cscfg_csdev_disable_active_config(csdev);
> > +
>
> In general, we need to split changes into several patches if each
> addresses a different issue.  From my understanding, the change above is
> to fix missing to disable config when disable Sysfs mode.
>
> If so, could we use a seperate patch for this change?
>

It's not a differnt issue. Without this line, the active count wouldn't
decrese and it raise another issue -- unloadable moudle for active_cnt :(
So I think it should be included in this patch.

> >     cpus_read_unlock();
> >
> >     /*
> > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> > index a70c1454b410..6d8c212ad434 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > @@ -391,14 +391,17 @@ static void cscfg_owner_put(struct cscfg_load_owner_info *owner_info)
> >  static void cscfg_remove_owned_csdev_configs(struct coresight_device *csdev, void *load_owner)
> >  {
> >     struct cscfg_config_csdev *config_csdev, *tmp;
> > +   unsigned long flags;
> >
> >     if (list_empty(&csdev->config_csdev_list))
> >             return;
> >
> > +   raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
>
> I think we should use spinlock to guard the condition checking
> list_empty().
>
> Here the race condition is the 'config_csdev_list' list and
> configurations on the list.  For atomicity, we should use lock to
> protect any operations on the list (read, add, delete, etc).

Interesting... Would you let me know which race it is?
here to check list_empty(), it already guarded with "cscfg_mutex".
However list_del() is special case because iterating config_csdev_list
can be done without cscfg_mutex -- see
cscfg_csdev_enable_active_config().
This gurad with spinlock purpose to guard race unloading and
get the config in cscfg_csdev_enable_active_config()
(Please see my response below...).

the emptiness of config_csdev_list is guarded with cscfg_mutex.
therefore, It seems enough to guard iterating part with spinlock :)

> A side topic, as here it adds locks for protecting 'config_csdev_list',
> I am wandering why we do not do the same thing for
> 'feature_csdev_list' (See cscfg_remove_owned_csdev_features() and
> cscfg_get_feat_csdev()).

In case of feature, It's okay since it couldn't be accessed when it
gets failed to get related config.

When we see cscfg_csdev_enable_active_config(), the config could be
accessed without cscfg_mutex lock. so the config need to be guarded with
spin_lock otherwise it could be acquired while unload module
(after get active_cnt in search logic cscfg_csdev_enable_active_config()
and other running unloading process)

But feature list is depends on config, If config is safe from
load/unload, this is not an issue so we don't need it.

Thanks for your review!

> >     list_for_each_entry_safe(config_csdev, tmp, &csdev->config_csdev_list, node) {
> >             if (config_csdev->config_desc->load_owner == load_owner)
> >                     list_del(&config_csdev->node);
> >     }
> > +   raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> >  }
> >
> >  static void cscfg_remove_owned_csdev_features(struct coresight_device *csdev, void *load_owner)
> > @@ -867,6 +870,25 @@ void cscfg_csdev_reset_feats(struct coresight_device *csdev)
> >  }
> >  EXPORT_SYMBOL_GPL(cscfg_csdev_reset_feats);
> >
> > +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc)
> > +{
> > +   if (!atomic_fetch_inc(&config_desc->active_cnt)) {
> > +           /* must ensure that config cannot be unloaded in use */
> > +           if (unlikely(cscfg_owner_get(config_desc->load_owner))) {
> > +                   atomic_dec(&config_desc->active_cnt);
> > +                   return false;
> > +           }
> > +   }
> > +
> > +   return true;
> > +}
> > +
> > +static void cscfg_config_desc_put(struct cscfg_config_desc *config_desc)
> > +{
> > +   if (!atomic_dec_return(&config_desc->active_cnt))
> > +           cscfg_owner_put(config_desc->load_owner);
> > +}
> > +
> >  /*
> >   * This activate configuration for either perf or sysfs. Perf can have multiple
> >   * active configs, selected per event, sysfs is limited to one.
> > @@ -890,22 +912,17 @@ static int _cscfg_activate_config(unsigned long cfg_hash)
> >                     if (config_desc->available == false)
> >                             return -EBUSY;
> >
> > -                   /* must ensure that config cannot be unloaded in use */
> > -                   err = cscfg_owner_get(config_desc->load_owner);
> > -                   if (err)
> > +                   if (!cscfg_config_desc_get(config_desc)) {
> > +                           err = -EINVAL;
> >                             break;
> > +                   }
> > +
> >                     /*
> >                      * increment the global active count - control changes to
> >                      * active configurations
> >                      */
> >                     atomic_inc(&cscfg_mgr->sys_active_cnt);
>
> Seems to me, it is more reasonable to use 'sys_active_cnt' to acquire
> the module reference instead of 'config_desc->active_cnt'.  The reason
> is 'sys_active_cnt' is a global counter.
>
> > -                   /*
> > -                    * mark the descriptor as active so enable config on a
> > -                    * device instance will use it
> > -                    */
> > -                   atomic_inc(&config_desc->active_cnt);
> > -
> >                     err = 0;
> >                     dev_dbg(cscfg_device(), "Activate config %s.\n", config_desc->name);
> >                     break;
> > @@ -920,9 +937,8 @@ static void _cscfg_deactivate_config(unsigned long cfg_hash)
> >
> >     list_for_each_entry(config_desc, &cscfg_mgr->config_desc_list, item) {
> >             if ((unsigned long)config_desc->event_ea->var == cfg_hash) {
> > -                   atomic_dec(&config_desc->active_cnt);
> >                     atomic_dec(&cscfg_mgr->sys_active_cnt);
> > -                   cscfg_owner_put(config_desc->load_owner);
> > +                   cscfg_config_desc_put(config_desc);
> >                     dev_dbg(cscfg_device(), "Deactivate config %s.\n", config_desc->name);
> >                     break;
> >             }
> > @@ -1047,7 +1063,7 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> >                                  unsigned long cfg_hash, int preset)
> >  {
> >     struct cscfg_config_csdev *config_csdev_active = NULL, *config_csdev_item;
> > -   const struct cscfg_config_desc *config_desc;
> > +   struct cscfg_config_desc *config_desc;
> >     unsigned long flags;
> >     int err = 0;
> >
> > @@ -1062,8 +1078,8 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> >     raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> >     list_for_each_entry(config_csdev_item, &csdev->config_csdev_list, node) {
> >             config_desc = config_csdev_item->config_desc;
> > -           if ((atomic_read(&config_desc->active_cnt)) &&
> > -               ((unsigned long)config_desc->event_ea->var == cfg_hash)) {
> > +           if (((unsigned long)config_desc->event_ea->var == cfg_hash) &&
> > +                           cscfg_config_desc_get(config_desc)) {
>
> This seems to me not right.  Why a config descriptor is get in multiple
> places?  One time getting a config descriptor is in
> _cscfg_activate_config(), another is at here.
>
> To be honest, I am not clear what is the difference between 'activate'
> config and 'enable active' config.  Literally, I think we only need to
> acquire the config at its creating phase (maybe match to activate
> config?).
>
> >                     config_csdev_active = config_csdev_item;
> >                     csdev->active_cscfg_ctxt = (void *)config_csdev_active;
> >                     break;
> > @@ -1097,7 +1113,11 @@ int cscfg_csdev_enable_active_config(struct coresight_device *csdev,
> >                             err = -EBUSY;
> >                     raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> >             }
> > +
> > +           if (err)
> > +                   cscfg_config_desc_put(config_desc);
> >     }
> > +
> >     return err;
> >  }
> >  EXPORT_SYMBOL_GPL(cscfg_csdev_enable_active_config);
> > @@ -1136,8 +1156,10 @@ void cscfg_csdev_disable_active_config(struct coresight_device *csdev)
> >     raw_spin_unlock_irqrestore(&csdev->cscfg_csdev_lock, flags);
> >
> >     /* true if there was an enabled active config */
> > -   if (config_csdev)
> > +   if (config_csdev) {
> >             cscfg_csdev_disable_config(config_csdev);
> > +           cscfg_config_desc_put(config_csdev->config_desc);
> > +   }
> >  }
> >  EXPORT_SYMBOL_GPL(cscfg_csdev_disable_active_config);
> >
> > --
> > LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
> >

Re: [PATCH v4] coresight: prevent deactivate active config while enabling the config

[Leo Yan]

Hi Levi,

On Wed, Mar 26, 2025 at 07:34:25AM +0000, Yeoreum Yun wrote:

[...]

> > > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > @@ -1020,6 +1020,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
> > >  	smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);
> > >
> > >  	raw_spin_unlock(&drvdata->spinlock);
> > > +
> > > +	cscfg_csdev_disable_active_config(csdev);
> > > +
> >
> > In general, we need to split changes into several patches if each
> > addresses a different issue.  From my understanding, the change above is
> > to fix missing to disable config when disable Sysfs mode.
> >
> > If so, could we use a seperate patch for this change?
> >
> 
> It's not a differnt issue. Without this line, the active count wouldn't
> decrese and it raise another issue -- unloadable moudle for active_cnt :(
> So I think it should be included in this patch.

I read the code again and concluded the change above is not related to
locking and would be a separate issue: when we close a Sysfs session,
we need to disable a config on a CoreSight device.

Could you clarify what is meaning "unloadable moudle for active_cnt"?
I only saw "cscfg_mgr->sys_active_cnt" is used for module unloading,
but have no clue why "active_cnt" impacts module unloading.

> > >  	cpus_read_unlock();
> > >
> > >  	/*
> > > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> > > index a70c1454b410..6d8c212ad434 100644
> > > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > > @@ -391,14 +391,17 @@ static void cscfg_owner_put(struct cscfg_load_owner_info *owner_info)
> > >  static void cscfg_remove_owned_csdev_configs(struct coresight_device *csdev, void *load_owner)
> > >  {
> > >  	struct cscfg_config_csdev *config_csdev, *tmp;
> > > +	unsigned long flags;
> > >
> > >  	if (list_empty(&csdev->config_csdev_list))
> > >  		return;
> > >
> > > +	raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> >
> > I think we should use spinlock to guard the condition checking
> > list_empty().
> >
> > Here the race condition is the 'config_csdev_list' list and
> > configurations on the list.  For atomicity, we should use lock to
> > protect any operations on the list (read, add, delete, etc).
> 
> Interesting... Would you let me know which race it is?
> here to check list_empty(), it already guarded with "cscfg_mutex".

Thanks for pointing out this.  I read the code and understood that in
some scenarios the list is protected by the mutex "cscfg_mutex".

I would argue for using locking, we need to make clear for two thigns:

- What is the race condition;
- What locking is used to protect the race condition.

For current case, a CoreSight device has a config list, the race
condition is the config list will be manipulated by multiple places
(e.g., for module loading / unloading, opening or closing a perf or
SysFS session).  So a spinlock is used to to protect the config list.

"cscfg_mutex" is a high level lock, my understanding is to protect the
high level operations from the Sysfs knobs, though sometimes it can
mitigate the race condition on configuration list mentioned above, but
the spinlock is the locking mechanism for the low level's config list
on a CoreSight device.

> However list_del() is special case because iterating config_csdev_list
> can be done without cscfg_mutex -- see
> cscfg_csdev_enable_active_config().
> This gurad with spinlock purpose to guard race unloading and
> get the config in cscfg_csdev_enable_active_config()
> (Please see my response below...).
> 
> the emptiness of config_csdev_list is guarded with cscfg_mutex.
> therefore, It seems enough to guard iterating part with spinlock :)

Mixed using cscfg_mutex and spinlock get code complexity, and I am a bit
concerned this is not best practice.

At a glance, I would say 'cscfg_mutex' is purposed to protect the global
'cscfg_mgr', per CoreSight device's config list should be protected by
'cscfg_csdev_lock'.

> > A side topic, as here it adds locks for protecting 'config_csdev_list',
> > I am wandering why we do not do the same thing for
> > 'feature_csdev_list' (See cscfg_remove_owned_csdev_features() and
> > cscfg_get_feat_csdev()).
> 
> In case of feature, It's okay since it couldn't be accessed when it
> gets failed to get related config.

I looked at cscfg_load_feat_csdev(), it uses 'cscfg_csdev_lock' to
protect the feature list.  This is why I thought the feature list also
need to be protected by the lock.  Now it is only partially protected.

> When we see cscfg_csdev_enable_active_config(), the config could be
> accessed without cscfg_mutex lock. so the config need to be guarded with
> spin_lock otherwise it could be acquired while unload module
> (after get active_cnt in search logic cscfg_csdev_enable_active_config()
> and other running unloading process)

To make things more clear, I have a questions.

'cscfg_mgr->sys_active_cnt' is used in the cscfg_unload_config_sets()
function to decide if can unload module, if any configuration is
active, why this variable cannot prevent unloading module?

Sorry for late replying.

Leo

> But feature list is depends on config, If config is safe from
> load/unload, this is not an issue so we don't need it.
> 
> Thanks for your review!

Re: [PATCH v4] coresight: prevent deactivate active config while enabling the config

[Yeoreum Yun]

Hi Leo,

>
> [...]
>
> > > > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > > @@ -1020,6 +1020,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
> > > >  	smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);
> > > >
> > > >  	raw_spin_unlock(&drvdata->spinlock);
> > > > +
> > > > +	cscfg_csdev_disable_active_config(csdev);
> > > > +
> > >
> > > In general, we need to split changes into several patches if each
> > > addresses a different issue.  From my understanding, the change above is
> > > to fix missing to disable config when disable Sysfs mode.
> > >
> > > If so, could we use a seperate patch for this change?
> > >
> >
> > It's not a differnt issue. Without this line, the active count wouldn't
> > decrese and it raise another issue -- unloadable moudle for active_cnt :(
> > So I think it should be included in this patch.
>
> I read the code again and concluded the change above is not related to
> locking and would be a separate issue: when we close a Sysfs session,
> we need to disable a config on a CoreSight device.
> Could you clarify what is meaning "unloadable moudle for active_cnt"?
> I only saw "cscfg_mgr->sys_active_cnt" is used for module unloading,
> but have no clue why "active_cnt" impacts module unloading.

Yes. but it also related "by this patch".
When the load config and "activate" configuration via sysfs,
not only the cscfg_mgr->sys_active_cnt is increase but also
"individual cscfg->active_cnt".
This patch extends the meaning of "cscfg->active_cnt" includes
"enable of configuraiton". so that cscfg_config_desc_put() prevent
decrease "module reference" while holding individual active_cnt.
That's why without this change, the "module reference" couldn't be
decreased. The problem before this change is deactivation doesn't
chekc cscfg->active_cnt but put module reference whenever it calls.

> > > >  	cpus_read_unlock();
> > > >
> > > >  	/*
> > > > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> > > > index a70c1454b410..6d8c212ad434 100644
> > > > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > > > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > > > @@ -391,14 +391,17 @@ static void cscfg_owner_put(struct cscfg_load_owner_info *owner_info)
> > > >  static void cscfg_remove_owned_csdev_configs(struct coresight_device *csdev, void *load_owner)
> > > >  {
> > > >  	struct cscfg_config_csdev *config_csdev, *tmp;
> > > > +	unsigned long flags;
> > > >
> > > >  	if (list_empty(&csdev->config_csdev_list))
> > > >  		return;
> > > >
> > > > +	raw_spin_lock_irqsave(&csdev->cscfg_csdev_lock, flags);
> > >
> > > I think we should use spinlock to guard the condition checking
> > > list_empty().
> > >
> > > Here the race condition is the 'config_csdev_list' list and
> > > configurations on the list.  For atomicity, we should use lock to
> > > protect any operations on the list (read, add, delete, etc).
> >
> > Interesting... Would you let me know which race it is?
> > here to check list_empty(), it already guarded with "cscfg_mutex".
>
> Thanks for pointing out this.  I read the code and understood that in
> some scenarios the list is protected by the mutex "cscfg_mutex".
>
> I would argue for using locking, we need to make clear for two thigns:
>
> - What is the race condition;
> - What locking is used to protect the race condition.
>
> For current case, a CoreSight device has a config list, the race
> condition is the config list will be manipulated by multiple places
> (e.g., for module loading / unloading, opening or closing a perf or
> SysFS session).  So a spinlock is used to to protect the config list.
>
> "cscfg_mutex" is a high level lock, my understanding is to protect the
> high level operations from the Sysfs knobs, though sometimes it can
> mitigate the race condition on configuration list mentioned above, but
> the spinlock is the locking mechanism for the low level's config list
> on a CoreSight device.

[...]

> > However list_del() is special case because iterating config_csdev_list
> > can be done without cscfg_mutex -- see
> > cscfg_csdev_enable_active_config().
> > This gurad with spinlock purpose to guard race unloading and
> > get the config in cscfg_csdev_enable_active_config()
> > (Please see my response below...).
> >
> > the emptiness of config_csdev_list is guarded with cscfg_mutex.
> > therefore, It seems enough to guard iterating part with spinlock :)
>
> Mixed using cscfg_mutex and spinlock get code complexity, and I am a bit
> concerned this is not best practice.
>
> At a glance, I would say 'cscfg_mutex' is purposed to protect the global
> 'cscfg_mgr', per CoreSight device's config list should be protected by
> 'cscfg_csdev_lock'.

Agree. BTW this patch is just for bugfix and consider to backport to
lower version.
I think it would be better to improve lock related things with another
patch if it's necessary.

> > > A side topic, as here it adds locks for protecting 'config_csdev_list',
> > > I am wandering why we do not do the same thing for
> > > 'feature_csdev_list' (See cscfg_remove_owned_csdev_features() and
> > > cscfg_get_feat_csdev()).
> >
> > In case of feature, It's okay since it couldn't be accessed when it
> > gets failed to get related config.
>
> I looked at cscfg_load_feat_csdev(), it uses 'cscfg_csdev_lock' to
> protect the feature list.  This is why I thought the feature list also
> need to be protected by the lock.  Now it is only partially protected.

I don't think the feature don't need.
Since when add the configuration. "feature first and config second"
and reference and remove is done with  "config first and feature second".

That menas if enable logic get configuration, we can gurantee the
liveness of each feature.

> > When we see cscfg_csdev_enable_active_config(), the config could be
> > accessed without cscfg_mutex lock. so the config need to be guarded with
> > spin_lock otherwise it could be acquired while unload module
> > (after get active_cnt in search logic cscfg_csdev_enable_active_config()
> > and other running unloading process)
>
> To make things more clear, I have a questions.
>
> 'cscfg_mgr->sys_active_cnt' is used in the cscfg_unload_config_sets()
> function to decide if can unload module, if any configuration is
> active, why this variable cannot prevent unloading module?

because cscfg_unload_config_sets() runs in "unload_modules" where
module reference is 0.

in the diagram. the enable get the configuration but other cpu do
deactivate and unload together.
Here, by other sys_active_cnt becames 0. while enable logic get the
config's pointer.


> > But feature list is depends on config, If config is safe from
> > load/unload, this is not an issue so we don't need it.
> >
> > Thanks for your review!

--
Sincerely,
Yeoreum Yun

Re: [PATCH v4] coresight: prevent deactivate active config while enabling the config

[Leo Yan]

Hi Levi,

On Fri, May 02, 2025 at 11:34:17AM +0100, Yeoreum Yun wrote:

[...]

> > > > > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > > > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > > > @@ -1020,6 +1020,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
> > > > >  	smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);
> > > > >
> > > > >  	raw_spin_unlock(&drvdata->spinlock);
> > > > > +
> > > > > +	cscfg_csdev_disable_active_config(csdev);
> > > > > +
> > > >
> > > > In general, we need to split changes into several patches if each
> > > > addresses a different issue.  From my understanding, the change above is
> > > > to fix missing to disable config when disable Sysfs mode.
> > > >
> > > > If so, could we use a seperate patch for this change?
> > > >
> > >
> > > It's not a differnt issue. Without this line, the active count wouldn't
> > > decrese and it raise another issue -- unloadable moudle for active_cnt :(
> > > So I think it should be included in this patch.
> >
> > I read the code again and concluded the change above is not related to
> > locking and would be a separate issue: when we close a Sysfs session,
> > we need to disable a config on a CoreSight device.
> > Could you clarify what is meaning "unloadable moudle for active_cnt"?
> > I only saw "cscfg_mgr->sys_active_cnt" is used for module unloading,
> > but have no clue why "active_cnt" impacts module unloading.
> 
> Yes. but it also related "by this patch".
> When the load config and "activate" configuration via sysfs,
> not only the cscfg_mgr->sys_active_cnt is increase but also
> "individual cscfg->active_cnt".
> This patch extends the meaning of "cscfg->active_cnt" includes
> "enable of configuraiton". so that cscfg_config_desc_put() prevent
> decrease "module reference" while holding individual active_cnt.
> That's why without this change, the "module reference" couldn't be
> decreased. The problem before this change is deactivation doesn't
> chekc cscfg->active_cnt but put module reference whenever it calls.

Thanks for explanation and it makes sense to me.

As we discussed, given this patch is relative big, let us divide into
three small patches for easier review.

- The first patch is to address that the sysfs interface misses to
  call cscfg_csdev_disable_active_config() for disabling config.

- The second patch fixes the locking issue for "config_csdev_list".
  As the "config_csdev_list" is protected by cscfg_csdev_lock, the
  cscfg_remove_owned_csdev_configs() function should use lock for
  exclusive operations.

- The third patch is to fix reference counter of a config module.
  The patch adds cscfg_config_desc_get() and cscfg_config_desc_put()
  in the config enabling / disabling flow for acquiring module
  reference counter.

Thanks,
Leo

Re: [PATCH v4] coresight: prevent deactivate active config while enabling the config

[Yeoreum Yun]

Hi Leo,

> [...]
>
> > > > > > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > > > > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > > > > @@ -1020,6 +1020,9 @@ static void etm4_disable_sysfs(struct coresight_device *csdev)
> > > > > >  	smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);
> > > > > >
> > > > > >  	raw_spin_unlock(&drvdata->spinlock);
> > > > > > +
> > > > > > +	cscfg_csdev_disable_active_config(csdev);
> > > > > > +
> > > > >
> > > > > In general, we need to split changes into several patches if each
> > > > > addresses a different issue.  From my understanding, the change above is
> > > > > to fix missing to disable config when disable Sysfs mode.
> > > > >
> > > > > If so, could we use a seperate patch for this change?
> > > > >
> > > >
> > > > It's not a differnt issue. Without this line, the active count wouldn't
> > > > decrese and it raise another issue -- unloadable moudle for active_cnt :(
> > > > So I think it should be included in this patch.
> > >
> > > I read the code again and concluded the change above is not related to
> > > locking and would be a separate issue: when we close a Sysfs session,
> > > we need to disable a config on a CoreSight device.
> > > Could you clarify what is meaning "unloadable moudle for active_cnt"?
> > > I only saw "cscfg_mgr->sys_active_cnt" is used for module unloading,
> > > but have no clue why "active_cnt" impacts module unloading.
> >
> > Yes. but it also related "by this patch".
> > When the load config and "activate" configuration via sysfs,
> > not only the cscfg_mgr->sys_active_cnt is increase but also
> > "individual cscfg->active_cnt".
> > This patch extends the meaning of "cscfg->active_cnt" includes
> > "enable of configuraiton". so that cscfg_config_desc_put() prevent
> > decrease "module reference" while holding individual active_cnt.
> > That's why without this change, the "module reference" couldn't be
> > decreased. The problem before this change is deactivation doesn't
> > chekc cscfg->active_cnt but put module reference whenever it calls.
>
> Thanks for explanation and it makes sense to me.
>
> As we discussed, given this patch is relative big, let us divide into
> three small patches for easier review.
>
> - The first patch is to address that the sysfs interface misses to
>   call cscfg_csdev_disable_active_config() for disabling config.
>
> - The second patch fixes the locking issue for "config_csdev_list".
>   As the "config_csdev_list" is protected by cscfg_csdev_lock, the
>   cscfg_remove_owned_csdev_configs() function should use lock for
>   exclusive operations.
>
> - The third patch is to fix reference counter of a config module.
>   The patch adds cscfg_config_desc_get() and cscfg_config_desc_put()
>   in the config enabling / disabling flow for acquiring module
>   reference counter.
>

Thanks for your review.
I'll separate the patchset according to your suggestion.

--
Sincerely,
Yeoreum Yun