linux-kernel - Re: [PATCH 1/1] coresight: prevent deactivate active config while enable the config

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <79e11033-181a-418a-a56f-068b9ca62f56@linaro.org>
Date: Tue, 24 Dec 2024 11:41:43 +0000
From: James Clark <james.clark@...aro.org>
To: Yeoreum Yun <yeoreum.yun@....com>, Mike Leach <mike.leach@...aro.org>
Cc: coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, nd@....com, suzuki.poulose@....com,
 alexander.shishkin@...ux.intel.com
Subject: Re: [PATCH 1/1] coresight: prevent deactivate active config while
 enable the config

On 24/12/2024 10:13 am, Yeoreum Yun wrote:
> Hi James.
>>> diff --git a/drivers/hwtracing/coresight/coresight-syscfg.c b/drivers/hwtracing/coresight/coresight-syscfg.c
>>> index a70c1454b410..dfa7dcbaf25d 100644
>>> --- a/drivers/hwtracing/coresight/coresight-syscfg.c
>>> +++ b/drivers/hwtracing/coresight/coresight-syscfg.c
>>> @@ -953,7 +953,8 @@ int cscfg_config_sysfs_activate(struct cscfg_config_desc *config_desc, bool acti
>>>    			cscfg_mgr->sysfs_active_config = cfg_hash;
>>>    	} else {
>>>    		/* disable if matching current value */
>>> -		if (cscfg_mgr->sysfs_active_config == cfg_hash) {
>>> +		if (cscfg_mgr->sysfs_active_config == cfg_hash &&
>>> +		    !atomic_read(&cscfg_mgr->sys_enable_cnt)) {
>>>    			_cscfg_deactivate_config(cfg_hash);
>>
>> So is sys_enable_cnt a global value? If a fix is needed doesn't it need to
>> be a per-config refcount?
>>
>> Say you have two active configs, sys_enable_cnt is now 2, how do you disable
>> one without it always skipping here when the other config is enabled?
> 
> Sorry to miss this one!.
> Because when one configuration is enabled,
> cscfg_mgr->sysfs_active_config becomes !NULL, so it wouldn't happen
> there is no two activate configurations. so sys_enable_cnt wouldn't be
> 2.
> 
> 
> 

Maybe "sys_enabled" is a better name then. Count implies that it can be 
more than one. And the doc could be updated to say it's only ever 0 or 1.

But what about my other point about enabled always being a subset of 
active? Can we not change "sys_active_cnt" to a more generic "refcount", 
then both activation and enabling steps increment that same refcount, 
because they are both technically users of the config. Then you can 
solve the problem without adding another separate counter. I think 
that's potentially easier to understand.

Although the easiest is just locking every function with the mutex (or a 
spinlock if it also needs to be used from Perf). Obviously all these 
atomics are harder to get right than that, and this isn't performance 
sensitive in any way.