Re: [PATCH 3/3] cpufreq: governor: Serialize governor callbacks

[Preeti U Murthy <preeti@linux.vnet.ibm.com>]

On 06/03/2015 03:57 PM, Viresh Kumar wrote:
> There are several races reported in cpufreq core around governors (only
> ondemand and conservative) by different people.
> 
> There are at least two race scenarios present in governor code:
>  (a) Concurrent access/updates of governor internal structures.
> 
>  It is possible that fields such as 'dbs_data->usage_count', etc.  are
>  accessed simultaneously for different policies using same governor
>  structure (i.e. CPUFREQ_HAVE_GOVERNOR_PER_POLICY flag unset). And
>  because of this we can dereference bad pointers.
> 
>  For example consider a system with two CPUs with separate 'struct
>  cpufreq_policy' instances. CPU0 governor: ondemand and CPU1: powersave.
>  CPU0 switching to powersave and CPU1 to ondemand:
> 	CPU0				CPU1
> 
> 	store*				store*
> 
> 	cpufreq_governor_exit()		cpufreq_governor_init()
> 					dbs_data = cdata->gdbs_data;
> 
> 	if (!--dbs_data->usage_count)
> 		kfree(dbs_data);
> 
> 					dbs_data->usage_count++;
> 					*Bad pointer dereference*
> 
>  There are other races possible between EXIT and START/STOP/LIMIT as
>  well. Its really complicated.
> 
>  (b) Switching governor state in bad sequence:
> 
>  For example trying to switch a governor to START state, when the
>  governor is in EXIT state. There are some checks present in
>  __cpufreq_governor() but they aren't sufficient as they compare events
>  against 'policy->governor_enabled', where as we need to take governor's
>  state into account, which can be used by multiple policies.
> 
> These two issues need to be solved separately and the responsibility
> should be properly divided between cpufreq and governor core.
> 
> The first problem is more about the governor core, as it needs to
> protect its structures properly. And the second problem should be fixed
> in cpufreq core instead of governor, as its all about sequence of
> events.
> 
> This patch is trying to solve only the first problem.
> 
> There are two types of data we need to protect,
> - 'struct common_dbs_data': No matter what, there is going to be a
>   single copy of this per governor.
> - 'struct dbs_data': With CPUFREQ_HAVE_GOVERNOR_PER_POLICY flag set, we
>   will have per-policy copy of this data, otherwise a single copy.
> 
> Because of such complexities, the mutex present in 'struct dbs_data' is
> insufficient to solve our problem. For example we need to protect
> fetching of 'dbs_data' from different structures at the beginning of
> cpufreq_governor_dbs(), to make sure it isn't currently being updated.
> 
> This can be fixed if we can guarantee serialization of event parsing
> code for an individual governor. This is best solved with a mutex per
> governor, and the placeholder for that is 'struct common_dbs_data'.
> 
> And so this patch moves the mutex from 'struct dbs_data' to 'struct
> common_dbs_data' and takes it at the beginning and drops it at the end
> of cpufreq_governor_dbs().
> 
> Tested with and without following configuration options:
> 
> CONFIG_LOCKDEP_SUPPORT=y
> CONFIG_DEBUG_RT_MUTEXES=y
> CONFIG_DEBUG_PI_LIST=y
> CONFIG_DEBUG_SPINLOCK=y
> CONFIG_DEBUG_MUTEXES=y
> CONFIG_DEBUG_LOCK_ALLOC=y
> CONFIG_PROVE_LOCKING=y
> CONFIG_LOCKDEP=y
> CONFIG_DEBUG_ATOMIC_SLEEP=y
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>