Re: [PATCH] cpufreq: Use struct kobj_attribute instead of struct global_attr

[Rafael J. Wysocki]

On Friday, January 25, 2019 8:23:07 AM CET Viresh Kumar wrote:
> The cpufreq_global_kobject is created using kobject_create_and_add()
> helper, which assigns the kobj_type as dynamic_kobj_ktype and show/store
> routines are set to kobj_attr_show() and kobj_attr_store().
> 
> These routines pass struct kobj_attribute as an argument to the
> show/store callbacks. But all the cpufreq files created using the
> cpufreq_global_kobject expect the argument to be of type struct
> attribute. Things work fine currently as no one accesses the "attr"
> argument. We may not see issues even if the argument is used, as struct
> kobj_attribute has struct attribute as its first element and so they
> will both get same address.
> 
> But this is logically incorrect and we should rather use struct
> kobj_attribute instead of struct global_attr in the cpufreq core and
> drivers and the show/store callbacks should take struct kobj_attribute
> as argument instead.
> 
> This bug is caught using CFI CLANG builds in android kernel which
> catches mismatch in function prototypes for such callbacks.
> 
> Reported-by: Donghee Han <dh.han@xxxxxxxxxxx>
> Reported-by: Sangkyu Kim <skwith.kim@xxxxxxxxxxx>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>  drivers/cpufreq/cpufreq.c      |  6 +++---
>  drivers/cpufreq/intel_pstate.c | 23 ++++++++++++-----------
>  include/linux/cpufreq.h        | 12 ++----------
>  3 files changed, 17 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index e35a886e00bc..ef0e33e21b98 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -545,13 +545,13 @@ EXPORT_SYMBOL_GPL(cpufreq_policy_transition_delay_us);
>   *                          SYSFS INTERFACE                          *
>   *********************************************************************/
>  static ssize_t show_boost(struct kobject *kobj,
> -				 struct attribute *attr, char *buf)
> +			  struct kobj_attribute *attr, char *buf)
>  {
>  	return sprintf(buf, "%d\n", cpufreq_driver->boost_enabled);
>  }
>  
> -static ssize_t store_boost(struct kobject *kobj, struct attribute *attr,
> -				  const char *buf, size_t count)
> +static ssize_t store_boost(struct kobject *kobj, struct kobj_attribute *attr,
> +			   const char *buf, size_t count)
>  {
>  	int ret, enable;
>  
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index dd66decf2087..5ab6a4fe93aa 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -895,7 +895,7 @@ static void intel_pstate_update_policies(void)
>  /************************** sysfs begin ************************/
>  #define show_one(file_name, object)					\
>  	static ssize_t show_##file_name					\
> -	(struct kobject *kobj, struct attribute *attr, char *buf)	\
> +	(struct kobject *kobj, struct kobj_attribute *attr, char *buf)	\
>  	{								\
>  		return sprintf(buf, "%u\n", global.object);		\
>  	}
> @@ -904,7 +904,7 @@ static ssize_t intel_pstate_show_status(char *buf);
>  static int intel_pstate_update_status(const char *buf, size_t size);
>  
>  static ssize_t show_status(struct kobject *kobj,
> -			   struct attribute *attr, char *buf)
> +			   struct kobj_attribute *attr, char *buf)
>  {
>  	ssize_t ret;
>  
> @@ -915,7 +915,7 @@ static ssize_t show_status(struct kobject *kobj,
>  	return ret;
>  }
>  
> -static ssize_t store_status(struct kobject *a, struct attribute *b,
> +static ssize_t store_status(struct kobject *a, struct kobj_attribute *b,
>  			    const char *buf, size_t count)
>  {
>  	char *p = memchr(buf, '\n', count);
> @@ -929,7 +929,7 @@ static ssize_t store_status(struct kobject *a, struct attribute *b,
>  }
>  
>  static ssize_t show_turbo_pct(struct kobject *kobj,
> -				struct attribute *attr, char *buf)
> +				struct kobj_attribute *attr, char *buf)
>  {
>  	struct cpudata *cpu;
>  	int total, no_turbo, turbo_pct;
> @@ -955,7 +955,7 @@ static ssize_t show_turbo_pct(struct kobject *kobj,
>  }
>  
>  static ssize_t show_num_pstates(struct kobject *kobj,
> -				struct attribute *attr, char *buf)
> +				struct kobj_attribute *attr, char *buf)
>  {
>  	struct cpudata *cpu;
>  	int total;
> @@ -976,7 +976,7 @@ static ssize_t show_num_pstates(struct kobject *kobj,
>  }
>  
>  static ssize_t show_no_turbo(struct kobject *kobj,
> -			     struct attribute *attr, char *buf)
> +			     struct kobj_attribute *attr, char *buf)
>  {
>  	ssize_t ret;
>  
> @@ -998,7 +998,7 @@ static ssize_t show_no_turbo(struct kobject *kobj,
>  	return ret;
>  }
>  
> -static ssize_t store_no_turbo(struct kobject *a, struct attribute *b,
> +static ssize_t store_no_turbo(struct kobject *a, struct kobj_attribute *b,
>  			      const char *buf, size_t count)
>  {
>  	unsigned int input;
> @@ -1045,7 +1045,7 @@ static ssize_t store_no_turbo(struct kobject *a, struct attribute *b,
>  	return count;
>  }
>  
> -static ssize_t store_max_perf_pct(struct kobject *a, struct attribute *b,
> +static ssize_t store_max_perf_pct(struct kobject *a, struct kobj_attribute *b,
>  				  const char *buf, size_t count)
>  {
>  	unsigned int input;
> @@ -1075,7 +1075,7 @@ static ssize_t store_max_perf_pct(struct kobject *a, struct attribute *b,
>  	return count;
>  }
>  
> -static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b,
> +static ssize_t store_min_perf_pct(struct kobject *a, struct kobj_attribute *b,
>  				  const char *buf, size_t count)
>  {
>  	unsigned int input;
> @@ -1107,12 +1107,13 @@ static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b,
>  }
>  
>  static ssize_t show_hwp_dynamic_boost(struct kobject *kobj,
> -				struct attribute *attr, char *buf)
> +				struct kobj_attribute *attr, char *buf)
>  {
>  	return sprintf(buf, "%u\n", hwp_boost);
>  }
>  
> -static ssize_t store_hwp_dynamic_boost(struct kobject *a, struct attribute *b,
> +static ssize_t store_hwp_dynamic_boost(struct kobject *a,
> +				       struct kobj_attribute *b,
>  				       const char *buf, size_t count)
>  {
>  	unsigned int input;
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index c86d6d8bdfed..0b427d5df0fe 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -254,20 +254,12 @@ __ATTR(_name, 0644, show_##_name, store_##_name)
>  static struct freq_attr _name =			\
>  __ATTR(_name, 0200, NULL, store_##_name)
>  
> -struct global_attr {
> -	struct attribute attr;
> -	ssize_t (*show)(struct kobject *kobj,
> -			struct attribute *attr, char *buf);
> -	ssize_t (*store)(struct kobject *a, struct attribute *b,
> -			 const char *c, size_t count);
> -};
> -
>  #define define_one_global_ro(_name)		\
> -static struct global_attr _name =		\
> +static struct kobj_attribute _name =		\
>  __ATTR(_name, 0444, show_##_name, NULL)
>  
>  #define define_one_global_rw(_name)		\
> -static struct global_attr _name =		\
> +static struct kobj_attribute _name =		\
>  __ATTR(_name, 0644, show_##_name, store_##_name)
>  
>  
> 

Applied, thanks!