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@xxxxxxxxxx> > --- > 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,
