LinuxLists.cc - [PATCH V2] sysfs: Add sysfs_emit and sysfs_emit

2020-08-29 23:49:25

Subject: [PATCH V2] sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs output

Output defects can exist in sysfs content using sprintf and snprintf.

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

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

Add a generic sysfs_emit_at function that can be used in multiple
call situations that also ensures that no overrun is done.

Signed-off-by: Joe Perches <[email protected]>
---

V2: Simplify sysfs_emit and add sysfs_emit_at
Include Documentation change

Documentation/filesystems/sysfs.rst | 8 ++---
fs/sysfs/file.c | 49 +++++++++++++++++++++++++++++
include/linux/sysfs.h | 15 +++++++++
3 files changed, 67 insertions(+), 5 deletions(-)

diff --git a/Documentation/filesystems/sysfs.rst b/Documentation/filesystems/sysfs.rst
index ab0f7795792b..d44249050f4a 100644
--- a/Documentation/filesystems/sysfs.rst
+++ b/Documentation/filesystems/sysfs.rst
@@ -242,12 +242,10 @@ Other notes:
is 4096.

- show() methods should return the number of bytes printed into the
- buffer. This is the return value of scnprintf().
+ buffer.

-- 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() should only use sysfs_emit() or sysfs_emit_at() when formatting
+ the value to be returned to user space.

- store() should return the number of bytes used from the buffer. If the
entire buffer has been used, just return the count argument.
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index eb6897ab78e7..e8c6d20bab8e 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -707,3 +707,52 @@ 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.
+ * @fmt: format
+ * @...: optional arguments to @format
+ *
+ *
+ * Returns number of characters written to @buf.
+ */
+int sysfs_emit(char *buf, const char *fmt, ...)
+{
+ va_list args;
+ int len;
+
+ va_start(args, fmt);
+ len = vscnprintf(buf, PAGE_SIZE, fmt, args);
+ va_end(args);
+
+ return len;
+}
+EXPORT_SYMBOL_GPL(sysfs_emit);
+
+/**
+ * sysfs_emit_at - scnprintf equivalent, aware of PAGE_SIZE buffer.
+ * @buf: start of PAGE_SIZE buffer.
+ * @at: offset in @buf to start write in bytes
+ * @at must be >= 0 && < PAGE_SIZE
+ * @fmt: format
+ * @...: optional arguments to @fmt
+ *
+ *
+ * Returns number of characters written starting at &@buf[@at].
+ */
+int sysfs_emit_at(char *buf, int at, const char *fmt, ...)
+{
+ va_list args;
+ int len;
+
+ if (WARN(at < 0 || at >= PAGE_SIZE, "invalid sysfs_emit_at: %d\n", at))
+ return 0;
+
+ va_start(args, fmt);
+ len = vscnprintf(buf + at, PAGE_SIZE - at, fmt, args);
+ va_end(args);
+
+ return len;
+}
+EXPORT_SYMBOL_GPL(sysfs_emit_at);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 34e84122f635..2caa34c1ca1a 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -329,6 +329,10 @@ 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(2, 3)
+int sysfs_emit(char *buf, const char *fmt, ...);
+__printf(3, 4)
+int sysfs_emit_at(char *buf, int at, const char *fmt, ...);

#else /* CONFIG_SYSFS */

@@ -576,6 +580,17 @@ static inline int sysfs_group_change_owner(struct kobject *kobj,
return 0;
}

+__printf(2, 3)
+static inline int sysfs_emit(char *buf, const char *fmt, ...)
+{
+ return 0;
+}
+
+__printf(3, 4)
+static inline int sysfs_emit_at(char *buf, int at, const char *fmt, ...)
+{
+ return 0;
+}
#endif /* CONFIG_SYSFS */

static inline int __must_check sysfs_create_file(struct kobject *kobj,
--
2.26.0

2020-08-30 00:45:31

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH V2] sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs output

On Sat, 2020-08-29 at 16:48 -0700, Joe Perches wrote:
> Output defects can exist in sysfs content using sprintf and snprintf.
>
> sprintf does not know the PAGE_SIZE maximum of the temporary buffer
> used for outputting sysfs content and it's possible to overrun the
> PAGE_SIZE buffer length.
>
> Add a generic sysfs_emit function that knows that the size of the
> temporary buffer and ensures that no overrun is done.
>
> Add a generic sysfs_emit_at function that can be used in multiple
> call situations that also ensures that no overrun is done.

This preliminary coccinelle script converts ~5000 instances treewide.
There are still many remaining instances that could be converted.

