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 for Android: free password hash cracker in your pocket

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

Message-ID: <Z2qx9B2v7WC79HyX@e129823.arm.com>
Date: Tue, 24 Dec 2024 13:07:00 +0000
From: Yeoreum Yun <yeoreum.yun@....com>
To: James Clark <james.clark@...aro.org>
Cc: Mike Leach <mike.leach@...aro.org>, 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

Hi James,

> > 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.

I think I'm not fully explained. sys_enable_cnt could be more than 2
if it runs with perf and sysfs both.
Because, perf uses cscfg_active_config() to activate configuration.
but, "sysfs" only can activate 1 configuration because it enables with
cscfg_config_sysfs_activate(). so the sys_enable_cnt could be more than
However, sys_enable_cnt can be increased only 1 by sysfs interface.

Maybe It could be hinder to disable by enabled activated by the sysfs.
However, That's wouldn't be critical.

>
> 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.

Actually, I think merging this two count (or with module ref too),
seems increasing commplexity right now.
To make clear It would be good However to fix bug for above case,
I think it's enough to add sys_enable_cnt and I think it doesn't loss
its name meaning and it seems more to be backportable.

> 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.

Agree, That's why there's one option to merge cscfg_mutex with
coresight_mutex. But I think this is too much to fix this problem.

Thanks.