[tip: perf/core] perf/core: Convert snprintf() to scnprintf()

[tip: perf/core] perf/core: Convert snprintf() to scnprintf()

[tip-bot2 for Jules Irenge]

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 <jbi.octave@gmail.com>
AuthorDate:    Sun, 18 Sep 2022 00:41:08 +01:00
Committer:     Ingo Molnar <mingo@kernel.org>
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 <jbi.octave@gmail.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 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);

Re: [tip: perf/core] perf/core: Convert snprintf() to scnprintf()

[Peter Zijlstra]

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 <jbi.octave@gmail.com>
> AuthorDate:    Sun, 18 Sep 2022 00:41:08 +01:00
> Committer:     Ingo Molnar <mingo@kernel.org>
> 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.

[tip: perf/core] perf/core: Convert snprintf() to scnprintf()

[tip-bot2 for Jules Irenge]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     dca6344d7a77dd0501a73745f4a9fb1ee2bc9d7c
Gitweb:        https://git.kernel.org/tip/dca6344d7a77dd0501a73745f4a9fb1ee2bc9d7c
Author:        Jules Irenge <jbi.octave@gmail.com>
AuthorDate:    Sun, 18 Sep 2022 00:41:08 +01:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 21 Sep 2022 12:34:36 +02:00

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.

[ mingo: Improved the changelog & reviewed the usage sites. ]

Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 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);

Re: [tip: perf/core] perf/core: Convert snprintf() to scnprintf()

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> 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 <jbi.octave@gmail.com>
> > AuthorDate:    Sun, 18 Sep 2022 00:41:08 +01:00
> > Committer:     Ingo Molnar <mingo@kernel.org>
> > 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

Re: [tip: perf/core] perf/core: Convert snprintf() to scnprintf()

[Jules Irenge]

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 <jbi.octave@gmail.com>
> > AuthorDate:    Sun, 18 Sep 2022 00:41:08 +01:00
> > Committer:     Ingo Molnar <mingo@kernel.org>
> > 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

Re: [tip: perf/core] perf/core: Convert snprintf() to scnprintf()

[David Sterba]

On Wed, Sep 21, 2022 at 12:44:35PM +0200, Ingo Molnar wrote:
> 
> * Peter Zijlstra <peterz@infradead.org> 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 <jbi.octave@gmail.com>
> > > AuthorDate:    Sun, 18 Sep 2022 00:41:08 +01:00
> > > Committer:     Ingo Molnar <mingo@kernel.org>
> > > 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.

Re: [tip: perf/core] perf/core: Convert snprintf() to scnprintf()

[Jules Irenge]

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 <peterz@infradead.org> 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 <jbi.octave@gmail.com>
> > > > AuthorDate:    Sun, 18 Sep 2022 00:41:08 +01:00
> > > > Committer:     Ingo Molnar <mingo@kernel.org>
> > > > 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