$ git grep -w sysfs_emit -- '*.[ch]'|wc -l
4702
$ git grep -w sysfs_emit_at -- '*.[ch]'|wc -l
229

$ cat sysfs_emit.cocci
@@
identifier d_show =~ "^.*show.*$";
identifier arg1, arg2, arg3;
@@
ssize_t d_show(struct device *
- arg1
+ dev
, struct device_attribute *
- arg2
+ attr
, char *
- arg3
+ buf
)
{
...
(
- arg1
+ dev
|
- arg2
+ attr
|
- arg3
+ buf
)
...
}

@@
identifier d_show =~ "^.*show.*$";
identifier dev, attr, buf;
@@

ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
{
<...
return
- sprintf(buf,
+ sysfs_emit(buf,
...);
...>
}

@@
identifier d_show =~ "^.*show.*$";
identifier dev, attr, buf;
@@

ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
{
<...
return
- snprintf(buf, PAGE_SIZE,
+ sysfs_emit(buf,
...);
...>
}

@@
identifier d_show =~ "^.*show.*$";
identifier dev, attr, buf;
@@

ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
{
<...
return
- scnprintf(buf, PAGE_SIZE,
+ sysfs_emit(buf,
...);
...>
}

@@
identifier d_show =~ "^.*show.*$";
identifier dev, attr, buf;
expression chr;
@@

ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
{
<...
return
- strcpy(buf, chr);
+ sysfs_emit(buf, chr);
...>
}

@@
identifier d_show =~ "^.*show.*$";
identifier dev, attr, buf;
identifier len;
@@

ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
{
<...
len =
- sprintf(buf,
+ sysfs_emit(buf,
...);
...>
return len;
}

@@
identifier d_show =~ "^.*show.*$";
identifier dev, attr, buf;
identifier len;
@@

ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
{
<...
len =
- snprintf(buf, PAGE_SIZE,
+ sysfs_emit(buf,
...);
...>
return len;
}

@@
identifier d_show =~ "^.*show.*$";
identifier dev, attr, buf;
identifier len;
@@

ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
{
<...
len =
- scnprintf(buf, PAGE_SIZE,
+ sysfs_emit(buf,
...);
...>
return len;
}

@@
identifier d_show =~ "^.*show.*$";
identifier dev, attr, buf;
identifier len;
@@

ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
{
<...
- len += scnprintf(buf + len, PAGE_SIZE - len,
+ len += sysfs_emit_at(buf, len,
...);
...>
return len;
}

@@
identifier d_show =~ "^.*show.*$";
identifier dev, attr, buf;
expression chr;
@@

ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
{
- strcpy(buf, chr);
- return strlen(buf);
+ return sysfs_emit(buf, chr);
}

@@
identifier k_show =~ "^.*show.*$";
identifier arg1, arg2, arg3;
@@
ssize_t k_show(struct kobject *
- arg1
+ kobj
, struct kobj_attribute *
- arg2
+ attr
, char *
- arg3
+ buf
)
{
...
(
- arg1
+ kobj
|
- arg2
+ attr
|
- arg3
+ buf
)
...
}

@@
identifier k_show =~ "^.*show.*$";
identifier kobj, attr, buf;
@@

ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
{
<...
return
- sprintf(buf,
+ sysfs_emit(buf,
...);
...>
}

@@
identifier k_show =~ "^.*show.*$";
identifier kobj, attr, buf;
@@

ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
{
<...
return
- snprintf(buf, PAGE_SIZE,
+ sysfs_emit(buf,
...);
...>
}

@@
identifier k_show =~ "^.*show.*$";
identifier kobj, attr, buf;
@@

ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
{
<...
return
- scnprintf(buf, PAGE_SIZE,
+ sysfs_emit(buf,
...);
...>
}

@@
identifier k_show =~ "^.*show.*$";
identifier kobj, attr, buf;
expression chr;
@@

ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
{
<...
return
- strcpy(buf, chr);
+ sysfs_emit(buf, chr);
...>
}

@@
identifier k_show =~ "^.*show.*$";
identifier kobj, attr, buf;
identifier len;
@@

ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
{
<...
len =
- sprintf(buf,
+ sysfs_emit(buf,
...);
...>
return len;
}

@@
identifier k_show =~ "^.*show.*$";
identifier kobj, attr, buf;
identifier len;
@@

ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
{
<...
len =
- snprintf(buf, PAGE_SIZE,
+ sysfs_emit(buf,
...);
...>
return len;
}

@@
identifier k_show =~ "^.*show.*$";
identifier kobj, attr, buf;
identifier len;
@@

ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
{
<...
len =
- scnprintf(buf, PAGE_SIZE,
+ sysfs_emit(buf,
...);
...>
return len;
}

@@
identifier k_show =~ "^.*show.*$";
identifier kobj, attr, buf;
identifier len;
@@

ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
{
<...
- len += scnprintf(buf + len, PAGE_SIZE - len,
+ len += sysfs_emit_at(buf, len,
...);
...>
return len;
}

@@
identifier k_show =~ "^.*show.*$";
identifier kobj, attr, buf;
expression chr;
@@

ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
{
- strcpy(buf, chr);
- return strlen(buf);
+ return sysfs_emit(buf, chr);
}

2020-08-30 09:04:02

by Julia Lawall

[permalink] [raw]

Subject: Re: [PATCH V2] sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs output

On Sat, 29 Aug 2020, Joe Perches wrote:

> On Sat, 2020-08-29 at 16:48 -0700, Joe Perches wrote:
> > Output defects can exist in sysfs content using sprintf and snprintf.
> >
> > sprintf does not know the PAGE_SIZE maximum of the temporary buffer
> > used for outputting sysfs content and it's possible to overrun the
> > PAGE_SIZE buffer length.
> >
> > Add a generic sysfs_emit function that knows that the size of the
> > temporary buffer and ensures that no overrun is done.
> >
> > Add a generic sysfs_emit_at function that can be used in multiple
> > call situations that also ensures that no overrun is done.
>
> This preliminary coccinelle script converts ~5000 instances treewide.
> There are still many remaining instances that could be converted.

Except for the issue with the ...s that has been discussed, this looks
basically reasonable to me.

julia

>
> $ git grep -w sysfs_emit -- '*.[ch]'|wc -l
> 4702
> $ git grep -w sysfs_emit_at -- '*.[ch]'|wc -l
> 229
>
> $ cat sysfs_emit.cocci
> @@
> identifier d_show =~ "^.*show.*$";
> identifier arg1, arg2, arg3;
> @@
> ssize_t d_show(struct device *
> - arg1
> + dev
> , struct device_attribute *
> - arg2
> + attr
> , char *
> - arg3
> + buf
> )
> {
> ...
> (
> - arg1
> + dev
> |
> - arg2
> + attr
> |
> - arg3
> + buf
> )
> ...
> }
>
> @@
> identifier d_show =~ "^.*show.*$";
> identifier dev, attr, buf;
> @@
>
> ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> <...
> return
> - sprintf(buf,
> + sysfs_emit(buf,
> ...);
> ...>
> }
>
> @@
> identifier d_show =~ "^.*show.*$";
> identifier dev, attr, buf;
> @@
>
> ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> <...
> return
> - snprintf(buf, PAGE_SIZE,
> + sysfs_emit(buf,
> ...);
> ...>
> }
>
> @@
> identifier d_show =~ "^.*show.*$";
> identifier dev, attr, buf;
> @@
>
> ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> <...
> return
> - scnprintf(buf, PAGE_SIZE,
> + sysfs_emit(buf,
> ...);
> ...>
> }
>
> @@
> identifier d_show =~ "^.*show.*$";
> identifier dev, attr, buf;
> expression chr;
> @@
>
> ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> <...
> return
> - strcpy(buf, chr);
> + sysfs_emit(buf, chr);
> ...>
> }
>
> @@
> identifier d_show =~ "^.*show.*$";
> identifier dev, attr, buf;
> identifier len;
> @@
>
> ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> <...
> len =
> - sprintf(buf,
> + sysfs_emit(buf,
> ...);
> ...>
> return len;
> }
>
> @@
> identifier d_show =~ "^.*show.*$";
> identifier dev, attr, buf;
> identifier len;
> @@
>
> ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> <...
> len =
> - snprintf(buf, PAGE_SIZE,
> + sysfs_emit(buf,
> ...);
> ...>
> return len;
> }
>
> @@
> identifier d_show =~ "^.*show.*$";
> identifier dev, attr, buf;
> identifier len;
> @@
>
> ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> <...
> len =
> - scnprintf(buf, PAGE_SIZE,
> + sysfs_emit(buf,
> ...);
> ...>
> return len;
> }
>
> @@
> identifier d_show =~ "^.*show.*$";
> identifier dev, attr, buf;
> identifier len;
> @@
>
> ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> <...
> - len += scnprintf(buf + len, PAGE_SIZE - len,
> + len += sysfs_emit_at(buf, len,
> ...);
> ...>
> return len;
> }
>
> @@
> identifier d_show =~ "^.*show.*$";
> identifier dev, attr, buf;
> expression chr;
> @@
>
> ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> - strcpy(buf, chr);
> - return strlen(buf);
> + return sysfs_emit(buf, chr);
> }
>
> @@
> identifier k_show =~ "^.*show.*$";
> identifier arg1, arg2, arg3;
> @@
> ssize_t k_show(struct kobject *
> - arg1
> + kobj
> , struct kobj_attribute *
> - arg2
> + attr
> , char *
> - arg3
> + buf
> )
> {
> ...
> (
> - arg1
> + kobj
> |
> - arg2
> + attr
> |
> - arg3
> + buf
> )
> ...
> }
>
> @@
> identifier k_show =~ "^.*show.*$";
> identifier kobj, attr, buf;
> @@
>
> ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> <...
> return
> - sprintf(buf,
> + sysfs_emit(buf,
> ...);
> ...>
> }
>
> @@
> identifier k_show =~ "^.*show.*$";
> identifier kobj, attr, buf;
> @@
>
> ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> <...
> return
> - snprintf(buf, PAGE_SIZE,
> + sysfs_emit(buf,
> ...);
> ...>
> }
>
> @@
> identifier k_show =~ "^.*show.*$";
> identifier kobj, attr, buf;
> @@
>
> ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> <...
> return
> - scnprintf(buf, PAGE_SIZE,
> + sysfs_emit(buf,
> ...);
> ...>
> }
>
> @@
> identifier k_show =~ "^.*show.*$";
> identifier kobj, attr, buf;
> expression chr;
> @@
>
> ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> <...
> return
> - strcpy(buf, chr);
> + sysfs_emit(buf, chr);
> ...>
> }
>
> @@
> identifier k_show =~ "^.*show.*$";
> identifier kobj, attr, buf;
> identifier len;
> @@
>
> ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> <...
> len =
> - sprintf(buf,
> + sysfs_emit(buf,
> ...);
> ...>
> return len;
> }
>
> @@
> identifier k_show =~ "^.*show.*$";
> identifier kobj, attr, buf;
> identifier len;
> @@
>
> ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> <...
> len =
> - snprintf(buf, PAGE_SIZE,
> + sysfs_emit(buf,
> ...);
> ...>
> return len;
> }
>
> @@
> identifier k_show =~ "^.*show.*$";
> identifier kobj, attr, buf;
> identifier len;
> @@
>
> ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> <...
> len =
> - scnprintf(buf, PAGE_SIZE,
> + sysfs_emit(buf,
> ...);
> ...>
> return len;
> }
>
> @@
> identifier k_show =~ "^.*show.*$";
> identifier kobj, attr, buf;
> identifier len;
> @@
>
> ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> <...
> - len += scnprintf(buf + len, PAGE_SIZE - len,
> + len += sysfs_emit_at(buf, len,
> ...);
> ...>
> return len;
> }
>
> @@
> identifier k_show =~ "^.*show.*$";
> identifier kobj, attr, buf;
> expression chr;
> @@
>
> ssize_t k_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> - strcpy(buf, chr);
> - return strlen(buf);
> + return sysfs_emit(buf, chr);
> }
>
>
>

2020-08-30 15:27:19

by Denis Efremov (Oracle)

[permalink] [raw]

Subject: Re: [PATCH V2] sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs output

On 8/30/20 3:43 AM, Joe Perches wrote:
> $ cat sysfs_emit.cocci
> @@
> identifier d_show =~ "^.*show.*$";

I think this additional pattern will allow to take more functions into the scope.

@da@
identifier show, store;
expression name, mode;
@@

(
DEVICE_ATTR(name, mode, show, store)
|
DEVICE_ATTR_PREALLOC(name, mode, show, store)
|
DEVICE_ATTR_IGNORE_LOCKDEP(name, mode, show, store)
)

@@
// I think device_show_ulong, device_show_int, device_show_bool
// functions deserve explicit handling because they are somewhat
// reference implementations.
identifier d_show = { da.show, device_show_ulong, device_show_int, device_show_bool };
identifier dev, attr, buf;
@@

* ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
{
...
}

I tried also to handle DEVICE_ATTR_RW, but I failed to use fresh identifier.
This doesn't work:

@darw@
identifier name;
@@

(
DEVICE_ATTR_RW(name)
|
DEVICE_ATTR_RO(name)
|
DEVICE_ATTR_WO(name)
)

@@
identifier darw.name;
fresh identifier d_show = name ## "_show"; // <== parse error
identifier dev, attr, buf;
@@

* ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
{
...
}

Regards,
Denis

2020-08-30 16:06:22

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH V2] sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs output

On Sun, 2020-08-30 at 18:25 +0300, Denis Efremov wrote:
>
> On 8/30/20 3:43 AM, Joe Perches wrote:
> > $ cat sysfs_emit.cocci
> > @@
> > identifier d_show =~ "^.*show.*$";
>
> I think this additional pattern will allow to take more functions into the scope.
>
> @da@
> identifier show, store;
> expression name, mode;
> @@
>
> (
> DEVICE_ATTR(name, mode, show, store)
> DEVICE_ATTR_PREALLOC(name, mode, show, store)
> DEVICE_ATTR_IGNORE_LOCKDEP(name, mode, show, store)
> )

Thanks Denis.

I'll try that out too.

A trivial grep shows there are at least 130+
DEVICE_ATTR functions that have a show function
that doesn't include "show" in the function name.

$ grep-2.5.4 -rP --include=*.[ch] '\bDEVICE_ATTR\s*\(\s*\w+\s*,\s*[^,]+,\s*[^,]*,[^;]+;' * | \
perl -p -e 's/[[:space:]]*//g; s/;/;\n/g' | \
cut -f3 -d, | \
grep -v show | \
sort | uniq | wc -l
139

> @@
> // I think device_show_ulong, device_show_int, device_show_bool
> // functions deserve explicit handling because they are somewhat
> // reference implementations.

Those reference implementations could be send as
a separate patch but this preliminary script does
already handle them.

I do like the idea below of renaming the show
functions without _show in the name adding _show.

> identifier d_show = { da.show, device_show_ulong, device_show_int, device_show_bool };
> identifier dev, attr, buf;
> @@
>
> * ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> ...
> }
>
>
> I tried also to handle DEVICE_ATTR_RW, but I failed to use fresh identifier.
> This doesn't work:
>
> @darw@
> identifier name;
> @@
>
> (
> DEVICE_ATTR_RW(name)
> DEVICE_ATTR_RO(name)
> DEVICE_ATTR_WO(name)
> )
>
> @@
> identifier darw.name;
> fresh identifier d_show = name ## "_show"; // <== parse error
> identifier dev, attr, buf;
> @@
>
> * ssize_t d_show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> ...
> }
>
>
> Regards,
> Denis

2020-09-06 17:28:06

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH V2] sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs output

2020-09-07 05:49:48

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH V2] sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs output

On Sun, Sep 06, 2020 at 10:24:20AM -0700, Joe Perches wrote:
> On Sat, 2020-08-29 at 16:48 -0700, Joe Perches wrote:
> > Output defects can exist in sysfs content using sprintf and snprintf.
> >
> > sprintf does not know the PAGE_SIZE maximum of the temporary buffer
> > used for outputting sysfs content and it's possible to overrun the
> > PAGE_SIZE buffer length.
> >
> > Add a generic sysfs_emit function that knows that the size of the
> > temporary buffer and ensures that no overrun is done.
> >
> > Add a generic sysfs_emit_at function that can be used in multiple
> > call situations that also ensures that no overrun is done.
> >
> > Signed-off-by: Joe Perches <[email protected]>
> > ---
> >
> > V2: Simplify sysfs_emit and add sysfs_emit_at
> > Include Documentation change
>
> Greg? Rafael? Thoughts on this?

I like the idea, give me a chance to catch up on patches, it's still in
my to-review queue...

thanks,

greg k-h

2020-09-07 13:58:24

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH V2] sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs output

On Sat, Aug 29, 2020 at 05:43:58PM -0700, Joe Perches wrote:
> On Sat, 2020-08-29 at 16:48 -0700, Joe Perches wrote:
> > Output defects can exist in sysfs content using sprintf and snprintf.
> >
> > sprintf does not know the PAGE_SIZE maximum of the temporary buffer
> > used for outputting sysfs content and it's possible to overrun the
> > PAGE_SIZE buffer length.
> >
> > Add a generic sysfs_emit function that knows that the size of the
> > temporary buffer and ensures that no overrun is done.
> >
> > Add a generic sysfs_emit_at function that can be used in multiple
> > call situations that also ensures that no overrun is done.
>
> This preliminary coccinelle script converts ~5000 instances treewide.
> There are still many remaining instances that could be converted.
>
> $ git grep -w sysfs_emit -- '*.[ch]'|wc -l
> 4702
> $ git grep -w sysfs_emit_at -- '*.[ch]'|wc -l
> 229

Can you send a patch that would at least convert the driver core code
(drivers/base/*) to use these new helpers so we have an in-tree user
when applying the first patch?

thanks,

greg k-h