[PATCH] sysfs: Add sysfs_emit to replace sprintf to PAGE_SIZE buffers.

[PATCH] sysfs: Add sysfs_emit to replace sprintf to PAGE_SIZE buffers.

[Joe Perches]

sprintf does not know the PAGE_SIZE maximum of the temporary buffer
used for outputting sysfs content requests and it's possible to
overrun the buffer length.

Add a generic sysfs_emit mechanism that knows that the size of the
temporary buffer and ensures that no overrun is done.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/sysfs/file.c       | 30 ++++++++++++++++++++++++++++++
 include/linux/sysfs.h |  8 ++++++++
 2 files changed, 38 insertions(+)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index eb6897ab78e7..06a13bbd7080 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -707,3 +707,33 @@ int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(sysfs_change_owner);
+
+/**
+ *	sysfs_emit - scnprintf equivalent, aware of PAGE_SIZE buffer.
+ *	@buf:	start of PAGE_SIZE buffer.
+ *	@pos:	current position in buffer
+ *              (pos - buf) must always be < PAGE_SIZE
+ *	@fmt:	format
+ *	@...:	arguments to format
+ *
+ *
+ * Returns number of characters written at pos.
+ */
+int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
+{
+	va_list args;
+	bool bad_pos = pos < buf;
+	bool bad_len = (pos - buf) >= PAGE_SIZE;
+	int len;
+
+	if (WARN(bad_pos || bad_len, "(pos < buf):%d (pos >= PAGE_SIZE):%d\n",
+		 bad_pos, bad_len))
+		return 0;
+
+	va_start(args, fmt);
+	len = vscnprintf(pos, PAGE_SIZE - (pos - buf), fmt, args);
+	va_end(args);
+
+	return len;
+}
+EXPORT_SYMBOL_GPL(sysfs_emit);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 34e84122f635..5a21d3d30016 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -329,6 +329,8 @@ int sysfs_groups_change_owner(struct kobject *kobj,
 int sysfs_group_change_owner(struct kobject *kobj,
 			     const struct attribute_group *groups, kuid_t kuid,
 			     kgid_t kgid);
+__printf(3, 4)
+int sysfs_emit(char *buf, char *pos, const char *fmt, ...);
 
 #else /* CONFIG_SYSFS */
 
@@ -576,6 +578,12 @@ static inline int sysfs_group_change_owner(struct kobject *kobj,
 	return 0;
 }
 
+__printf(3, 4)
+static inline int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
+{
+	return 0;
+}
+
 #endif /* CONFIG_SYSFS */
 
 static inline int __must_check sysfs_create_file(struct kobject *kobj,
-- 
2.26.0

Re: [PATCH] sysfs: Add sysfs_emit to replace sprintf to PAGE_SIZE buffers.

[Greg Kroah-Hartman]

On Fri, Aug 28, 2020 at 03:52:13PM -0700, Joe Perches wrote:
> sprintf does not know the PAGE_SIZE maximum of the temporary buffer
> used for outputting sysfs content requests and it's possible to
> overrun the buffer length.
> 
> Add a generic sysfs_emit mechanism that knows that the size of the
> temporary buffer and ensures that no overrun is done.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  fs/sysfs/file.c       | 30 ++++++++++++++++++++++++++++++
>  include/linux/sysfs.h |  8 ++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index eb6897ab78e7..06a13bbd7080 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -707,3 +707,33 @@ int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(sysfs_change_owner);
> +
> +/**
> + *	sysfs_emit - scnprintf equivalent, aware of PAGE_SIZE buffer.
> + *	@buf:	start of PAGE_SIZE buffer.
> + *	@pos:	current position in buffer
> + *              (pos - buf) must always be < PAGE_SIZE

sysfs files are always supposed to be "one value per file", so why would
you ever need a 'pos' variable to show the location in the buffer?

This would encourage people to do things they shouldn't be doing, and it
makes the people who are doing things properly, have to constantly see
'pos' as something that maybe they should be using?

I think if you tried to convert a subsystem to use this (try USB as an
example), you would see why that parameter isn't needed.

thanks,

greg k-h

Re: [PATCH] sysfs: Add sysfs_emit to replace sprintf to PAGE_SIZE buffers.

[Joe Perches]

On Sat, 2020-08-29 at 08:22 +0200, Greg Kroah-Hartman wrote:
> On Fri, Aug 28, 2020 at 03:52:13PM -0700, Joe Perches wrote:
> > sprintf does not know the PAGE_SIZE maximum of the temporary buffer
> > used for outputting sysfs content requests and it's possible to
> > overrun the buffer length.
> > 
> > Add a generic sysfs_emit mechanism that knows that the size of the
> > temporary buffer and ensures that no overrun is done.
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> > ---
> >  fs/sysfs/file.c       | 30 ++++++++++++++++++++++++++++++
> >  include/linux/sysfs.h |  8 ++++++++
> >  2 files changed, 38 insertions(+)
> > 
> > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > index eb6897ab78e7..06a13bbd7080 100644
> > --- a/fs/sysfs/file.c
> > +++ b/fs/sysfs/file.c
> > @@ -707,3 +707,33 @@ int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(sysfs_change_owner);
> > +
> > +/**
> > + *	sysfs_emit - scnprintf equivalent, aware of PAGE_SIZE buffer.
> > + *	@buf:	start of PAGE_SIZE buffer.
> > + *	@pos:	current position in buffer
> > + *              (pos - buf) must always be < PAGE_SIZE
> 
> sysfs files are always supposed to be "one value per file", so why would
> you ever need a 'pos' variable to show the location in the buffer?

I've done treewide conversions using cocci.
It's used all over the place.
Especially in loops with arrays.

Sometimes the output is single line.
Sometimes multiple lines.

Look at the sample conversion of mem_sleep_show I posted earlier.

#ifdef CONFIG_SUSPEND
 static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
                              char *buf)
 {
-       char *s = buf;
+       char *pos = buf;
        suspend_state_t i;
 
        for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++)
                if (mem_sleep_states[i]) {
                        const char *label = mem_sleep_states[i];
 
                        if (mem_sleep_current == i)
-                               s += sprintf(s, "[%s] ", label);
+                               pos += sysfs_emit(buf, pos, "[%s] ", label);
                        else
-                               s += sprintf(s, "%s ", label);
+                               pos += sysfs_emit(buf, pos, "%s ", label);
                }
 
        /* Convert the last space to a newline if needed. */
-       if (s != buf)
-               *(s-1) = '\n';
+       if (pos != buf)
+               *(pos - 1) = '\n';
 
-       return (s - buf);
+       return pos - buf;
 }
 

Re: [PATCH] sysfs: Add sysfs_emit to replace sprintf to PAGE_SIZE buffers.

[Greg Kroah-Hartman]

On Fri, Aug 28, 2020 at 11:41:00PM -0700, Joe Perches wrote:
> On Sat, 2020-08-29 at 08:22 +0200, Greg Kroah-Hartman wrote:
> > On Fri, Aug 28, 2020 at 03:52:13PM -0700, Joe Perches wrote:
> > > sprintf does not know the PAGE_SIZE maximum of the temporary buffer
> > > used for outputting sysfs content requests and it's possible to
> > > overrun the buffer length.
> > > 
> > > Add a generic sysfs_emit mechanism that knows that the size of the
> > > temporary buffer and ensures that no overrun is done.
> > > 
> > > Signed-off-by: Joe Perches <joe@perches.com>
> > > ---
> > >  fs/sysfs/file.c       | 30 ++++++++++++++++++++++++++++++
> > >  include/linux/sysfs.h |  8 ++++++++
> > >  2 files changed, 38 insertions(+)
> > > 
> > > diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> > > index eb6897ab78e7..06a13bbd7080 100644
> > > --- a/fs/sysfs/file.c
> > > +++ b/fs/sysfs/file.c
> > > @@ -707,3 +707,33 @@ int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
> > >  	return 0;
> > >  }
> > >  EXPORT_SYMBOL_GPL(sysfs_change_owner);
> > > +
> > > +/**
> > > + *	sysfs_emit - scnprintf equivalent, aware of PAGE_SIZE buffer.
> > > + *	@buf:	start of PAGE_SIZE buffer.
> > > + *	@pos:	current position in buffer
> > > + *              (pos - buf) must always be < PAGE_SIZE
> > 
> > sysfs files are always supposed to be "one value per file", so why would
> > you ever need a 'pos' variable to show the location in the buffer?
> 
> I've done treewide conversions using cocci.
> It's used all over the place.
> Especially in loops with arrays.
> 
> Sometimes the output is single line.
> Sometimes multiple lines.
> 
> Look at the sample conversion of mem_sleep_show I posted earlier.
> 
> #ifdef CONFIG_SUSPEND
>  static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
>                               char *buf)
>  {
> -       char *s = buf;
> +       char *pos = buf;
>         suspend_state_t i;
>  
>         for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++)
>                 if (mem_sleep_states[i]) {
>                         const char *label = mem_sleep_states[i];
>  
>                         if (mem_sleep_current == i)
> -                               s += sprintf(s, "[%s] ", label);
> +                               pos += sysfs_emit(buf, pos, "[%s] ", label);
>                         else
> -                               s += sprintf(s, "%s ", label);
> +                               pos += sysfs_emit(buf, pos, "%s ", label);
>                 }
>  
>         /* Convert the last space to a newline if needed. */
> -       if (s != buf)
> -               *(s-1) = '\n';
> +       if (pos != buf)
> +               *(pos - 1) = '\n';
>  
> -       return (s - buf);
> +       return pos - buf;
>  }

