* Re: [PATCH] platform/x86: intel-uncore-freq: Fix types in sysfs callbacks
2024-01-04 22:59 [PATCH] platform/x86: intel-uncore-freq: Fix types in sysfs callbacks Nathan Chancellor
@ 2024-01-04 23:31 ` Sami Tolvanen
2024-01-05 3:42 ` srinivas pandruvada
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Sami Tolvanen @ 2024-01-04 23:31 UTC (permalink / raw)
To: Nathan Chancellor
Cc: srinivas.pandruvada, hdegoede, ilpo.jarvinen, keescook,
platform-driver-x86, llvm, patches
Hi Nathan,
On Thu, Jan 4, 2024 at 2:59 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> When booting a kernel with CONFIG_CFI_CLANG, there is a CFI failure when
> accessing any of the values under
> /sys/devices/system/cpu/intel_uncore_frequency/package_00_die_00:
>
> $ cat /sys/devices/system/cpu/intel_uncore_frequency/package_00_die_00/max_freq_khz
> fish: Job 1, 'cat /sys/devices/system/cpu/int…' terminated by signal SIGSEGV (Address boundary error)
>
> $ sudo dmesg &| grep 'CFI failure'
> [ 170.953925] CFI failure at kobj_attr_show+0x19/0x30 (target: show_max_freq_khz+0x0/0xc0 [intel_uncore_frequency_common]; expected type: 0xd34078c5
>
> The sysfs callback functions such as show_domain_id() are written as if
> they are going to be called by dev_attr_show() but as the above message
> shows, they are instead called by kobj_attr_show(). kCFI checks that the
> destination of an indirect jump has the exact same type as the prototype
> of the function pointer it is called through and fails when they do not.
>
> These callbacks are called through kobj_attr_show() because
> uncore_root_kobj was initialized with kobject_create_and_add(), which
> means uncore_root_kobj has a ->sysfs_ops of kobj_sysfs_ops from
> kobject_create(), which uses kobj_attr_show() as its ->show() value.
>
> The only reason there has not been a more noticeable problem until this
> point is that 'struct kobj_attribute' and 'struct device_attribute' have
> the same layout, so getting the callback from container_of() works the
> same with either value.
>
> Change all the callbacks and their uses to be compatible with
> kobj_attr_show() and kobj_attr_store(), which resolves the kCFI failure
> and allows the sysfs files to work properly.
>
> Closes: https://github.com/ClangBuiltLinux/linux/issues/1974
> Fixes: ae7b2ce57851 ("platform/x86/intel/uncore-freq: Use sysfs API to create attributes")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
> I think I got the fixes tag right. I only started seeing this because of
> commit 27f2b08735c9 ("platform/x86: intel-uncore-freq: Add additional
> client processors"), which allows this driver to load on my i7-11700
> test system but I think the commit in the Fixes tag incorrectly changes
> the types of the callbacks.
> ---
> .../uncore-frequency/uncore-frequency-common.c | 82 +++++++++++-----------
> .../uncore-frequency/uncore-frequency-common.h | 32 ++++-----
> 2 files changed, 57 insertions(+), 57 deletions(-)
Looks good to me. Thanks for the patch!
Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
Sami
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] platform/x86: intel-uncore-freq: Fix types in sysfs callbacks
2024-01-04 22:59 [PATCH] platform/x86: intel-uncore-freq: Fix types in sysfs callbacks Nathan Chancellor
2024-01-04 23:31 ` Sami Tolvanen
@ 2024-01-05 3:42 ` srinivas pandruvada
2024-01-08 7:25 ` srinivas pandruvada
2024-01-08 14:52 ` Hans de Goede
3 siblings, 0 replies; 6+ messages in thread
From: srinivas pandruvada @ 2024-01-05 3:42 UTC (permalink / raw)
To: Nathan Chancellor, hdegoede, ilpo.jarvinen
Cc: keescook, samitolvanen, platform-driver-x86, llvm, patches
Hi Nathan,
On vacation. Will check the patch next week.
Thanks,
Srinivas
On Thu, 2024-01-04 at 15:59 -0700, Nathan Chancellor wrote:
> When booting a kernel with CONFIG_CFI_CLANG, there is a CFI failure
> when
> accessing any of the values under
> /sys/devices/system/cpu/intel_uncore_frequency/package_00_die_00:
>
> $ cat
> /sys/devices/system/cpu/intel_uncore_frequency/package_00_die_00/max_
> freq_khz
> fish: Job 1, 'cat /sys/devices/system/cpu/int…' terminated by
> signal SIGSEGV (Address boundary error)
>
> $ sudo dmesg &| grep 'CFI failure'
> [ 170.953925] CFI failure at kobj_attr_show+0x19/0x30 (target:
> show_max_freq_khz+0x0/0xc0 [intel_uncore_frequency_common]; expected
> type: 0xd34078c5
>
> The sysfs callback functions such as show_domain_id() are written as
> if
> they are going to be called by dev_attr_show() but as the above
> message
> shows, they are instead called by kobj_attr_show(). kCFI checks that
> the
> destination of an indirect jump has the exact same type as the
> prototype
> of the function pointer it is called through and fails when they do
> not.
>
> These callbacks are called through kobj_attr_show() because
> uncore_root_kobj was initialized with kobject_create_and_add(), which
> means uncore_root_kobj has a ->sysfs_ops of kobj_sysfs_ops from
> kobject_create(), which uses kobj_attr_show() as its ->show() value.
>
> The only reason there has not been a more noticeable problem until
> this
> point is that 'struct kobj_attribute' and 'struct device_attribute'
> have
> the same layout, so getting the callback from container_of() works
> the
> same with either value.
>
> Change all the callbacks and their uses to be compatible with
> kobj_attr_show() and kobj_attr_store(), which resolves the kCFI
> failure
> and allows the sysfs files to work properly.
>
> Closes: https://github.com/ClangBuiltLinux/linux/issues/1974
> Fixes: ae7b2ce57851 ("platform/x86/intel/uncore-freq: Use sysfs API
> to create attributes")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
> I think I got the fixes tag right. I only started seeing this because
> of
> commit 27f2b08735c9 ("platform/x86: intel-uncore-freq: Add additional
> client processors"), which allows this driver to load on my i7-11700
> test system but I think the commit in the Fixes tag incorrectly
> changes
> the types of the callbacks.
> ---
> .../uncore-frequency/uncore-frequency-common.c | 82 +++++++++++-
> ----------
> .../uncore-frequency/uncore-frequency-common.h | 32 ++++-----
> 2 files changed, 57 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-
> frequency-common.c b/drivers/platform/x86/intel/uncore-
> frequency/uncore-frequency-common.c
> index 33ab207493e3..33bb58dc3f78 100644
> --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> common.c
> +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> common.c
> @@ -23,23 +23,23 @@ static int (*uncore_read)(struct uncore_data
> *data, unsigned int *min, unsigned
> static int (*uncore_write)(struct uncore_data *data, unsigned int
> input, unsigned int min_max);
> static int (*uncore_read_freq)(struct uncore_data *data, unsigned
> int *freq);
>
> -static ssize_t show_domain_id(struct device *dev, struct
> device_attribute *attr, char *buf)
> +static ssize_t show_domain_id(struct kobject *kobj, struct
> kobj_attribute *attr, char *buf)
> {
> - struct uncore_data *data = container_of(attr, struct
> uncore_data, domain_id_dev_attr);
> + struct uncore_data *data = container_of(attr, struct
> uncore_data, domain_id_kobj_attr);
>
> return sprintf(buf, "%u\n", data->domain_id);
> }
>
> -static ssize_t show_fabric_cluster_id(struct device *dev, struct
> device_attribute *attr, char *buf)
> +static ssize_t show_fabric_cluster_id(struct kobject *kobj, struct
> kobj_attribute *attr, char *buf)
> {
> - struct uncore_data *data = container_of(attr, struct
> uncore_data, fabric_cluster_id_dev_attr);
> + struct uncore_data *data = container_of(attr, struct
> uncore_data, fabric_cluster_id_kobj_attr);
>
> return sprintf(buf, "%u\n", data->cluster_id);
> }
>
> -static ssize_t show_package_id(struct device *dev, struct
> device_attribute *attr, char *buf)
> +static ssize_t show_package_id(struct kobject *kobj, struct
> kobj_attribute *attr, char *buf)
> {
> - struct uncore_data *data = container_of(attr, struct
> uncore_data, package_id_dev_attr);
> + struct uncore_data *data = container_of(attr, struct
> uncore_data, package_id_kobj_attr);
>
> return sprintf(buf, "%u\n", data->package_id);
> }
> @@ -97,30 +97,30 @@ static ssize_t show_perf_status_freq_khz(struct
> uncore_data *data, char *buf)
> }
>
> #define store_uncore_min_max(name,
> min_max) \
> - static ssize_t store_##name(struct device *dev, \
> - struct device_attribute
> *attr, \
> + static ssize_t store_##name(struct kobject
> *kobj, \
> + struct kobj_attribute
> *attr, \
> const char *buf, size_t
> count) \
> {
> \
> - struct uncore_data *data = container_of(attr, struct
> uncore_data, name##_dev_attr);\
> + struct uncore_data *data = container_of(attr, struct
> uncore_data, name##_kobj_attr);\
>
> \
> return store_min_max_freq_khz(data, buf, count, \
> min_max); \
> }
>
> #define show_uncore_min_max(name,
> min_max) \
> - static ssize_t show_##name(struct device *dev, \
> - struct device_attribute *attr,
> char *buf)\
> + static ssize_t show_##name(struct kobject
> *kobj, \
> + struct kobj_attribute *attr, char
> *buf)\
> {
> \
> - struct uncore_data *data = container_of(attr, struct
> uncore_data, name##_dev_attr);\
> + struct uncore_data *data = container_of(attr, struct
> uncore_data, name##_kobj_attr);\
>
> \
> return show_min_max_freq_khz(data, buf,
> min_max); \
> }
>
> #define
> show_uncore_perf_status(name) \
> - static ssize_t show_##name(struct device *dev, \
> - struct device_attribute *attr,
> char *buf)\
> + static ssize_t show_##name(struct kobject
> *kobj, \
> + struct kobj_attribute *attr, char
> *buf)\
> {
> \
> - struct uncore_data *data = container_of(attr, struct
> uncore_data, name##_dev_attr);\
> + struct uncore_data *data = container_of(attr, struct
> uncore_data, name##_kobj_attr);\
>
> \
> return show_perf_status_freq_khz(data, buf); \
> }
> @@ -134,11 +134,11 @@ show_uncore_min_max(max_freq_khz, 1);
> show_uncore_perf_status(current_freq_khz);
>
> #define
> show_uncore_data(member_name) \
> - static ssize_t show_##member_name(struct device *dev, \
> - struct device_attribute
> *attr, char *buf)\
> + static ssize_t show_##member_name(struct kobject *kobj, \
> + struct kobj_attribute
> *attr, char *buf)\
> {
> \
> struct uncore_data *data = container_of(attr, struct
> uncore_data,\
> -
> member_name##_dev_attr);\
> +
> member_name##_kobj_attr);\
>
> \
> return sysfs_emit(buf,
> "%u\n", \
> data-
> >member_name); \
> @@ -149,29 +149,29 @@ show_uncore_data(initial_max_freq_khz);
>
> #define
> init_attribute_rw(_name) \
> do
> { \
> - sysfs_attr_init(&data->_name##_dev_attr.attr); \
> - data->_name##_dev_attr.show =
> show_##_name; \
> - data->_name##_dev_attr.store =
> store_##_name; \
> - data->_name##_dev_attr.attr.name =
> #_name; \
> - data->_name##_dev_attr.attr.mode =
> 0644; \
> + sysfs_attr_init(&data->_name##_kobj_attr.attr); \
> + data->_name##_kobj_attr.show =
> show_##_name; \
> + data->_name##_kobj_attr.store =
> store_##_name; \
> + data->_name##_kobj_attr.attr.name =
> #_name; \
> + data->_name##_kobj_attr.attr.mode =
> 0644; \
> } while (0)
>
> #define
> init_attribute_ro(_name) \
> do
> { \
> - sysfs_attr_init(&data->_name##_dev_attr.attr); \
> - data->_name##_dev_attr.show =
> show_##_name; \
> - data->_name##_dev_attr.store =
> NULL; \
> - data->_name##_dev_attr.attr.name =
> #_name; \
> - data->_name##_dev_attr.attr.mode =
> 0444; \
> + sysfs_attr_init(&data->_name##_kobj_attr.attr); \
> + data->_name##_kobj_attr.show =
> show_##_name; \
> + data->_name##_kobj_attr.store =
> NULL; \
> + data->_name##_kobj_attr.attr.name =
> #_name; \
> + data->_name##_kobj_attr.attr.mode =
> 0444; \
> } while (0)
>
> #define
> init_attribute_root_ro(_name) \
> do
> { \
> - sysfs_attr_init(&data->_name##_dev_attr.attr); \
> - data->_name##_dev_attr.show =
> show_##_name; \
> - data->_name##_dev_attr.store =
> NULL; \
> - data->_name##_dev_attr.attr.name =
> #_name; \
> - data->_name##_dev_attr.attr.mode =
> 0400; \
> + sysfs_attr_init(&data->_name##_kobj_attr.attr); \
> + data->_name##_kobj_attr.show =
> show_##_name; \
> + data->_name##_kobj_attr.store =
> NULL; \
> + data->_name##_kobj_attr.attr.name =
> #_name; \
> + data->_name##_kobj_attr.attr.mode =
> 0400; \
> } while (0)
>
> static int create_attr_group(struct uncore_data *data, char *name)
> @@ -186,21 +186,21 @@ static int create_attr_group(struct uncore_data
> *data, char *name)
>
> if (data->domain_id != UNCORE_DOMAIN_ID_INVALID) {
> init_attribute_root_ro(domain_id);
> - data->uncore_attrs[index++] = &data-
> >domain_id_dev_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >domain_id_kobj_attr.attr;
> init_attribute_root_ro(fabric_cluster_id);
> - data->uncore_attrs[index++] = &data-
> >fabric_cluster_id_dev_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >fabric_cluster_id_kobj_attr.attr;
> init_attribute_root_ro(package_id);
> - data->uncore_attrs[index++] = &data-
> >package_id_dev_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >package_id_kobj_attr.attr;
> }
>
> - data->uncore_attrs[index++] = &data-
> >max_freq_khz_dev_attr.attr;
> - data->uncore_attrs[index++] = &data-
> >min_freq_khz_dev_attr.attr;
> - data->uncore_attrs[index++] = &data-
> >initial_min_freq_khz_dev_attr.attr;
> - data->uncore_attrs[index++] = &data-
> >initial_max_freq_khz_dev_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >max_freq_khz_kobj_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >min_freq_khz_kobj_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >initial_min_freq_khz_kobj_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >initial_max_freq_khz_kobj_attr.attr;
>
> ret = uncore_read_freq(data, &freq);
> if (!ret)
> - data->uncore_attrs[index++] = &data-
> >current_freq_khz_dev_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >current_freq_khz_kobj_attr.attr;
>
> data->uncore_attrs[index] = NULL;
>
> diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-
> frequency-common.h b/drivers/platform/x86/intel/uncore-
> frequency/uncore-frequency-common.h
> index 7afb69977c7e..0e5bf507e555 100644
> --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> common.h
> +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> common.h
> @@ -26,14 +26,14 @@
> * @instance_id: Unique instance id to append to directory
> name
> * @name: Sysfs entry name for this instance
> * @uncore_attr_group: Attribute group storage
> - * @max_freq_khz_dev_attr: Storage for device attribute max_freq_khz
> - * @mix_freq_khz_dev_attr: Storage for device attribute min_freq_khz
> - * @initial_max_freq_khz_dev_attr: Storage for device attribute
> initial_max_freq_khz
> - * @initial_min_freq_khz_dev_attr: Storage for device attribute
> initial_min_freq_khz
> - * @current_freq_khz_dev_attr: Storage for device attribute
> current_freq_khz
> - * @domain_id_dev_attr: Storage for device attribute domain_id
> - * @fabric_cluster_id_dev_attr: Storage for device attribute
> fabric_cluster_id
> - * @package_id_dev_attr: Storage for device attribute package_id
> + * @max_freq_khz_kobj_attr: Storage for kobject attribute
> max_freq_khz
> + * @mix_freq_khz_kobj_attr: Storage for kobject attribute
> min_freq_khz
> + * @initial_max_freq_khz_kobj_attr: Storage for kobject attribute
> initial_max_freq_khz
> + * @initial_min_freq_khz_kobj_attr: Storage for kobject attribute
> initial_min_freq_khz
> + * @current_freq_khz_kobj_attr: Storage for kobject attribute
> current_freq_khz
> + * @domain_id_kobj_attr: Storage for kobject attribute domain_id
> + * @fabric_cluster_id_kobj_attr: Storage for kobject attribute
> fabric_cluster_id
> + * @package_id_kobj_attr: Storage for kobject attribute package_id
> * @uncore_attrs: Attribute storage for group creation
> *
> * This structure is used to encapsulate all data related to uncore
> sysfs
> @@ -53,14 +53,14 @@ struct uncore_data {
> char name[32];
>
> struct attribute_group uncore_attr_group;
> - struct device_attribute max_freq_khz_dev_attr;
> - struct device_attribute min_freq_khz_dev_attr;
> - struct device_attribute initial_max_freq_khz_dev_attr;
> - struct device_attribute initial_min_freq_khz_dev_attr;
> - struct device_attribute current_freq_khz_dev_attr;
> - struct device_attribute domain_id_dev_attr;
> - struct device_attribute fabric_cluster_id_dev_attr;
> - struct device_attribute package_id_dev_attr;
> + struct kobj_attribute max_freq_khz_kobj_attr;
> + struct kobj_attribute min_freq_khz_kobj_attr;
> + struct kobj_attribute initial_max_freq_khz_kobj_attr;
> + struct kobj_attribute initial_min_freq_khz_kobj_attr;
> + struct kobj_attribute current_freq_khz_kobj_attr;
> + struct kobj_attribute domain_id_kobj_attr;
> + struct kobj_attribute fabric_cluster_id_kobj_attr;
> + struct kobj_attribute package_id_kobj_attr;
> struct attribute *uncore_attrs[9];
> };
>
>
> ---
> base-commit: 236f7d8034ff401d02fa6d74bae494a2b54e1834
> change-id: 20240104-intel-uncore-freq-kcfi-fix-6ef07053db58
>
> Best regards,
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] platform/x86: intel-uncore-freq: Fix types in sysfs callbacks
2024-01-04 22:59 [PATCH] platform/x86: intel-uncore-freq: Fix types in sysfs callbacks Nathan Chancellor
2024-01-04 23:31 ` Sami Tolvanen
2024-01-05 3:42 ` srinivas pandruvada
@ 2024-01-08 7:25 ` srinivas pandruvada
2024-01-08 16:47 ` Nathan Chancellor
2024-01-08 14:52 ` Hans de Goede
3 siblings, 1 reply; 6+ messages in thread
From: srinivas pandruvada @ 2024-01-08 7:25 UTC (permalink / raw)
To: Nathan Chancellor, hdegoede, ilpo.jarvinen
Cc: keescook, samitolvanen, platform-driver-x86, llvm, patches
On Thu, 2024-01-04 at 15:59 -0700, Nathan Chancellor wrote:
> When booting a kernel with CONFIG_CFI_CLANG, there is a CFI failure
> when
> accessing any of the values under
> /sys/devices/system/cpu/intel_uncore_frequency/package_00_die_00:
>
> $ cat
> /sys/devices/system/cpu/intel_uncore_frequency/package_00_die_00/max_
> freq_khz
> fish: Job 1, 'cat /sys/devices/system/cpu/int…' terminated by
> signal SIGSEGV (Address boundary error)
>
> $ sudo dmesg &| grep 'CFI failure'
> [ 170.953925] CFI failure at kobj_attr_show+0x19/0x30 (target:
> show_max_freq_khz+0x0/0xc0 [intel_uncore_frequency_common]; expected
> type: 0xd34078c5
>
> The sysfs callback functions such as show_domain_id() are written as
> if
> they are going to be called by dev_attr_show() but as the above
> message
> shows, they are instead called by kobj_attr_show(). kCFI checks that
> the
> destination of an indirect jump has the exact same type as the
> prototype
> of the function pointer it is called through and fails when they do
> not.
>
> These callbacks are called through kobj_attr_show() because
> uncore_root_kobj was initialized with kobject_create_and_add(), which
> means uncore_root_kobj has a ->sysfs_ops of kobj_sysfs_ops from
> kobject_create(), which uses kobj_attr_show() as its ->show() value.
>
> The only reason there has not been a more noticeable problem until
> this
> point is that 'struct kobj_attribute' and 'struct device_attribute'
> have
> the same layout, so getting the callback from container_of() works
> the
> same with either value.
>
> Change all the callbacks and their uses to be compatible with
> kobj_attr_show() and kobj_attr_store(), which resolves the kCFI
> failure
> and allows the sysfs files to work properly.
>
> Closes: https://github.com/ClangBuiltLinux/linux/issues/1974
> Fixes: ae7b2ce57851 ("platform/x86/intel/uncore-freq: Use sysfs API
> to create attributes")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Do you need Cc: stable@vger.kernel.org
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> I think I got the fixes tag right. I only started seeing this because
> of
> commit 27f2b08735c9 ("platform/x86: intel-uncore-freq: Add additional
> client processors"), which allows this driver to load on my i7-11700
> test system but I think the commit in the Fixes tag incorrectly
> changes
> the types of the callbacks.
> ---
> .../uncore-frequency/uncore-frequency-common.c | 82 +++++++++++-
> ----------
> .../uncore-frequency/uncore-frequency-common.h | 32 ++++-----
> 2 files changed, 57 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-
> frequency-common.c b/drivers/platform/x86/intel/uncore-
> frequency/uncore-frequency-common.c
> index 33ab207493e3..33bb58dc3f78 100644
> --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> common.c
> +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> common.c
> @@ -23,23 +23,23 @@ static int (*uncore_read)(struct uncore_data
> *data, unsigned int *min, unsigned
> static int (*uncore_write)(struct uncore_data *data, unsigned int
> input, unsigned int min_max);
> static int (*uncore_read_freq)(struct uncore_data *data, unsigned
> int *freq);
>
> -static ssize_t show_domain_id(struct device *dev, struct
> device_attribute *attr, char *buf)
> +static ssize_t show_domain_id(struct kobject *kobj, struct
> kobj_attribute *attr, char *buf)
> {
> - struct uncore_data *data = container_of(attr, struct
> uncore_data, domain_id_dev_attr);
> + struct uncore_data *data = container_of(attr, struct
> uncore_data, domain_id_kobj_attr);
>
> return sprintf(buf, "%u\n", data->domain_id);
> }
>
> -static ssize_t show_fabric_cluster_id(struct device *dev, struct
> device_attribute *attr, char *buf)
> +static ssize_t show_fabric_cluster_id(struct kobject *kobj, struct
> kobj_attribute *attr, char *buf)
> {
> - struct uncore_data *data = container_of(attr, struct
> uncore_data, fabric_cluster_id_dev_attr);
> + struct uncore_data *data = container_of(attr, struct
> uncore_data, fabric_cluster_id_kobj_attr);
>
> return sprintf(buf, "%u\n", data->cluster_id);
> }
>
> -static ssize_t show_package_id(struct device *dev, struct
> device_attribute *attr, char *buf)
> +static ssize_t show_package_id(struct kobject *kobj, struct
> kobj_attribute *attr, char *buf)
> {
> - struct uncore_data *data = container_of(attr, struct
> uncore_data, package_id_dev_attr);
> + struct uncore_data *data = container_of(attr, struct
> uncore_data, package_id_kobj_attr);
>
> return sprintf(buf, "%u\n", data->package_id);
> }
> @@ -97,30 +97,30 @@ static ssize_t show_perf_status_freq_khz(struct
> uncore_data *data, char *buf)
> }
>
> #define store_uncore_min_max(name,
> min_max) \
> - static ssize_t store_##name(struct device *dev, \
> - struct device_attribute
> *attr, \
> + static ssize_t store_##name(struct kobject
> *kobj, \
> + struct kobj_attribute
> *attr, \
> const char *buf, size_t
> count) \
> {
> \
> - struct uncore_data *data = container_of(attr, struct
> uncore_data, name##_dev_attr);\
> + struct uncore_data *data = container_of(attr, struct
> uncore_data, name##_kobj_attr);\
>
> \
> return store_min_max_freq_khz(data, buf, count, \
> min_max); \
> }
>
> #define show_uncore_min_max(name,
> min_max) \
> - static ssize_t show_##name(struct device *dev, \
> - struct device_attribute *attr,
> char *buf)\
> + static ssize_t show_##name(struct kobject
> *kobj, \
> + struct kobj_attribute *attr, char
> *buf)\
> {
> \
> - struct uncore_data *data = container_of(attr, struct
> uncore_data, name##_dev_attr);\
> + struct uncore_data *data = container_of(attr, struct
> uncore_data, name##_kobj_attr);\
>
> \
> return show_min_max_freq_khz(data, buf,
> min_max); \
> }
>
> #define
> show_uncore_perf_status(name) \
> - static ssize_t show_##name(struct device *dev, \
> - struct device_attribute *attr,
> char *buf)\
> + static ssize_t show_##name(struct kobject
> *kobj, \
> + struct kobj_attribute *attr, char
> *buf)\
> {
> \
> - struct uncore_data *data = container_of(attr, struct
> uncore_data, name##_dev_attr);\
> + struct uncore_data *data = container_of(attr, struct
> uncore_data, name##_kobj_attr);\
>
> \
> return show_perf_status_freq_khz(data, buf); \
> }
> @@ -134,11 +134,11 @@ show_uncore_min_max(max_freq_khz, 1);
> show_uncore_perf_status(current_freq_khz);
>
> #define
> show_uncore_data(member_name) \
> - static ssize_t show_##member_name(struct device *dev, \
> - struct device_attribute
> *attr, char *buf)\
> + static ssize_t show_##member_name(struct kobject *kobj, \
> + struct kobj_attribute
> *attr, char *buf)\
> {
> \
> struct uncore_data *data = container_of(attr, struct
> uncore_data,\
> -
> member_name##_dev_attr);\
> +
> member_name##_kobj_attr);\
>
> \
> return sysfs_emit(buf,
> "%u\n", \
> data-
> >member_name); \
> @@ -149,29 +149,29 @@ show_uncore_data(initial_max_freq_khz);
>
> #define
> init_attribute_rw(_name) \
> do
> { \
> - sysfs_attr_init(&data->_name##_dev_attr.attr); \
> - data->_name##_dev_attr.show =
> show_##_name; \
> - data->_name##_dev_attr.store =
> store_##_name; \
> - data->_name##_dev_attr.attr.name =
> #_name; \
> - data->_name##_dev_attr.attr.mode =
> 0644; \
> + sysfs_attr_init(&data->_name##_kobj_attr.attr); \
> + data->_name##_kobj_attr.show =
> show_##_name; \
> + data->_name##_kobj_attr.store =
> store_##_name; \
> + data->_name##_kobj_attr.attr.name =
> #_name; \
> + data->_name##_kobj_attr.attr.mode =
> 0644; \
> } while (0)
>
> #define
> init_attribute_ro(_name) \
> do
> { \
> - sysfs_attr_init(&data->_name##_dev_attr.attr); \
> - data->_name##_dev_attr.show =
> show_##_name; \
> - data->_name##_dev_attr.store =
> NULL; \
> - data->_name##_dev_attr.attr.name =
> #_name; \
> - data->_name##_dev_attr.attr.mode =
> 0444; \
> + sysfs_attr_init(&data->_name##_kobj_attr.attr); \
> + data->_name##_kobj_attr.show =
> show_##_name; \
> + data->_name##_kobj_attr.store =
> NULL; \
> + data->_name##_kobj_attr.attr.name =
> #_name; \
> + data->_name##_kobj_attr.attr.mode =
> 0444; \
> } while (0)
>
> #define
> init_attribute_root_ro(_name) \
> do
> { \
> - sysfs_attr_init(&data->_name##_dev_attr.attr); \
> - data->_name##_dev_attr.show =
> show_##_name; \
> - data->_name##_dev_attr.store =
> NULL; \
> - data->_name##_dev_attr.attr.name =
> #_name; \
> - data->_name##_dev_attr.attr.mode =
> 0400; \
> + sysfs_attr_init(&data->_name##_kobj_attr.attr); \
> + data->_name##_kobj_attr.show =
> show_##_name; \
> + data->_name##_kobj_attr.store =
> NULL; \
> + data->_name##_kobj_attr.attr.name =
> #_name; \
> + data->_name##_kobj_attr.attr.mode =
> 0400; \
> } while (0)
>
> static int create_attr_group(struct uncore_data *data, char *name)
> @@ -186,21 +186,21 @@ static int create_attr_group(struct uncore_data
> *data, char *name)
>
> if (data->domain_id != UNCORE_DOMAIN_ID_INVALID) {
> init_attribute_root_ro(domain_id);
> - data->uncore_attrs[index++] = &data-
> >domain_id_dev_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >domain_id_kobj_attr.attr;
> init_attribute_root_ro(fabric_cluster_id);
> - data->uncore_attrs[index++] = &data-
> >fabric_cluster_id_dev_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >fabric_cluster_id_kobj_attr.attr;
> init_attribute_root_ro(package_id);
> - data->uncore_attrs[index++] = &data-
> >package_id_dev_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >package_id_kobj_attr.attr;
> }
>
> - data->uncore_attrs[index++] = &data-
> >max_freq_khz_dev_attr.attr;
> - data->uncore_attrs[index++] = &data-
> >min_freq_khz_dev_attr.attr;
> - data->uncore_attrs[index++] = &data-
> >initial_min_freq_khz_dev_attr.attr;
> - data->uncore_attrs[index++] = &data-
> >initial_max_freq_khz_dev_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >max_freq_khz_kobj_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >min_freq_khz_kobj_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >initial_min_freq_khz_kobj_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >initial_max_freq_khz_kobj_attr.attr;
>
> ret = uncore_read_freq(data, &freq);
> if (!ret)
> - data->uncore_attrs[index++] = &data-
> >current_freq_khz_dev_attr.attr;
> + data->uncore_attrs[index++] = &data-
> >current_freq_khz_kobj_attr.attr;
>
> data->uncore_attrs[index] = NULL;
>
> diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-
> frequency-common.h b/drivers/platform/x86/intel/uncore-
> frequency/uncore-frequency-common.h
> index 7afb69977c7e..0e5bf507e555 100644
> --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> common.h
> +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> common.h
> @@ -26,14 +26,14 @@
> * @instance_id: Unique instance id to append to directory
> name
> * @name: Sysfs entry name for this instance
> * @uncore_attr_group: Attribute group storage
> - * @max_freq_khz_dev_attr: Storage for device attribute max_freq_khz
> - * @mix_freq_khz_dev_attr: Storage for device attribute min_freq_khz
> - * @initial_max_freq_khz_dev_attr: Storage for device attribute
> initial_max_freq_khz
> - * @initial_min_freq_khz_dev_attr: Storage for device attribute
> initial_min_freq_khz
> - * @current_freq_khz_dev_attr: Storage for device attribute
> current_freq_khz
> - * @domain_id_dev_attr: Storage for device attribute domain_id
> - * @fabric_cluster_id_dev_attr: Storage for device attribute
> fabric_cluster_id
> - * @package_id_dev_attr: Storage for device attribute package_id
> + * @max_freq_khz_kobj_attr: Storage for kobject attribute
> max_freq_khz
> + * @mix_freq_khz_kobj_attr: Storage for kobject attribute
> min_freq_khz
> + * @initial_max_freq_khz_kobj_attr: Storage for kobject attribute
> initial_max_freq_khz
> + * @initial_min_freq_khz_kobj_attr: Storage for kobject attribute
> initial_min_freq_khz
> + * @current_freq_khz_kobj_attr: Storage for kobject attribute
> current_freq_khz
> + * @domain_id_kobj_attr: Storage for kobject attribute domain_id
> + * @fabric_cluster_id_kobj_attr: Storage for kobject attribute
> fabric_cluster_id
> + * @package_id_kobj_attr: Storage for kobject attribute package_id
> * @uncore_attrs: Attribute storage for group creation
> *
> * This structure is used to encapsulate all data related to uncore
> sysfs
> @@ -53,14 +53,14 @@ struct uncore_data {
> char name[32];
>
> struct attribute_group uncore_attr_group;
> - struct device_attribute max_freq_khz_dev_attr;
> - struct device_attribute min_freq_khz_dev_attr;
> - struct device_attribute initial_max_freq_khz_dev_attr;
> - struct device_attribute initial_min_freq_khz_dev_attr;
> - struct device_attribute current_freq_khz_dev_attr;
> - struct device_attribute domain_id_dev_attr;
> - struct device_attribute fabric_cluster_id_dev_attr;
> - struct device_attribute package_id_dev_attr;
> + struct kobj_attribute max_freq_khz_kobj_attr;
> + struct kobj_attribute min_freq_khz_kobj_attr;
> + struct kobj_attribute initial_max_freq_khz_kobj_attr;
> + struct kobj_attribute initial_min_freq_khz_kobj_attr;
> + struct kobj_attribute current_freq_khz_kobj_attr;
> + struct kobj_attribute domain_id_kobj_attr;
> + struct kobj_attribute fabric_cluster_id_kobj_attr;
> + struct kobj_attribute package_id_kobj_attr;
> struct attribute *uncore_attrs[9];
> };
>
>
> ---
> base-commit: 236f7d8034ff401d02fa6d74bae494a2b54e1834
> change-id: 20240104-intel-uncore-freq-kcfi-fix-6ef07053db58
>
> Best regards,
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] platform/x86: intel-uncore-freq: Fix types in sysfs callbacks
2024-01-08 7:25 ` srinivas pandruvada
@ 2024-01-08 16:47 ` Nathan Chancellor
0 siblings, 0 replies; 6+ messages in thread
From: Nathan Chancellor @ 2024-01-08 16:47 UTC (permalink / raw)
To: srinivas pandruvada
Cc: hdegoede, ilpo.jarvinen, keescook, samitolvanen,
platform-driver-x86, llvm, patches
On Sun, Jan 07, 2024 at 11:25:53PM -0800, srinivas pandruvada wrote:
> On Thu, 2024-01-04 at 15:59 -0700, Nathan Chancellor wrote:
> > When booting a kernel with CONFIG_CFI_CLANG, there is a CFI failure
> > when
> > accessing any of the values under
> > /sys/devices/system/cpu/intel_uncore_frequency/package_00_die_00:
> >
> > $ cat
> > /sys/devices/system/cpu/intel_uncore_frequency/package_00_die_00/max_
> > freq_khz
> > fish: Job 1, 'cat /sys/devices/system/cpu/int…' terminated by
> > signal SIGSEGV (Address boundary error)
> >
> > $ sudo dmesg &| grep 'CFI failure'
> > [ 170.953925] CFI failure at kobj_attr_show+0x19/0x30 (target:
> > show_max_freq_khz+0x0/0xc0 [intel_uncore_frequency_common]; expected
> > type: 0xd34078c5
> >
> > The sysfs callback functions such as show_domain_id() are written as
> > if
> > they are going to be called by dev_attr_show() but as the above
> > message
> > shows, they are instead called by kobj_attr_show(). kCFI checks that
> > the
> > destination of an indirect jump has the exact same type as the
> > prototype
> > of the function pointer it is called through and fails when they do
> > not.
> >
> > These callbacks are called through kobj_attr_show() because
> > uncore_root_kobj was initialized with kobject_create_and_add(), which
> > means uncore_root_kobj has a ->sysfs_ops of kobj_sysfs_ops from
> > kobject_create(), which uses kobj_attr_show() as its ->show() value.
> >
> > The only reason there has not been a more noticeable problem until
> > this
> > point is that 'struct kobj_attribute' and 'struct device_attribute'
> > have
> > the same layout, so getting the callback from container_of() works
> > the
> > same with either value.
> >
> > Change all the callbacks and their uses to be compatible with
> > kobj_attr_show() and kobj_attr_store(), which resolves the kCFI
> > failure
> > and allows the sysfs files to work properly.
> >
> > Closes: https://github.com/ClangBuiltLinux/linux/issues/1974
> > Fixes: ae7b2ce57851 ("platform/x86/intel/uncore-freq: Use sysfs API
> > to create attributes")
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> Do you need Cc: stable@vger.kernel.org
Yes, I think we do, and I see Hans graciously added that for me during
application. Thanks for taking a look!
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>
> > ---
> > I think I got the fixes tag right. I only started seeing this because
> > of
> > commit 27f2b08735c9 ("platform/x86: intel-uncore-freq: Add additional
> > client processors"), which allows this driver to load on my i7-11700
> > test system but I think the commit in the Fixes tag incorrectly
> > changes
> > the types of the callbacks.
> > ---
> > .../uncore-frequency/uncore-frequency-common.c | 82 +++++++++++-
> > ----------
> > .../uncore-frequency/uncore-frequency-common.h | 32 ++++-----
> > 2 files changed, 57 insertions(+), 57 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-
> > frequency-common.c b/drivers/platform/x86/intel/uncore-
> > frequency/uncore-frequency-common.c
> > index 33ab207493e3..33bb58dc3f78 100644
> > --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> > common.c
> > +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> > common.c
> > @@ -23,23 +23,23 @@ static int (*uncore_read)(struct uncore_data
> > *data, unsigned int *min, unsigned
> > static int (*uncore_write)(struct uncore_data *data, unsigned int
> > input, unsigned int min_max);
> > static int (*uncore_read_freq)(struct uncore_data *data, unsigned
> > int *freq);
> >
> > -static ssize_t show_domain_id(struct device *dev, struct
> > device_attribute *attr, char *buf)
> > +static ssize_t show_domain_id(struct kobject *kobj, struct
> > kobj_attribute *attr, char *buf)
> > {
> > - struct uncore_data *data = container_of(attr, struct
> > uncore_data, domain_id_dev_attr);
> > + struct uncore_data *data = container_of(attr, struct
> > uncore_data, domain_id_kobj_attr);
> >
> > return sprintf(buf, "%u\n", data->domain_id);
> > }
> >
> > -static ssize_t show_fabric_cluster_id(struct device *dev, struct
> > device_attribute *attr, char *buf)
> > +static ssize_t show_fabric_cluster_id(struct kobject *kobj, struct
> > kobj_attribute *attr, char *buf)
> > {
> > - struct uncore_data *data = container_of(attr, struct
> > uncore_data, fabric_cluster_id_dev_attr);
> > + struct uncore_data *data = container_of(attr, struct
> > uncore_data, fabric_cluster_id_kobj_attr);
> >
> > return sprintf(buf, "%u\n", data->cluster_id);
> > }
> >
> > -static ssize_t show_package_id(struct device *dev, struct
> > device_attribute *attr, char *buf)
> > +static ssize_t show_package_id(struct kobject *kobj, struct
> > kobj_attribute *attr, char *buf)
> > {
> > - struct uncore_data *data = container_of(attr, struct
> > uncore_data, package_id_dev_attr);
> > + struct uncore_data *data = container_of(attr, struct
> > uncore_data, package_id_kobj_attr);
> >
> > return sprintf(buf, "%u\n", data->package_id);
> > }
> > @@ -97,30 +97,30 @@ static ssize_t show_perf_status_freq_khz(struct
> > uncore_data *data, char *buf)
> > }
> >
> > #define store_uncore_min_max(name,
> > min_max) \
> > - static ssize_t store_##name(struct device *dev, \
> > - struct device_attribute
> > *attr, \
> > + static ssize_t store_##name(struct kobject
> > *kobj, \
> > + struct kobj_attribute
> > *attr, \
> > const char *buf, size_t
> > count) \
> > {
> > \
> > - struct uncore_data *data = container_of(attr, struct
> > uncore_data, name##_dev_attr);\
> > + struct uncore_data *data = container_of(attr, struct
> > uncore_data, name##_kobj_attr);\
> >
> > \
> > return store_min_max_freq_khz(data, buf, count, \
> > min_max); \
> > }
> >
> > #define show_uncore_min_max(name,
> > min_max) \
> > - static ssize_t show_##name(struct device *dev, \
> > - struct device_attribute *attr,
> > char *buf)\
> > + static ssize_t show_##name(struct kobject
> > *kobj, \
> > + struct kobj_attribute *attr, char
> > *buf)\
> > {
> > \
> > - struct uncore_data *data = container_of(attr, struct
> > uncore_data, name##_dev_attr);\
> > + struct uncore_data *data = container_of(attr, struct
> > uncore_data, name##_kobj_attr);\
> >
> > \
> > return show_min_max_freq_khz(data, buf,
> > min_max); \
> > }
> >
> > #define
> > show_uncore_perf_status(name) \
> > - static ssize_t show_##name(struct device *dev, \
> > - struct device_attribute *attr,
> > char *buf)\
> > + static ssize_t show_##name(struct kobject
> > *kobj, \
> > + struct kobj_attribute *attr, char
> > *buf)\
> > {
> > \
> > - struct uncore_data *data = container_of(attr, struct
> > uncore_data, name##_dev_attr);\
> > + struct uncore_data *data = container_of(attr, struct
> > uncore_data, name##_kobj_attr);\
> >
> > \
> > return show_perf_status_freq_khz(data, buf); \
> > }
> > @@ -134,11 +134,11 @@ show_uncore_min_max(max_freq_khz, 1);
> > show_uncore_perf_status(current_freq_khz);
> >
> > #define
> > show_uncore_data(member_name) \
> > - static ssize_t show_##member_name(struct device *dev, \
> > - struct device_attribute
> > *attr, char *buf)\
> > + static ssize_t show_##member_name(struct kobject *kobj, \
> > + struct kobj_attribute
> > *attr, char *buf)\
> > {
> > \
> > struct uncore_data *data = container_of(attr, struct
> > uncore_data,\
> > -
> > member_name##_dev_attr);\
> > +
> > member_name##_kobj_attr);\
> >
> > \
> > return sysfs_emit(buf,
> > "%u\n", \
> > data-
> > >member_name); \
> > @@ -149,29 +149,29 @@ show_uncore_data(initial_max_freq_khz);
> >
> > #define
> > init_attribute_rw(_name) \
> > do
> > { \
> > - sysfs_attr_init(&data->_name##_dev_attr.attr); \
> > - data->_name##_dev_attr.show =
> > show_##_name; \
> > - data->_name##_dev_attr.store =
> > store_##_name; \
> > - data->_name##_dev_attr.attr.name =
> > #_name; \
> > - data->_name##_dev_attr.attr.mode =
> > 0644; \
> > + sysfs_attr_init(&data->_name##_kobj_attr.attr); \
> > + data->_name##_kobj_attr.show =
> > show_##_name; \
> > + data->_name##_kobj_attr.store =
> > store_##_name; \
> > + data->_name##_kobj_attr.attr.name =
> > #_name; \
> > + data->_name##_kobj_attr.attr.mode =
> > 0644; \
> > } while (0)
> >
> > #define
> > init_attribute_ro(_name) \
> > do
> > { \
> > - sysfs_attr_init(&data->_name##_dev_attr.attr); \
> > - data->_name##_dev_attr.show =
> > show_##_name; \
> > - data->_name##_dev_attr.store =
> > NULL; \
> > - data->_name##_dev_attr.attr.name =
> > #_name; \
> > - data->_name##_dev_attr.attr.mode =
> > 0444; \
> > + sysfs_attr_init(&data->_name##_kobj_attr.attr); \
> > + data->_name##_kobj_attr.show =
> > show_##_name; \
> > + data->_name##_kobj_attr.store =
> > NULL; \
> > + data->_name##_kobj_attr.attr.name =
> > #_name; \
> > + data->_name##_kobj_attr.attr.mode =
> > 0444; \
> > } while (0)
> >
> > #define
> > init_attribute_root_ro(_name) \
> > do
> > { \
> > - sysfs_attr_init(&data->_name##_dev_attr.attr); \
> > - data->_name##_dev_attr.show =
> > show_##_name; \
> > - data->_name##_dev_attr.store =
> > NULL; \
> > - data->_name##_dev_attr.attr.name =
> > #_name; \
> > - data->_name##_dev_attr.attr.mode =
> > 0400; \
> > + sysfs_attr_init(&data->_name##_kobj_attr.attr); \
> > + data->_name##_kobj_attr.show =
> > show_##_name; \
> > + data->_name##_kobj_attr.store =
> > NULL; \
> > + data->_name##_kobj_attr.attr.name =
> > #_name; \
> > + data->_name##_kobj_attr.attr.mode =
> > 0400; \
> > } while (0)
> >
> > static int create_attr_group(struct uncore_data *data, char *name)
> > @@ -186,21 +186,21 @@ static int create_attr_group(struct uncore_data
> > *data, char *name)
> >
> > if (data->domain_id != UNCORE_DOMAIN_ID_INVALID) {
> > init_attribute_root_ro(domain_id);
> > - data->uncore_attrs[index++] = &data-
> > >domain_id_dev_attr.attr;
> > + data->uncore_attrs[index++] = &data-
> > >domain_id_kobj_attr.attr;
> > init_attribute_root_ro(fabric_cluster_id);
> > - data->uncore_attrs[index++] = &data-
> > >fabric_cluster_id_dev_attr.attr;
> > + data->uncore_attrs[index++] = &data-
> > >fabric_cluster_id_kobj_attr.attr;
> > init_attribute_root_ro(package_id);
> > - data->uncore_attrs[index++] = &data-
> > >package_id_dev_attr.attr;
> > + data->uncore_attrs[index++] = &data-
> > >package_id_kobj_attr.attr;
> > }
> >
> > - data->uncore_attrs[index++] = &data-
> > >max_freq_khz_dev_attr.attr;
> > - data->uncore_attrs[index++] = &data-
> > >min_freq_khz_dev_attr.attr;
> > - data->uncore_attrs[index++] = &data-
> > >initial_min_freq_khz_dev_attr.attr;
> > - data->uncore_attrs[index++] = &data-
> > >initial_max_freq_khz_dev_attr.attr;
> > + data->uncore_attrs[index++] = &data-
> > >max_freq_khz_kobj_attr.attr;
> > + data->uncore_attrs[index++] = &data-
> > >min_freq_khz_kobj_attr.attr;
> > + data->uncore_attrs[index++] = &data-
> > >initial_min_freq_khz_kobj_attr.attr;
> > + data->uncore_attrs[index++] = &data-
> > >initial_max_freq_khz_kobj_attr.attr;
> >
> > ret = uncore_read_freq(data, &freq);
> > if (!ret)
> > - data->uncore_attrs[index++] = &data-
> > >current_freq_khz_dev_attr.attr;
> > + data->uncore_attrs[index++] = &data-
> > >current_freq_khz_kobj_attr.attr;
> >
> > data->uncore_attrs[index] = NULL;
> >
> > diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-
> > frequency-common.h b/drivers/platform/x86/intel/uncore-
> > frequency/uncore-frequency-common.h
> > index 7afb69977c7e..0e5bf507e555 100644
> > --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> > common.h
> > +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-
> > common.h
> > @@ -26,14 +26,14 @@
> > * @instance_id: Unique instance id to append to directory
> > name
> > * @name: Sysfs entry name for this instance
> > * @uncore_attr_group: Attribute group storage
> > - * @max_freq_khz_dev_attr: Storage for device attribute max_freq_khz
> > - * @mix_freq_khz_dev_attr: Storage for device attribute min_freq_khz
> > - * @initial_max_freq_khz_dev_attr: Storage for device attribute
> > initial_max_freq_khz
> > - * @initial_min_freq_khz_dev_attr: Storage for device attribute
> > initial_min_freq_khz
> > - * @current_freq_khz_dev_attr: Storage for device attribute
> > current_freq_khz
> > - * @domain_id_dev_attr: Storage for device attribute domain_id
> > - * @fabric_cluster_id_dev_attr: Storage for device attribute
> > fabric_cluster_id
> > - * @package_id_dev_attr: Storage for device attribute package_id
> > + * @max_freq_khz_kobj_attr: Storage for kobject attribute
> > max_freq_khz
> > + * @mix_freq_khz_kobj_attr: Storage for kobject attribute
> > min_freq_khz
> > + * @initial_max_freq_khz_kobj_attr: Storage for kobject attribute
> > initial_max_freq_khz
> > + * @initial_min_freq_khz_kobj_attr: Storage for kobject attribute
> > initial_min_freq_khz
> > + * @current_freq_khz_kobj_attr: Storage for kobject attribute
> > current_freq_khz
> > + * @domain_id_kobj_attr: Storage for kobject attribute domain_id
> > + * @fabric_cluster_id_kobj_attr: Storage for kobject attribute
> > fabric_cluster_id
> > + * @package_id_kobj_attr: Storage for kobject attribute package_id
> > * @uncore_attrs: Attribute storage for group creation
> > *
> > * This structure is used to encapsulate all data related to uncore
> > sysfs
> > @@ -53,14 +53,14 @@ struct uncore_data {
> > char name[32];
> >
> > struct attribute_group uncore_attr_group;
> > - struct device_attribute max_freq_khz_dev_attr;
> > - struct device_attribute min_freq_khz_dev_attr;
> > - struct device_attribute initial_max_freq_khz_dev_attr;
> > - struct device_attribute initial_min_freq_khz_dev_attr;
> > - struct device_attribute current_freq_khz_dev_attr;
> > - struct device_attribute domain_id_dev_attr;
> > - struct device_attribute fabric_cluster_id_dev_attr;
> > - struct device_attribute package_id_dev_attr;
> > + struct kobj_attribute max_freq_khz_kobj_attr;
> > + struct kobj_attribute min_freq_khz_kobj_attr;
> > + struct kobj_attribute initial_max_freq_khz_kobj_attr;
> > + struct kobj_attribute initial_min_freq_khz_kobj_attr;
> > + struct kobj_attribute current_freq_khz_kobj_attr;
> > + struct kobj_attribute domain_id_kobj_attr;
> > + struct kobj_attribute fabric_cluster_id_kobj_attr;
> > + struct kobj_attribute package_id_kobj_attr;
> > struct attribute *uncore_attrs[9];
> > };
> >
> >
> > ---
> > base-commit: 236f7d8034ff401d02fa6d74bae494a2b54e1834
> > change-id: 20240104-intel-uncore-freq-kcfi-fix-6ef07053db58
> >
> > Best regards,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] platform/x86: intel-uncore-freq: Fix types in sysfs callbacks
2024-01-04 22:59 [PATCH] platform/x86: intel-uncore-freq: Fix types in sysfs callbacks Nathan Chancellor
` (2 preceding siblings ...)
2024-01-08 7:25 ` srinivas pandruvada
@ 2024-01-08 14:52 ` Hans de Goede
3 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2024-01-08 14:52 UTC (permalink / raw)
To: Nathan Chancellor, srinivas.pandruvada, ilpo.jarvinen
Cc: keescook, samitolvanen, platform-driver-x86, llvm, patches
Hi,
On 1/4/24 23:59, Nathan Chancellor wrote:
> When booting a kernel with CONFIG_CFI_CLANG, there is a CFI failure when
> accessing any of the values under
> /sys/devices/system/cpu/intel_uncore_frequency/package_00_die_00:
>
> $ cat /sys/devices/system/cpu/intel_uncore_frequency/package_00_die_00/max_freq_khz
> fish: Job 1, 'cat /sys/devices/system/cpu/int…' terminated by signal SIGSEGV (Address boundary error)
>
> $ sudo dmesg &| grep 'CFI failure'
> [ 170.953925] CFI failure at kobj_attr_show+0x19/0x30 (target: show_max_freq_khz+0x0/0xc0 [intel_uncore_frequency_common]; expected type: 0xd34078c5
>
> The sysfs callback functions such as show_domain_id() are written as if
> they are going to be called by dev_attr_show() but as the above message
> shows, they are instead called by kobj_attr_show(). kCFI checks that the
> destination of an indirect jump has the exact same type as the prototype
> of the function pointer it is called through and fails when they do not.
>
> These callbacks are called through kobj_attr_show() because
> uncore_root_kobj was initialized with kobject_create_and_add(), which
> means uncore_root_kobj has a ->sysfs_ops of kobj_sysfs_ops from
> kobject_create(), which uses kobj_attr_show() as its ->show() value.
>
> The only reason there has not been a more noticeable problem until this
> point is that 'struct kobj_attribute' and 'struct device_attribute' have
> the same layout, so getting the callback from container_of() works the
> same with either value.
>
> Change all the callbacks and their uses to be compatible with
> kobj_attr_show() and kobj_attr_store(), which resolves the kCFI failure
> and allows the sysfs files to work properly.
>
> Closes: https://github.com/ClangBuiltLinux/linux/issues/1974
> Fixes: ae7b2ce57851 ("platform/x86/intel/uncore-freq: Use sysfs API to create attributes")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
> I think I got the fixes tag right. I only started seeing this because of
> commit 27f2b08735c9 ("platform/x86: intel-uncore-freq: Add additional
> client processors"), which allows this driver to load on my i7-11700
> test system but I think the commit in the Fixes tag incorrectly changes
> the types of the callbacks.
Thank you for your patch, I've applied this patch to my review-hans
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.
I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.
Regards,
Hans
> ---
> .../uncore-frequency/uncore-frequency-common.c | 82 +++++++++++-----------
> .../uncore-frequency/uncore-frequency-common.h | 32 ++++-----
> 2 files changed, 57 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
> index 33ab207493e3..33bb58dc3f78 100644
> --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
> +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
> @@ -23,23 +23,23 @@ static int (*uncore_read)(struct uncore_data *data, unsigned int *min, unsigned
> static int (*uncore_write)(struct uncore_data *data, unsigned int input, unsigned int min_max);
> static int (*uncore_read_freq)(struct uncore_data *data, unsigned int *freq);
>
> -static ssize_t show_domain_id(struct device *dev, struct device_attribute *attr, char *buf)
> +static ssize_t show_domain_id(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> - struct uncore_data *data = container_of(attr, struct uncore_data, domain_id_dev_attr);
> + struct uncore_data *data = container_of(attr, struct uncore_data, domain_id_kobj_attr);
>
> return sprintf(buf, "%u\n", data->domain_id);
> }
>
> -static ssize_t show_fabric_cluster_id(struct device *dev, struct device_attribute *attr, char *buf)
> +static ssize_t show_fabric_cluster_id(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> - struct uncore_data *data = container_of(attr, struct uncore_data, fabric_cluster_id_dev_attr);
> + struct uncore_data *data = container_of(attr, struct uncore_data, fabric_cluster_id_kobj_attr);
>
> return sprintf(buf, "%u\n", data->cluster_id);
> }
>
> -static ssize_t show_package_id(struct device *dev, struct device_attribute *attr, char *buf)
> +static ssize_t show_package_id(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> - struct uncore_data *data = container_of(attr, struct uncore_data, package_id_dev_attr);
> + struct uncore_data *data = container_of(attr, struct uncore_data, package_id_kobj_attr);
>
> return sprintf(buf, "%u\n", data->package_id);
> }
> @@ -97,30 +97,30 @@ static ssize_t show_perf_status_freq_khz(struct uncore_data *data, char *buf)
> }
>
> #define store_uncore_min_max(name, min_max) \
> - static ssize_t store_##name(struct device *dev, \
> - struct device_attribute *attr, \
> + static ssize_t store_##name(struct kobject *kobj, \
> + struct kobj_attribute *attr, \
> const char *buf, size_t count) \
> { \
> - struct uncore_data *data = container_of(attr, struct uncore_data, name##_dev_attr);\
> + struct uncore_data *data = container_of(attr, struct uncore_data, name##_kobj_attr);\
> \
> return store_min_max_freq_khz(data, buf, count, \
> min_max); \
> }
>
> #define show_uncore_min_max(name, min_max) \
> - static ssize_t show_##name(struct device *dev, \
> - struct device_attribute *attr, char *buf)\
> + static ssize_t show_##name(struct kobject *kobj, \
> + struct kobj_attribute *attr, char *buf)\
> { \
> - struct uncore_data *data = container_of(attr, struct uncore_data, name##_dev_attr);\
> + struct uncore_data *data = container_of(attr, struct uncore_data, name##_kobj_attr);\
> \
> return show_min_max_freq_khz(data, buf, min_max); \
> }
>
> #define show_uncore_perf_status(name) \
> - static ssize_t show_##name(struct device *dev, \
> - struct device_attribute *attr, char *buf)\
> + static ssize_t show_##name(struct kobject *kobj, \
> + struct kobj_attribute *attr, char *buf)\
> { \
> - struct uncore_data *data = container_of(attr, struct uncore_data, name##_dev_attr);\
> + struct uncore_data *data = container_of(attr, struct uncore_data, name##_kobj_attr);\
> \
> return show_perf_status_freq_khz(data, buf); \
> }
> @@ -134,11 +134,11 @@ show_uncore_min_max(max_freq_khz, 1);
> show_uncore_perf_status(current_freq_khz);
>
> #define show_uncore_data(member_name) \
> - static ssize_t show_##member_name(struct device *dev, \
> - struct device_attribute *attr, char *buf)\
> + static ssize_t show_##member_name(struct kobject *kobj, \
> + struct kobj_attribute *attr, char *buf)\
> { \
> struct uncore_data *data = container_of(attr, struct uncore_data,\
> - member_name##_dev_attr);\
> + member_name##_kobj_attr);\
> \
> return sysfs_emit(buf, "%u\n", \
> data->member_name); \
> @@ -149,29 +149,29 @@ show_uncore_data(initial_max_freq_khz);
>
> #define init_attribute_rw(_name) \
> do { \
> - sysfs_attr_init(&data->_name##_dev_attr.attr); \
> - data->_name##_dev_attr.show = show_##_name; \
> - data->_name##_dev_attr.store = store_##_name; \
> - data->_name##_dev_attr.attr.name = #_name; \
> - data->_name##_dev_attr.attr.mode = 0644; \
> + sysfs_attr_init(&data->_name##_kobj_attr.attr); \
> + data->_name##_kobj_attr.show = show_##_name; \
> + data->_name##_kobj_attr.store = store_##_name; \
> + data->_name##_kobj_attr.attr.name = #_name; \
> + data->_name##_kobj_attr.attr.mode = 0644; \
> } while (0)
>
> #define init_attribute_ro(_name) \
> do { \
> - sysfs_attr_init(&data->_name##_dev_attr.attr); \
> - data->_name##_dev_attr.show = show_##_name; \
> - data->_name##_dev_attr.store = NULL; \
> - data->_name##_dev_attr.attr.name = #_name; \
> - data->_name##_dev_attr.attr.mode = 0444; \
> + sysfs_attr_init(&data->_name##_kobj_attr.attr); \
> + data->_name##_kobj_attr.show = show_##_name; \
> + data->_name##_kobj_attr.store = NULL; \
> + data->_name##_kobj_attr.attr.name = #_name; \
> + data->_name##_kobj_attr.attr.mode = 0444; \
> } while (0)
>
> #define init_attribute_root_ro(_name) \
> do { \
> - sysfs_attr_init(&data->_name##_dev_attr.attr); \
> - data->_name##_dev_attr.show = show_##_name; \
> - data->_name##_dev_attr.store = NULL; \
> - data->_name##_dev_attr.attr.name = #_name; \
> - data->_name##_dev_attr.attr.mode = 0400; \
> + sysfs_attr_init(&data->_name##_kobj_attr.attr); \
> + data->_name##_kobj_attr.show = show_##_name; \
> + data->_name##_kobj_attr.store = NULL; \
> + data->_name##_kobj_attr.attr.name = #_name; \
> + data->_name##_kobj_attr.attr.mode = 0400; \
> } while (0)
>
> static int create_attr_group(struct uncore_data *data, char *name)
> @@ -186,21 +186,21 @@ static int create_attr_group(struct uncore_data *data, char *name)
>
> if (data->domain_id != UNCORE_DOMAIN_ID_INVALID) {
> init_attribute_root_ro(domain_id);
> - data->uncore_attrs[index++] = &data->domain_id_dev_attr.attr;
> + data->uncore_attrs[index++] = &data->domain_id_kobj_attr.attr;
> init_attribute_root_ro(fabric_cluster_id);
> - data->uncore_attrs[index++] = &data->fabric_cluster_id_dev_attr.attr;
> + data->uncore_attrs[index++] = &data->fabric_cluster_id_kobj_attr.attr;
> init_attribute_root_ro(package_id);
> - data->uncore_attrs[index++] = &data->package_id_dev_attr.attr;
> + data->uncore_attrs[index++] = &data->package_id_kobj_attr.attr;
> }
>
> - data->uncore_attrs[index++] = &data->max_freq_khz_dev_attr.attr;
> - data->uncore_attrs[index++] = &data->min_freq_khz_dev_attr.attr;
> - data->uncore_attrs[index++] = &data->initial_min_freq_khz_dev_attr.attr;
> - data->uncore_attrs[index++] = &data->initial_max_freq_khz_dev_attr.attr;
> + data->uncore_attrs[index++] = &data->max_freq_khz_kobj_attr.attr;
> + data->uncore_attrs[index++] = &data->min_freq_khz_kobj_attr.attr;
> + data->uncore_attrs[index++] = &data->initial_min_freq_khz_kobj_attr.attr;
> + data->uncore_attrs[index++] = &data->initial_max_freq_khz_kobj_attr.attr;
>
> ret = uncore_read_freq(data, &freq);
> if (!ret)
> - data->uncore_attrs[index++] = &data->current_freq_khz_dev_attr.attr;
> + data->uncore_attrs[index++] = &data->current_freq_khz_kobj_attr.attr;
>
> data->uncore_attrs[index] = NULL;
>
> diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
> index 7afb69977c7e..0e5bf507e555 100644
> --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
> +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
> @@ -26,14 +26,14 @@
> * @instance_id: Unique instance id to append to directory name
> * @name: Sysfs entry name for this instance
> * @uncore_attr_group: Attribute group storage
> - * @max_freq_khz_dev_attr: Storage for device attribute max_freq_khz
> - * @mix_freq_khz_dev_attr: Storage for device attribute min_freq_khz
> - * @initial_max_freq_khz_dev_attr: Storage for device attribute initial_max_freq_khz
> - * @initial_min_freq_khz_dev_attr: Storage for device attribute initial_min_freq_khz
> - * @current_freq_khz_dev_attr: Storage for device attribute current_freq_khz
> - * @domain_id_dev_attr: Storage for device attribute domain_id
> - * @fabric_cluster_id_dev_attr: Storage for device attribute fabric_cluster_id
> - * @package_id_dev_attr: Storage for device attribute package_id
> + * @max_freq_khz_kobj_attr: Storage for kobject attribute max_freq_khz
> + * @mix_freq_khz_kobj_attr: Storage for kobject attribute min_freq_khz
> + * @initial_max_freq_khz_kobj_attr: Storage for kobject attribute initial_max_freq_khz
> + * @initial_min_freq_khz_kobj_attr: Storage for kobject attribute initial_min_freq_khz
> + * @current_freq_khz_kobj_attr: Storage for kobject attribute current_freq_khz
> + * @domain_id_kobj_attr: Storage for kobject attribute domain_id
> + * @fabric_cluster_id_kobj_attr: Storage for kobject attribute fabric_cluster_id
> + * @package_id_kobj_attr: Storage for kobject attribute package_id
> * @uncore_attrs: Attribute storage for group creation
> *
> * This structure is used to encapsulate all data related to uncore sysfs
> @@ -53,14 +53,14 @@ struct uncore_data {
> char name[32];
>
> struct attribute_group uncore_attr_group;
> - struct device_attribute max_freq_khz_dev_attr;
> - struct device_attribute min_freq_khz_dev_attr;
> - struct device_attribute initial_max_freq_khz_dev_attr;
> - struct device_attribute initial_min_freq_khz_dev_attr;
> - struct device_attribute current_freq_khz_dev_attr;
> - struct device_attribute domain_id_dev_attr;
> - struct device_attribute fabric_cluster_id_dev_attr;
> - struct device_attribute package_id_dev_attr;
> + struct kobj_attribute max_freq_khz_kobj_attr;
> + struct kobj_attribute min_freq_khz_kobj_attr;
> + struct kobj_attribute initial_max_freq_khz_kobj_attr;
> + struct kobj_attribute initial_min_freq_khz_kobj_attr;
> + struct kobj_attribute current_freq_khz_kobj_attr;
> + struct kobj_attribute domain_id_kobj_attr;
> + struct kobj_attribute fabric_cluster_id_kobj_attr;
> + struct kobj_attribute package_id_kobj_attr;
> struct attribute *uncore_attrs[9];
> };
>
>
> ---
> base-commit: 236f7d8034ff401d02fa6d74bae494a2b54e1834
> change-id: 20240104-intel-uncore-freq-kcfi-fix-6ef07053db58
>
> Best regards,
^ permalink raw reply [flat|nested] 6+ messages in thread