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

[PATCH v5] 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,
  Patch #1 fix missing disable config when etm4 device is disabled via sysfs
  Patch #2 holding cscfg_csdev_lock while removing config from cscfg_csdsev_list
  Patch #3 add wrapper to prevent deactivation of config whlie enabling it

Patch History
==============
From v4 to v5
  - separate patch set (from Leo Yan)
  - https://lore.kernel.org/all/20250324191740.64964-1-yeoreum.yun@arm.com/


Yeoreum Yun (3):
  coresight/etm4: fix missing disable active config
  coresight: holding cscfg_csdev_lock while removing cscfg from csdev
  coresight: prevent deactivate active config while enabling the config

 .../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(-)

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

[PATCH v5 1/3] coresight/etm4: fix missing disable active config

[Yeoreum Yun]

When etm4 device is disabled via sysfs, it should disable its active
count.

Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 2b8f10463840..f008f87f0801 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();
 
 	/*
-- 
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

[PATCH v5 2/3] coresight: holding cscfg_csdev_lock while removing cscfg from csdev

[Yeoreum Yun]

There'll be possible race scenario for coresight config:

CPU0                                          CPU1
(perf enable)                                 load module
                                              cscfg_load_config_sets()
                                              activate config. // sysfs
                                              (sys_active_cnt == 1)
...
cscfg_csdev_enable_active_config()
  lock(csdev->cscfg_csdev_lock)
                                              deactivate config // sysfs
                                              (sys_activec_cnt == 0)
                                              cscfg_unload_config_sets()
  <iterating config_csdev_list>               cscfg_remove_owned_csdev_configs()
  // here load config activate by CPU1
  unlock(csdev->cscfg_csdev_lock)

iterating config_csdev_list could be raced with config_csdev_list's
entry delete.

To resolve this race , hold csdev->cscfg_csdev_lock() while
cscfg_remove_owned_csdev_configs()

Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
Fixes: 02bd588e12df ("coresight: configuration: Update API to permit dynamic load/unload")
---
 drivers/hwtracing/coresight/coresight-syscfg.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
index a70c1454b410..5d194b9269f5 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)
-- 
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

[PATCH v5 3/3] 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.
cscfg_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>
---
 .../hwtracing/coresight/coresight-config.h    |  2 +-
 .../hwtracing/coresight/coresight-syscfg.c    | 49 +++++++++++++------
 2 files changed, 35 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-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
index 5d194b9269f5..6d8c212ad434 100644
--- a/drivers/hwtracing/coresight/coresight-syscfg.c
+++ b/drivers/hwtracing/coresight/coresight-syscfg.c
@@ -870,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.
@@ -893,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;
@@ -923,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;
 		}
@@ -1050,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;
 
@@ -1065,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;
@@ -1100,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);
@@ -1139,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 v5 1/3] coresight/etm4: fix missing disable active config

[Leo Yan]

On Tue, May 13, 2025 at 06:06:20PM +0100, Yeoreum Yun wrote:
> When etm4 device is disabled via sysfs, it should disable its active
> count.
> 
> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>

With adding fix tag:

Fixes: 7ebd0ec6cf94 ("coresight: configfs: Allow configfs to activate configuration")

Reviewed-by: Leo Yan <leo.yan@arm.com>

> ---
>  drivers/hwtracing/coresight/coresight-etm4x-core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 2b8f10463840..f008f87f0801 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();
>  
>  	/*
> -- 
> LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
> 

Re: [PATCH v5 2/3] coresight: holding cscfg_csdev_lock while removing cscfg from csdev

[Leo Yan]

On Tue, May 13, 2025 at 06:06:21PM +0100, Yeoreum Yun wrote:
> There'll be possible race scenario for coresight config:
> 
> CPU0                                          CPU1
> (perf enable)                                 load module
>                                               cscfg_load_config_sets()
>                                               activate config. // sysfs
>                                               (sys_active_cnt == 1)
> ...
> cscfg_csdev_enable_active_config()
>   lock(csdev->cscfg_csdev_lock)
>                                               deactivate config // sysfs
>                                               (sys_activec_cnt == 0)
>                                               cscfg_unload_config_sets()
>   <iterating config_csdev_list>               cscfg_remove_owned_csdev_configs()
>   // here load config activate by CPU1
>   unlock(csdev->cscfg_csdev_lock)
> 
> iterating config_csdev_list could be raced with config_csdev_list's
> entry delete.
> 
> To resolve this race , hold csdev->cscfg_csdev_lock() while
> cscfg_remove_owned_csdev_configs()
> 
> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> Fixes: 02bd588e12df ("coresight: configuration: Update API to permit dynamic load/unload")
> ---
>  drivers/hwtracing/coresight/coresight-syscfg.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> index a70c1454b410..5d194b9269f5 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);

Could we use the format:

   guard(raw_spinlock_irqsave)(&csdev->cscfg_csdev_lock);

Sorry I did not mention this in the earlier review.  Otherwise:

Reviewed-by: Leo Yan <leo.yan@arm.com>

>  	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)
> -- 
> LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
> 

Re: [PATCH v5 3/3] coresight: prevent deactivate active config while enabling the config

[Leo Yan]

On Tue, May 13, 2025 at 06:06:22PM +0100, 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.
> cscfg_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>
> ---
>  .../hwtracing/coresight/coresight-config.h    |  2 +-
>  .../hwtracing/coresight/coresight-syscfg.c    | 49 +++++++++++++------
>  2 files changed, 35 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-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> index 5d194b9269f5..6d8c212ad434 100644
> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> @@ -870,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)

I would like to change the return type to int, so the error is handled
within the function.  As a result, the caller _cscfg_activate_config()
does not need to explicitly return an error value.

Otherwise, the patch looks good to me.

Thanks,
Leo

> +{
> +	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.
> @@ -893,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;
> @@ -923,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;
>  		}
> @@ -1050,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;
>  
> @@ -1065,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;
> @@ -1100,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);
> @@ -1139,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 v5 3/3] coresight: prevent deactivate active config while enabling the config

[Leo Yan]

On Wed, May 14, 2025 at 10:30:19AM +0100, Leo Yan wrote:
> On Tue, May 13, 2025 at 06:06:22PM +0100, 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.
> > cscfg_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>

And please add a fix tag:

Fixes: f8cce2ff3c04 ("coresight: syscfg: Add API to activate and enable configurations")

> > ---
> >  .../hwtracing/coresight/coresight-config.h    |  2 +-
> >  .../hwtracing/coresight/coresight-syscfg.c    | 49 +++++++++++++------
> >  2 files changed, 35 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-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> > index 5d194b9269f5..6d8c212ad434 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > @@ -870,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)
> 
> I would like to change the return type to int, so the error is handled
> within the function.  As a result, the caller _cscfg_activate_config()
> does not need to explicitly return an error value.
> 
> Otherwise, the patch looks good to me.
> 
> Thanks,
> Leo
> 
> > +{
> > +	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.
> > @@ -893,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;
> > @@ -923,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;
> >  		}
> > @@ -1050,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;
> >  
> > @@ -1065,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;
> > @@ -1100,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);
> > @@ -1139,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}
> > 
> _______________________________________________
> CoreSight mailing list -- coresight@lists.linaro.org
> To unsubscribe send an email to coresight-leave@lists.linaro.org

Re: [PATCH v5 3/3] coresight: prevent deactivate active config while enabling the config

[Yeoreum Yun]

Hi Leo,

> On Tue, May 13, 2025 at 06:06:22PM +0100, 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.
> > cscfg_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>
> > ---
> >  .../hwtracing/coresight/coresight-config.h    |  2 +-
> >  .../hwtracing/coresight/coresight-syscfg.c    | 49 +++++++++++++------
> >  2 files changed, 35 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-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> > index 5d194b9269f5..6d8c212ad434 100644
> > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > @@ -870,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)
>
> I would like to change the return type to int, so the error is handled
> within the function.  As a result, the caller _cscfg_activate_config()
> does not need to explicitly return an error value.

I think it's not good since cscfg_config_desc_get() failed only when
try_module_get() failed and its return type is "bool".
also, If we make defines error type in here it would make different
error code compared before this patch in some function
(i.e) cscfg_csdev_enable_active_config() want to receive -EBUSY when
cscfg_config_desc_get() failed but _cscfg_activate_config() want to
return -EINVAL.

So I think it would be good to sustain current return type.

>
> Otherwise, the patch looks good to me.

Thanks ;)

--
Sincerely,
Yeoreum Yun

Re: [PATCH v5 2/3] coresight: holding cscfg_csdev_lock while removing cscfg from csdev

[Yeoreum Yun]

Hi Leo,

> On Tue, May 13, 2025 at 06:06:21PM +0100, Yeoreum Yun wrote:
> > There'll be possible race scenario for coresight config:
> >
> > CPU0                                          CPU1
> > (perf enable)                                 load module
> >                                               cscfg_load_config_sets()
> >                                               activate config. // sysfs
> >                                               (sys_active_cnt == 1)
> > ...
> > cscfg_csdev_enable_active_config()
> >   lock(csdev->cscfg_csdev_lock)
> >                                               deactivate config // sysfs
> >                                               (sys_activec_cnt == 0)
> >                                               cscfg_unload_config_sets()
> >   <iterating config_csdev_list>               cscfg_remove_owned_csdev_configs()
> >   // here load config activate by CPU1
> >   unlock(csdev->cscfg_csdev_lock)
> >
> > iterating config_csdev_list could be raced with config_csdev_list's
> > entry delete.
> >
> > To resolve this race , hold csdev->cscfg_csdev_lock() while
> > cscfg_remove_owned_csdev_configs()
> >
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> > Fixes: 02bd588e12df ("coresight: configuration: Update API to permit dynamic load/unload")
> > ---
> >  drivers/hwtracing/coresight/coresight-syscfg.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> > index a70c1454b410..5d194b9269f5 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);
>
> Could we use the format:
>
>    guard(raw_spinlock_irqsave)(&csdev->cscfg_csdev_lock);

No problem. I'll convert with guard.

> Sorry I did not mention this in the earlier review.  Otherwise:
>
> Reviewed-by: Leo Yan <leo.yan@arm.com>

Thanks :)
>
> >  	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)
> > --
> > LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
> >

--
Sincerely,
Yeoreum Yun

Re: [PATCH v5 3/3] coresight: prevent deactivate active config while enabling the config

[Yeoreum Yun]

Hi Leo,

> On Wed, May 14, 2025 at 10:30:19AM +0100, Leo Yan wrote:
> > On Tue, May 13, 2025 at 06:06:22PM +0100, 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.
> > > cscfg_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>
>
> And please add a fix tag:
>
> Fixes: f8cce2ff3c04 ("coresight: syscfg: Add API to activate and enable configurations")

Sorry to annoying you. Thanks ;)

>
> > > ---
> > >  .../hwtracing/coresight/coresight-config.h    |  2 +-
> > >  .../hwtracing/coresight/coresight-syscfg.c    | 49 +++++++++++++------
> > >  2 files changed, 35 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-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
> > > index 5d194b9269f5..6d8c212ad434 100644
> > > --- a/drivers/hwtracing/coresight/coresight-syscfg.c
> > > +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
> > > @@ -870,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)
> >
> > I would like to change the return type to int, so the error is handled
> > within the function.  As a result, the caller _cscfg_activate_config()
> > does not need to explicitly return an error value.
> >
> > Otherwise, the patch looks good to me.
> >
> > Thanks,
> > Leo
> >
> > > +{
> > > +	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.
> > > @@ -893,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;
> > > @@ -923,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;
> > >  		}
> > > @@ -1050,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;
> > >
> > > @@ -1065,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;
> > > @@ -1100,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);
> > > @@ -1139,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}
> > >
> > _______________________________________________
> > CoreSight mailing list -- coresight@lists.linaro.org
> > To unsubscribe send an email to coresight-leave@lists.linaro.org

--
Sincerely,
Yeoreum Yun

Re: [PATCH v5 1/3] coresight/etm4: fix missing disable active config

[Yeoreum Yun]

> On Tue, May 13, 2025 at 06:06:20PM +0100, Yeoreum Yun wrote:
> > When etm4 device is disabled via sysfs, it should disable its active
> > count.
> >
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
>
> With adding fix tag:
>
> Fixes: 7ebd0ec6cf94 ("coresight: configfs: Allow configfs to activate configuration")
>
> Reviewed-by: Leo Yan <leo.yan@arm.com>

Thanks!
>
> > ---
> >  drivers/hwtracing/coresight/coresight-etm4x-core.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index 2b8f10463840..f008f87f0801 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();
> >
> >  	/*
> > --
> > LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
> >

--
Sincerely,
Yeoreum Yun

Re: [PATCH v5 3/3] coresight: prevent deactivate active config while enabling the config

[Leo Yan]

Hi Levi,

On Wed, May 14, 2025 at 12:04:39PM +0100, Yeoreum Yun wrote:

[...]


> > > +static bool cscfg_config_desc_get(struct cscfg_config_desc *config_desc)
> >
> > I would like to change the return type to int, so the error is handled
> > within the function.  As a result, the caller _cscfg_activate_config()
> > does not need to explicitly return an error value.
> 
> I think it's not good since cscfg_config_desc_get() failed only when
> try_module_get() failed and its return type is "bool".

Understood.  I thought it would be easier for later maintenance if we
encapsulate error handling in the function, but I don't have strong
opinion.  It is fine for me to return bool type.

Thanks,
Leo