The following commit has been merged into the perf/core branch of tip:
Commit-ID: 678739d622ae7b75b62d550858b6bf104c43e2df
Gitweb: https://git.kernel.org/tip/678739d622ae7b75b62d550858b6bf104c43e2df
Author: Jules Irenge <[email protected]>
AuthorDate: Sun, 18 Sep 2022 00:41:08 +01:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Wed, 21 Sep 2022 10:01:20 +02:00
perf/core: Convert snprintf() to scnprintf()
Coccinelle reports a warning:
WARNING: use scnprintf or sprintf
Adding to that, there has also been some slow migration from snprintf to scnprintf.
This LWN article explains the rationale for this change:
https: //lwn.net/Articles/69419/
No change in behavior.
[ mingo: Improved the changelog. ]
Signed-off-by: Jules Irenge <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
kernel/events/core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 7da5515..c07e9a3 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10952,7 +10952,7 @@ static ssize_t nr_addr_filters_show(struct device *dev,
{
struct pmu *pmu = dev_get_drvdata(dev);
- return snprintf(page, PAGE_SIZE - 1, "%d\n", pmu->nr_addr_filters);
+ return scnprintf(page, PAGE_SIZE - 1, "%d\n", pmu->nr_addr_filters);
}
DEVICE_ATTR_RO(nr_addr_filters);
@@ -10963,7 +10963,7 @@ type_show(struct device *dev, struct device_attribute *attr, char *page)
{
struct pmu *pmu = dev_get_drvdata(dev);
- return snprintf(page, PAGE_SIZE-1, "%d\n", pmu->type);
+ return scnprintf(page, PAGE_SIZE - 1, "%d\n", pmu->type);
}
static DEVICE_ATTR_RO(type);
@@ -10974,7 +10974,7 @@ perf_event_mux_interval_ms_show(struct device *dev,
{
struct pmu *pmu = dev_get_drvdata(dev);
- return snprintf(page, PAGE_SIZE-1, "%d\n", pmu->hrtimer_interval_ms);
+ return scnprintf(page, PAGE_SIZE - 1, "%d\n", pmu->hrtimer_interval_ms);
}
static DEFINE_MUTEX(mux_interval_mutex);
On Wed, Sep 21, 2022 at 08:08:55AM -0000, tip-bot2 for Jules Irenge wrote:
> The following commit has been merged into the perf/core branch of tip:
>
> Commit-ID: 678739d622ae7b75b62d550858b6bf104c43e2df
> Gitweb: https://git.kernel.org/tip/678739d622ae7b75b62d550858b6bf104c43e2df
> Author: Jules Irenge <[email protected]>
> AuthorDate: Sun, 18 Sep 2022 00:41:08 +01:00
> Committer: Ingo Molnar <[email protected]>
> CommitterDate: Wed, 21 Sep 2022 10:01:20 +02:00
>
> perf/core: Convert snprintf() to scnprintf()
>
> Coccinelle reports a warning:
>
> WARNING: use scnprintf or sprintf
>
> Adding to that, there has also been some slow migration from snprintf to scnprintf.
>
> This LWN article explains the rationale for this change:
>
> https: //lwn.net/Articles/69419/
>
> No change in behavior.
>
> [ mingo: Improved the changelog. ]
And yet, at this point I still have no clue what's wrong with
snprintf(). So not much improvement :/
As such I'm still very much against this patch.
* Peter Zijlstra <[email protected]> wrote:
> On Wed, Sep 21, 2022 at 08:08:55AM -0000, tip-bot2 for Jules Irenge wrote:
> > The following commit has been merged into the perf/core branch of tip:
> >
> > Commit-ID: 678739d622ae7b75b62d550858b6bf104c43e2df
> > Gitweb: https://git.kernel.org/tip/678739d622ae7b75b62d550858b6bf104c43e2df
> > Author: Jules Irenge <[email protected]>
> > AuthorDate: Sun, 18 Sep 2022 00:41:08 +01:00
> > Committer: Ingo Molnar <[email protected]>
> > CommitterDate: Wed, 21 Sep 2022 10:01:20 +02:00
> >
> > perf/core: Convert snprintf() to scnprintf()
> >
> > Coccinelle reports a warning:
> >
> > WARNING: use scnprintf or sprintf
> >
> > Adding to that, there has also been some slow migration from snprintf to scnprintf.
> >
> > This LWN article explains the rationale for this change:
> >
> > https: //lwn.net/Articles/69419/
> >
> > No change in behavior.
> >
> > [ mingo: Improved the changelog. ]
>
> And yet, at this point I still have no clue what's wrong with
> snprintf(). So not much improvement :/
I've added this to the changelog:
perf/core: Convert snprintf() to scnprintf()
Coccinelle reports a warning:
WARNING: use scnprintf or sprintf
This LWN article explains the rationale for this change:
https: //lwn.net/Articles/69419/
Ie. snprintf() returns what *would* be the resulting length,
while scnprintf() returns the actual length.
Adding to that, there has also been some slow migration from snprintf to scnprintf,
here's the shift in usage in the past 3.5 years, in all fs/ files:
v5.0 v6.0-rc6
--------------------------------------
snprintf() uses: 63 213
scnprintf() uses: 374 186
No intended change in behavior.
I also reviewed the usage sites - this patch could in fact be a bugfix as
we are passing back these lengths to the VFS, which lengths are arguably
bogus in the snprintf() case, should we ever get close to the buffer limits
to trigger output truncation - which with PAGE_SIZE-1 is very unlikely.
[ Anyway, the shift in interface usage for FS code makes it pretty clear
that when in doubt we should use scnprintf() for FS code. snprintf() is
arguably actively dangerous whenever it works differently from sprintf()
... ]
Thanks,
Ingo
On Wed, Sep 21, 2022 at 10:34:35AM +0200, Peter Zijlstra wrote:
> On Wed, Sep 21, 2022 at 08:08:55AM -0000, tip-bot2 for Jules Irenge wrote:
> > The following commit has been merged into the perf/core branch of tip:
> >
> > Commit-ID: 678739d622ae7b75b62d550858b6bf104c43e2df
> > Gitweb: https://git.kernel.org/tip/678739d622ae7b75b62d550858b6bf104c43e2df
> > Author: Jules Irenge <[email protected]>
> > AuthorDate: Sun, 18 Sep 2022 00:41:08 +01:00
> > Committer: Ingo Molnar <[email protected]>
> > CommitterDate: Wed, 21 Sep 2022 10:01:20 +02:00
> >
> > perf/core: Convert snprintf() to scnprintf()
> >
> > Coccinelle reports a warning:
> >
> > WARNING: use scnprintf or sprintf
> >
> > Adding to that, there has also been some slow migration from snprintf to scnprintf.
> >
> > This LWN article explains the rationale for this change:
> >
> > https: //lwn.net/Articles/69419/
> >
> > No change in behavior.
> >
> > [ mingo: Improved the changelog. ]
>
> And yet, at this point I still have no clue what's wrong with
> snprintf(). So not much improvement :/
>
> As such I'm still very much against this patch.
Hi Peter,
Thanks for the feedback,
My bad, I am still a newbie. I will try to improve on my changelog next time.
But I have learned that the difference is as Ingo pointed out:
snprintf return the length of the buffer to be written with assumption it all fits in the destination array
while scnprintf return the actual length that fit in the destination array(eg. buf below).
This is just by precaution or safety in mind in case the PAGE - 1 is
overun.
I did some digging and came up with a code like this for the corner
case.
#define BUFSIZE 4
static int __init my_init(void)
{
char buf[BUFSIZE];
int x,y;
x = snprintf(buf, BUFSIZE, "Linux"); // length is 5 here : return length of expected to be written when the BUFFSIZE is 4
pr_info(" With length %d, The string is %s\n", x, buf);
y = scnprintf(buf, BUFSIZE, "Linux"); //length is 3 : return length of what is actually written to buff
pr_info(" With length %d, The string is %s\n", y, buf);
return 0;
}
I appreciate any comment as I am on learning journey.
Thank you,
Jules
On Wed, Sep 21, 2022 at 12:44:35PM +0200, Ingo Molnar wrote:
>
> * Peter Zijlstra <[email protected]> wrote:
>
> > On Wed, Sep 21, 2022 at 08:08:55AM -0000, tip-bot2 for Jules Irenge wrote:
> > > The following commit has been merged into the perf/core branch of tip:
> > >
> > > Commit-ID: 678739d622ae7b75b62d550858b6bf104c43e2df
> > > Gitweb: https://git.kernel.org/tip/678739d622ae7b75b62d550858b6bf104c43e2df
> > > Author: Jules Irenge <[email protected]>
> > > AuthorDate: Sun, 18 Sep 2022 00:41:08 +01:00
> > > Committer: Ingo Molnar <[email protected]>
> > > CommitterDate: Wed, 21 Sep 2022 10:01:20 +02:00
> > >
> > > perf/core: Convert snprintf() to scnprintf()
> > >
> > > Coccinelle reports a warning:
> > >
> > > WARNING: use scnprintf or sprintf
> > >
> > > Adding to that, there has also been some slow migration from snprintf to scnprintf.
> > >
> > > This LWN article explains the rationale for this change:
> > >
> > > https: //lwn.net/Articles/69419/
> > >
> > > No change in behavior.
> > >
> > > [ mingo: Improved the changelog. ]
> >
> > And yet, at this point I still have no clue what's wrong with
> > snprintf(). So not much improvement :/
>
> I've added this to the changelog:
>
> perf/core: Convert snprintf() to scnprintf()
I'm not sure if it would apply in this case as it's for a device
attribute, but there's another helper sysfs_emit that does the safe
print to string and one does not have to care which flavor of s*printf
it is. We had patches in btrfs converting from snprintf to scnprintf and
the latest one is sysfs_emit which is convenient to use but assumes the
PAGE_SIZE of the buffer.
On Wed, Sep 21, 2022 at 02:55:35PM +0200, David Sterba wrote:
> On Wed, Sep 21, 2022 at 12:44:35PM +0200, Ingo Molnar wrote:
> >
> > * Peter Zijlstra <[email protected]> wrote:
> >
> > > On Wed, Sep 21, 2022 at 08:08:55AM -0000, tip-bot2 for Jules Irenge wrote:
> > > > The following commit has been merged into the perf/core branch of tip:
> > > >
> > > > Commit-ID: 678739d622ae7b75b62d550858b6bf104c43e2df
> > > > Gitweb: https://git.kernel.org/tip/678739d622ae7b75b62d550858b6bf104c43e2df
> > > > Author: Jules Irenge <[email protected]>
> > > > AuthorDate: Sun, 18 Sep 2022 00:41:08 +01:00
> > > > Committer: Ingo Molnar <[email protected]>
> > > > CommitterDate: Wed, 21 Sep 2022 10:01:20 +02:00
> > > >
> > > > perf/core: Convert snprintf() to scnprintf()
> > > >
> > > > Coccinelle reports a warning:
> > > >
> > > > WARNING: use scnprintf or sprintf
> > > >
> > > > Adding to that, there has also been some slow migration from snprintf to scnprintf.
> > > >
> > > > This LWN article explains the rationale for this change:
> > > >
> > > > https: //lwn.net/Articles/69419/
> > > >
> > > > No change in behavior.
> > > >
> > > > [ mingo: Improved the changelog. ]
> > >
> > > And yet, at this point I still have no clue what's wrong with
> > > snprintf(). So not much improvement :/
> >
> > I've added this to the changelog:
> >
> > perf/core: Convert snprintf() to scnprintf()
>
> I'm not sure if it would apply in this case as it's for a device
> attribute, but there's another helper sysfs_emit that does the safe
> print to string and one does not have to care which flavor of s*printf
> it is. We had patches in btrfs converting from snprintf to scnprintf and
> the latest one is sysfs_emit which is convenient to use but assumes the
> PAGE_SIZE of the buffer.
Yes, you are right. I can resend the patch with sysfs_emit() if
possible as the latest documentation on sysfs states that
show() device function should only use sysfs_emit() or sysfs_emit_at()
when formatting the value to be returned to user space.
* https://www.kernel.org/doc/html/latest/filesystems/sysfs.html
I don't know whether it may apply to this subsystem. I have to read
more about it and test
thanks
jules