linux-kernel - Re: [PATCH 05/42] arch/x86/events/intel: Convert snprintf to sysfs

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <afd87194-d21a-4d4a-a984-1e340252a6a8@fujitsu.com>
Date: Mon, 22 Jan 2024 02:59:17 +0000
From: "Zhijian Li (Fujitsu)" <lizhijian@...itsu.com>
To: Namhyung Kim <namhyung@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Peter
 Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, Arnaldo
 Carvalho de Melo <acme@...nel.org>, Mark Rutland <mark.rutland@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa
	<jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>, Adrian Hunter
	<adrian.hunter@...el.com>, Thomas Gleixner <tglx@...utronix.de>, Borislav
 Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
	"x86@...nel.org" <x86@...nel.org>, "H. Peter Anvin" <hpa@...or.com>,
	"linux-perf-users@...r.kernel.org" <linux-perf-users@...r.kernel.org>
Subject: Re: [PATCH 05/42] arch/x86/events/intel: Convert snprintf to
 sysfs_emit



On 20/01/2024 06:00, Namhyung Kim wrote:
> Hello,
> 
> On Mon, Jan 15, 2024 at 8:53 PM Li Zhijian <lizhijian@...itsu.com> wrote:
>>
>> Per filesystems/sysfs.rst, show() should only use sysfs_emit()
>> or sysfs_emit_at() when formatting the value to be returned to user space.
>>
>> coccinelle complains that there are still a couple of functions that use
>> snprintf(). Convert them to sysfs_emit().
>>
>>> ./arch/x86/events/intel/core.c:5496:8-16: WARNING: please use sysfs_emit
>>> ./arch/x86/events/intel/core.c:5530:8-16: WARNING: please use sysfs_emit
>>> ./arch/x86/events/intel/core.c:5546:8-16: WARNING: please use sysfs_emit
>>> ./arch/x86/events/intel/pt.c:99:8-16: WARNING: please use sysfs_emit
>>
>> No functional change intended
>>
>> CC: Peter Zijlstra <peterz@...radead.org>
>> CC: Ingo Molnar <mingo@...hat.com>
>> CC: Arnaldo Carvalho de Melo <acme@...nel.org>
>> CC: Mark Rutland <mark.rutland@....com>
>> CC: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
>> CC: Jiri Olsa <jolsa@...nel.org>
>> CC: Namhyung Kim <namhyung@...nel.org>
>> CC: Ian Rogers <irogers@...gle.com>
>> CC: Adrian Hunter <adrian.hunter@...el.com>
>> CC: Thomas Gleixner <tglx@...utronix.de>
>> CC: Borislav Petkov <bp@...en8.de>
>> CC: Dave Hansen <dave.hansen@...ux.intel.com>
>> CC: x86@...nel.org
>> CC: "H. Peter Anvin" <hpa@...or.com>
>> CC: linux-perf-users@...r.kernel.org
>> Signed-off-by: Li Zhijian <lizhijian@...itsu.com>
>> ---
>>   arch/x86/events/intel/core.c | 6 +++---
>>   arch/x86/events/intel/pt.c   | 2 +-
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index 0f2786d4e405..aa5fa64a923b 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -5493,7 +5493,7 @@ static ssize_t show_sysctl_tfa(struct device *cdev,
>>                                struct device_attribute *attr,
>>                                char *buf)
>>   {
>> -       return snprintf(buf, 40, "%d\n", allow_tsx_force_abort);
>> +       return sysfs_emit(buf, "%d\n", allow_tsx_force_abort);
> 
> I guess the size doesn't matter here..

Indeed, it doesn't matter. But the kernel has already formed a unified convention
(see filesystems/sysfs.rst), and we also have static tools to check this issue.

This fix can both make it compliant with convention and eliminate warnings complained
by the coccinelle.

So I hope that the community can accept these fixes to make all source code
consistent and beautiful.

(2 abused cases were newly introduced in 6.8 merge windows)


> 
> Anyway there's freeze_on_smi_show() even uses sprintf().

Yes, there are still several places using sprintf() which should also convert
to sysfs_emit IMHO. They are alreay in TODOs(see the cover letter). I plan to fix
them in another set if this set goes smoothly.

Thanks
Zhijian



> 
> Thanks,
> Namhyung
> 
>>   }
>>
>>   static ssize_t set_sysctl_tfa(struct device *cdev,
>> @@ -5527,7 +5527,7 @@ static ssize_t branches_show(struct device *cdev,
>>                               struct device_attribute *attr,
>>                               char *buf)
>>   {
>> -       return snprintf(buf, PAGE_SIZE, "%d\n", x86_pmu.lbr_nr);
>> +       return sysfs_emit(buf, "%d\n", x86_pmu.lbr_nr);
>>   }
>>
>>   static DEVICE_ATTR_RO(branches);
>> @@ -5543,7 +5543,7 @@ static ssize_t pmu_name_show(struct device *cdev,
>>                               struct device_attribute *attr,
>>                               char *buf)
>>   {
>> -       return snprintf(buf, PAGE_SIZE, "%s\n", pmu_name_str);
>> +       return sysfs_emit(buf, "%s\n", pmu_name_str);
>>   }
>>
>>   static DEVICE_ATTR_RO(pmu_name);
>> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
>> index 8e2a12235e62..30bba5f3a840 100644
>> --- a/arch/x86/events/intel/pt.c
>> +++ b/arch/x86/events/intel/pt.c
>> @@ -96,7 +96,7 @@ static ssize_t pt_cap_show(struct device *cdev,
>>                  container_of(attr, struct dev_ext_attribute, attr);
>>          enum pt_capabilities cap = (long)ea->var;
>>
>> -       return snprintf(buf, PAGE_SIZE, "%x\n", intel_pt_validate_hw_cap(cap));
>> +       return sysfs_emit(buf, "%x\n", intel_pt_validate_hw_cap(cap));
>>   }
>>
>>   static struct attribute_group pt_cap_group __ro_after_init = {
>> --
>> 2.29.2
>>