And again, this is the rare exception, not the rule, please do not make
a generic helper function "easy" to do crazy things like this in sysfs.

Heck, make it explicit, call this function sysfs_emit_pos() and the
non-pos version sysfs_emit().  That way I can easily search for the
"offending" users of the sysfs api.

thanks,

greg k-h

Re: [PATCH] sysfs: Add sysfs_emit to replace sprintf to PAGE_SIZE buffers.

[Denis Efremov]

Hi,

On 8/29/20 1:52 AM, Joe Perches wrote:
> sprintf does not know the PAGE_SIZE maximum of the temporary buffer
> used for outputting sysfs content requests and it's possible to
> overrun the buffer length.
> 
> Add a generic sysfs_emit mechanism that knows that the size of the
> temporary buffer and ensures that no overrun is done.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---


It could be a good idea to update the docs to, i.e.:
https://www.kernel.org/doc/html/latest/filesystems/sysfs.html


>  fs/sysfs/file.c       | 30 ++++++++++++++++++++++++++++++
>  include/linux/sysfs.h |  8 ++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index eb6897ab78e7..06a13bbd7080 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -707,3 +707,33 @@ int sysfs_change_owner(struct kobject *kobj, kuid_t kuid, kgid_t kgid)
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(sysfs_change_owner);
> +
> +/**
> + *	sysfs_emit - scnprintf equivalent, aware of PAGE_SIZE buffer.
> + *	@buf:	start of PAGE_SIZE buffer.
> + *	@pos:	current position in buffer
> + *              (pos - buf) must always be < PAGE_SIZE
> + *	@fmt:	format
> + *	@...:	arguments to format
> + *
> + *
> + * Returns number of characters written at pos.
> + */
> +int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
> +{
> +	va_list args;
> +	bool bad_pos = pos < buf;
> +	bool bad_len = (pos - buf) >= PAGE_SIZE;
> +	int len;
> +
> +	if (WARN(bad_pos || bad_len, "(pos < buf):%d (pos >= PAGE_SIZE):%d\n",
> +		 bad_pos, bad_len))
> +		return 0;
> +
> +	va_start(args, fmt);
> +	len = vscnprintf(pos, PAGE_SIZE - (pos - buf), fmt, args);
> +	va_end(args);
> +
> +	return len;
> +}
> +EXPORT_SYMBOL_GPL(sysfs_emit);
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 34e84122f635..5a21d3d30016 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -329,6 +329,8 @@ int sysfs_groups_change_owner(struct kobject *kobj,
>  int sysfs_group_change_owner(struct kobject *kobj,
>  			     const struct attribute_group *groups, kuid_t kuid,
>  			     kgid_t kgid);
> +__printf(3, 4)
> +int sysfs_emit(char *buf, char *pos, const char *fmt, ...);
>  
>  #else /* CONFIG_SYSFS */
>  
> @@ -576,6 +578,12 @@ static inline int sysfs_group_change_owner(struct kobject *kobj,
>  	return 0;
>  }
>  
> +__printf(3, 4)
> +static inline int sysfs_emit(char *buf, char *pos, const char *fmt, ...)
> +{
> +	return 0;
> +}
> +
>  #endif /* CONFIG_SYSFS */
>  
>  static inline int __must_check sysfs_create_file(struct kobject *kobj,
> 

Thanks,
Denis

Re: [PATCH] sysfs: Add sysfs_emit to replace sprintf to PAGE_SIZE buffers.

[Joe Perches]

On Sat, 2020-08-29 at 09:59 +0300, Denis Efremov wrote:
> Hi,
> 
> On 8/29/20 1:52 AM, Joe Perches wrote:
> > sprintf does not know the PAGE_SIZE maximum of the temporary buffer
> > used for outputting sysfs content requests and it's possible to
> > overrun the buffer length.
> > 
> > Add a generic sysfs_emit mechanism that knows that the size of the
> > temporary buffer and ensures that no overrun is done.
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> > --- rK
> 
> It could be a good idea to update the docs to, i.e.:
> https://www.kernel.org/doc/html/latest/filesystems/sysfs.html

Yes, thanks.

I have the below already, but Greg makes a sensible point
about the generic use of sysfs_emit for single values
which is ~95% of the actual uses, so likely there will be
two functions.  Given the multiple thousand instances,
using 2 functions would be smaller overall object code
as well.

Perhaps:

sysfs_emit (for single value output)
sysfs_emit_at (or sysfs_emit_pos ? or some better name?)

	int sysfs_emit(char *buf, const char *fmt, ...)
	int sysfs_emit_at(char *buf, int pos, const char *fmt, ...)

or maybe use

	int sysfs_emit_pos(char *buf, char *pos, const char *fmt, ...)

The multiple use emit_at with int as the 2nd parameter would
make the direct return easier than the char * which needs a
subtraction.

six of this/half dozen of that...

cheers...

---

Anyway, this will need updating, likely with better examples.

diff --git a/Documentation/filesystems/sysfs.rst b/Documentation/filesystems/sysfs.rst
index ab0f7795792b..13c7a86fa6c8 100644
--- a/Documentation/filesystems/sysfs.rst
+++ b/Documentation/filesystems/sysfs.rst
@@ -242,12 +242,9 @@ Other notes:
   is 4096.
 
 - show() methods should return the number of bytes printed into the
-  buffer. This is the return value of scnprintf().
+  buffer. This is the return value of sysfs_emit().
 
-- show() must not use snprintf() when formatting the value to be
-  returned to user space. If you can guarantee that an overflow
-  will never happen you can use sprintf() otherwise you must use
-  scnprintf().
+- show() methods should only use sysfs_emit to format output.

Re: [PATCH] sysfs: Add sysfs_emit to replace sprintf to PAGE_SIZE buffers.

[Denis Efremov]

> 
> Anyway, this will need updating, likely with better examples.
> 
> diff --git a/Documentation/filesystems/sysfs.rst b/Documentation/filesystems/sysfs.rst
> index ab0f7795792b..13c7a86fa6c8 100644
> --- a/Documentation/filesystems/sysfs.rst
> +++ b/Documentation/filesystems/sysfs.rst
> @@ -242,12 +242,9 @@ Other notes:
>    is 4096.
>  
>  - show() methods should return the number of bytes printed into the
> -  buffer. This is the return value of scnprintf().
> +  buffer. This is the return value of sysfs_emit().
>  
> -- show() must not use snprintf() when formatting the value to be
> -  returned to user space. If you can guarantee that an overflow
> -  will never happen you can use sprintf() otherwise you must use
> -  scnprintf().
> +- show() methods should only use sysfs_emit to format output.
> 

I think it's good to reflect in docs that sysfs_emit_at/sysfs_emit_pos is
only for "legacy" code and should not be used in new code (checkpatch.pl warning?)
because of sysfs design principles.
And something about newlines "General rule is to add newlines at the end of output."

Thanks,
Denis

Re: [PATCH] sysfs: Add sysfs_emit to replace sprintf to PAGE_SIZE buffers.

[Joe Perches]

On Sun, 2020-08-30 at 00:53 +0300, Denis Efremov wrote:
> > Anyway, this will need updating, likely with better examples.
[]
> I think it's good to reflect in docs that sysfs_emit_at/sysfs_emit_pos is
> only for "legacy" code and should not be used in new code (checkpatch.pl warning?)
> because of sysfs design principles.

sysfs_emit_at is also used for arrays.