[PATCH v1] seq_file: Fix overflow condition in seq_commit

[PATCH v1] seq_file: Fix overflow condition in seq_commit

[Arun KS]

[-- Attachment #1: Type: text/plain, Size: 3802 bytes --]

>From 932a134abeac597f18c86c513704709ad154994b Mon Sep 17 00:00:00 2001
From: Arun KS <arun.ks@broadcom.com>
Date: Mon, 19 Aug 2013 12:06:33 +0530
Subject: seq_file: Fix overflow condition in seq_commit

seq_path()/seq_commit() is treating a d_path() failure as an overflow
condition, but it isn't.

seq_read handles overflow by reallocating more buffer. And this
continues in a loop utill we increase the size of seq buf beyond
KMALLOC_MAX_SIZE and hence a WARN_ON.

[  363.192565] ------------[ cut here ]------------
[  363.199462] WARNING: at mm/slab_common.c:377 kmalloc_slab+0x34/0x9c()
[  363.208557] Modules linked in:
[  363.212219] CPU: 1 PID: 1742 Comm: Binder_2 Tainted: G        W3.10.0+ #17
[  363.222930] [<c00151c4>] (unwind_backtrace+0x0/0x11c)
from[<c0011a24>] (show_stack+0x10/0x14)
[  363.235229] [<c0011a24>] (show_stack+0x10/0x14) from
[<c0059fb0>](warn_slowpath_common+0x4c/0x68)
[  363.247253] [<c0059fb0>] (warn_slowpath_common+0x4c/0x68)
from[<c0059fe4>] (warn_slowpath_null+0x18/0x1c)
[  363.259307] [<c0059fe4>] (warn_slowpath_null+0x18/0x1c)
from[<c00fa400>] (kmalloc_slab+0x34/0x9c)
[  363.270812] [<c00fa400>] (kmalloc_slab+0x34/0x9c) from
[<c010ec20>](__kmalloc+0x14/0x1fc)
[  363.281433] [<c010ec20>] (__kmalloc+0x14/0x1fc) from
[<c012ef70>](seq_read+0x24c/0x438)
[  363.291992] [<c012ef70>] (seq_read+0x24c/0x438) from
[<c011389c>](vfs_read+0xac/0x124)
[  363.302398] [<c011389c>] (vfs_read+0xac/0x124) from
[<c0113a38>](SyS_read+0x3c/0x60)
[  363.312591] [<c0113a38>] (SyS_read+0x3c/0x60) from
[<c000e180>](ret_fast_syscall+0x0/0x48)
[  363.323303] ---[ end trace 46c6467e2db7bcd4 ]---

Pass -ENOBUFS to seq_commit to signal an overflow condition and a
negative value for all other errors.

Signed-off-by: Arun KS <arun.ks@broadcom.com>
---
 fs/seq_file.c            |  6 +++---
 include/linux/seq_file.h | 12 +++++++-----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3135c25..6a33f9c 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -463,7 +463,7 @@ int seq_path(struct seq_file *m, const struct path
*path, const char *esc)
 {
  char *buf;
  size_t size = seq_get_buf(m, &buf);
- int res = -1;
+ int res = -ENOBUFS;

  if (size) {
  char *p = d_path(path, buf, size);
@@ -487,7 +487,7 @@ int seq_path_root(struct seq_file *m, const struct
path *path,
 {
  char *buf;
  size_t size = seq_get_buf(m, &buf);
- int res = -ENAMETOOLONG;
+ int res = -ENOBUFS;

  if (size) {
  char *p;
@@ -516,7 +516,7 @@ int seq_dentry(struct seq_file *m, struct dentry
*dentry, const char *esc)
 {
  char *buf;
  size_t size = seq_get_buf(m, &buf);
- int res = -1;
+ int res = -ENOBUFS;

  if (size) {
  char *p = dentry_path(dentry, buf, size);
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 4e32edc..43f51a0 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -7,6 +7,7 @@
 #include <linux/mutex.h>
 #include <linux/cpumask.h>
 #include <linux/nodemask.h>
+#include <linux/errno.h>

 struct seq_operations;
 struct file;
@@ -66,16 +67,17 @@ static inline size_t seq_get_buf(struct seq_file
*m, char **bufp)
  * @num: the number of bytes to commit
  *
  * Commit @num bytes of data written to a buffer previously acquired
- * by seq_buf_get.  To signal an error condition, or that the data
- * didn't fit in the available space, pass a negative @num value.
+ * by seq_buf_get.  To signal an overflow condition(data didn't fit
+ * in the available space), pass -ENOBUFS and for other errors pass a
+ * negative @num value.
  */
 static inline void seq_commit(struct seq_file *m, int num)
 {
- if (num < 0) {
- m->count = m->size;
- } else {
+ if (num >= 0) {
  BUG_ON(m->count + num > m->size);
  m->count += num;
+ } else if (num == -ENOBUFS)
+ m->count = m->size;
  }
 }

-- 
1.8.2

[-- Attachment #2: 0001-seq_file-Fix-overflow-condition-in-seq_commit.patch --]
[-- Type: application/octet-stream, Size: 3814 bytes --]

From 932a134abeac597f18c86c513704709ad154994b Mon Sep 17 00:00:00 2001
From: Arun KS <arun.ks@broadcom.com>
Date: Mon, 19 Aug 2013 12:06:33 +0530
Subject: seq_file: Fix overflow condition in seq_commit

seq_path()/seq_commit() is treating a d_path() failure as an overflow
condition, but it isn't.

seq_read handles overflow by reallocating more buffer. And this
continues in a loop utill we increase the size of seq buf beyond
KMALLOC_MAX_SIZE and hence a WARN_ON.

[  363.192565] ------------[ cut here ]------------
[  363.199462] WARNING: at mm/slab_common.c:377 kmalloc_slab+0x34/0x9c()
[  363.208557] Modules linked in:
[  363.212219] CPU: 1 PID: 1742 Comm: Binder_2 Tainted: G        W3.10.0+ #17
[  363.222930] [<c00151c4>] (unwind_backtrace+0x0/0x11c) from[<c0011a24>] (show_stack+0x10/0x14)
[  363.235229] [<c0011a24>] (show_stack+0x10/0x14) from [<c0059fb0>](warn_slowpath_common+0x4c/0x68)
[  363.247253] [<c0059fb0>] (warn_slowpath_common+0x4c/0x68) from[<c0059fe4>] (warn_slowpath_null+0x18/0x1c)
[  363.259307] [<c0059fe4>] (warn_slowpath_null+0x18/0x1c) from[<c00fa400>] (kmalloc_slab+0x34/0x9c)
[  363.270812] [<c00fa400>] (kmalloc_slab+0x34/0x9c) from [<c010ec20>](__kmalloc+0x14/0x1fc)
[  363.281433] [<c010ec20>] (__kmalloc+0x14/0x1fc) from [<c012ef70>](seq_read+0x24c/0x438)
[  363.291992] [<c012ef70>] (seq_read+0x24c/0x438) from [<c011389c>](vfs_read+0xac/0x124)
[  363.302398] [<c011389c>] (vfs_read+0xac/0x124) from [<c0113a38>](SyS_read+0x3c/0x60)
[  363.312591] [<c0113a38>] (SyS_read+0x3c/0x60) from [<c000e180>](ret_fast_syscall+0x0/0x48)
[  363.323303] ---[ end trace 46c6467e2db7bcd4 ]---

Pass -ENOBUFS to seq_commit to signal an overflow condition and a
negative value for all other errors.

Signed-off-by: Arun KS <arun.ks@broadcom.com>
---
 fs/seq_file.c            |  6 +++---
 include/linux/seq_file.h | 12 +++++++-----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3135c25..6a33f9c 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -463,7 +463,7 @@ int seq_path(struct seq_file *m, const struct path *path, const char *esc)
 {
 	char *buf;
 	size_t size = seq_get_buf(m, &buf);
-	int res = -1;
+	int res = -ENOBUFS;
 
 	if (size) {
 		char *p = d_path(path, buf, size);
@@ -487,7 +487,7 @@ int seq_path_root(struct seq_file *m, const struct path *path,
 {
 	char *buf;
 	size_t size = seq_get_buf(m, &buf);
-	int res = -ENAMETOOLONG;
+	int res = -ENOBUFS;
 
 	if (size) {
 		char *p;
@@ -516,7 +516,7 @@ int seq_dentry(struct seq_file *m, struct dentry *dentry, const char *esc)
 {
 	char *buf;
 	size_t size = seq_get_buf(m, &buf);
-	int res = -1;
+	int res = -ENOBUFS;
 
 	if (size) {
 		char *p = dentry_path(dentry, buf, size);
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 4e32edc..43f51a0 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -7,6 +7,7 @@
 #include <linux/mutex.h>
 #include <linux/cpumask.h>
 #include <linux/nodemask.h>
+#include <linux/errno.h>
 
 struct seq_operations;
 struct file;
@@ -66,16 +67,17 @@ static inline size_t seq_get_buf(struct seq_file *m, char **bufp)
  * @num: the number of bytes to commit
  *
  * Commit @num bytes of data written to a buffer previously acquired
- * by seq_buf_get.  To signal an error condition, or that the data
- * didn't fit in the available space, pass a negative @num value.
+ * by seq_buf_get.  To signal an overflow condition(data didn't fit
+ * in the available space), pass -ENOBUFS and for other errors pass a
+ * negative @num value.
  */
 static inline void seq_commit(struct seq_file *m, int num)
 {
-	if (num < 0) {
-		m->count = m->size;
-	} else {
+	if (num >= 0) {
 		BUG_ON(m->count + num > m->size);
 		m->count += num;
+	} else if (num == -ENOBUFS)
+		m->count = m->size;
 	}
 }
 
-- 
1.8.2

Re: [PATCH v1] seq_file: Fix overflow condition in seq_commit

[Al Viro]

On Tue, Aug 20, 2013 at 10:55:40AM +0530, Arun KS wrote:
> >From 932a134abeac597f18c86c513704709ad154994b Mon Sep 17 00:00:00 2001
> From: Arun KS <arun.ks@broadcom.com>
> Date: Mon, 19 Aug 2013 12:06:33 +0530
> Subject: seq_file: Fix overflow condition in seq_commit
> 
> seq_path()/seq_commit() is treating a d_path() failure as an overflow
> condition, but it isn't.
> 
> seq_read handles overflow by reallocating more buffer. And this
> continues in a loop utill we increase the size of seq buf beyond
> KMALLOC_MAX_SIZE and hence a WARN_ON.
> 
> [  363.192565] ------------[ cut here ]------------
> [  363.199462] WARNING: at mm/slab_common.c:377 kmalloc_slab+0x34/0x9c()
> [  363.208557] Modules linked in:
> [  363.212219] CPU: 1 PID: 1742 Comm: Binder_2 Tainted: G        W3.10.0+ #17
> [  363.222930] [<c00151c4>] (unwind_backtrace+0x0/0x11c)
> from[<c0011a24>] (show_stack+0x10/0x14)
> [  363.235229] [<c0011a24>] (show_stack+0x10/0x14) from
> [<c0059fb0>](warn_slowpath_common+0x4c/0x68)
> [  363.247253] [<c0059fb0>] (warn_slowpath_common+0x4c/0x68)
> from[<c0059fe4>] (warn_slowpath_null+0x18/0x1c)
> [  363.259307] [<c0059fe4>] (warn_slowpath_null+0x18/0x1c)
> from[<c00fa400>] (kmalloc_slab+0x34/0x9c)
> [  363.270812] [<c00fa400>] (kmalloc_slab+0x34/0x9c) from
> [<c010ec20>](__kmalloc+0x14/0x1fc)
> [  363.281433] [<c010ec20>] (__kmalloc+0x14/0x1fc) from
> [<c012ef70>](seq_read+0x24c/0x438)
> [  363.291992] [<c012ef70>] (seq_read+0x24c/0x438) from
> [<c011389c>](vfs_read+0xac/0x124)
> [  363.302398] [<c011389c>] (vfs_read+0xac/0x124) from
> [<c0113a38>](SyS_read+0x3c/0x60)
> [  363.312591] [<c0113a38>] (SyS_read+0x3c/0x60) from
> [<c000e180>](ret_fast_syscall+0x0/0x48)
> [  363.323303] ---[ end trace 46c6467e2db7bcd4 ]---
> 
> Pass -ENOBUFS to seq_commit to signal an overflow condition and a
> negative value for all other errors.

Excuse me, _what_ other errors?  Please, explain what errors can
be returned by d_path/__d_path/dentry_path.  Normally all of those
return ERR_PTR(-ENAMETOOLONG) if the pathname to be generated would
not fit into a buffer.  In which case the only sane behaviour is
to use a bigger buffer.

Could you please post the reproducer for that trace of yours?
I might be missing something obvious, of course, but AFAICS you
are just papering over a bug somewhere else.  If you really
manage to get d_path() with output that wouldn't fit into 128Mb,
you have a whole lot of unpleasant problems beyond a warning in
seq_read().  And silent truncation of pathnames that don't happen
to fit into what's left of the default buffer is simply wrong -
4095-byte pathnames are perfectly fine.

Re: [PATCH v1] seq_file: Fix overflow condition in seq_commit

[Arun KS]

Hi Al Viro,

On Tue, Aug 20, 2013 at 11:21 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Aug 20, 2013 at 10:55:40AM +0530, Arun KS wrote:
>> >From 932a134abeac597f18c86c513704709ad154994b Mon Sep 17 00:00:00 2001
>> From: Arun KS <arun.ks@broadcom.com>
>> Date: Mon, 19 Aug 2013 12:06:33 +0530
>> Subject: seq_file: Fix overflow condition in seq_commit
>>
>> seq_path()/seq_commit() is treating a d_path() failure as an overflow
>> condition, but it isn't.
>>
>> seq_read handles overflow by reallocating more buffer. And this
>> continues in a loop utill we increase the size of seq buf beyond
>> KMALLOC_MAX_SIZE and hence a WARN_ON.
>>
>> [  363.192565] ------------[ cut here ]------------
>> [  363.199462] WARNING: at mm/slab_common.c:377 kmalloc_slab+0x34/0x9c()
>> [  363.208557] Modules linked in:
>> [  363.212219] CPU: 1 PID: 1742 Comm: Binder_2 Tainted: G        W3.10.0+ #17
>> [  363.222930] [<c00151c4>] (unwind_backtrace+0x0/0x11c)
>> from[<c0011a24>] (show_stack+0x10/0x14)
>> [  363.235229] [<c0011a24>] (show_stack+0x10/0x14) from
>> [<c0059fb0>](warn_slowpath_common+0x4c/0x68)
>> [  363.247253] [<c0059fb0>] (warn_slowpath_common+0x4c/0x68)
>> from[<c0059fe4>] (warn_slowpath_null+0x18/0x1c)
>> [  363.259307] [<c0059fe4>] (warn_slowpath_null+0x18/0x1c)
>> from[<c00fa400>] (kmalloc_slab+0x34/0x9c)
>> [  363.270812] [<c00fa400>] (kmalloc_slab+0x34/0x9c) from
>> [<c010ec20>](__kmalloc+0x14/0x1fc)
>> [  363.281433] [<c010ec20>] (__kmalloc+0x14/0x1fc) from
>> [<c012ef70>](seq_read+0x24c/0x438)
>> [  363.291992] [<c012ef70>] (seq_read+0x24c/0x438) from
>> [<c011389c>](vfs_read+0xac/0x124)
>> [  363.302398] [<c011389c>] (vfs_read+0xac/0x124) from
>> [<c0113a38>](SyS_read+0x3c/0x60)
>> [  363.312591] [<c0113a38>] (SyS_read+0x3c/0x60) from
>> [<c000e180>](ret_fast_syscall+0x0/0x48)
>> [  363.323303] ---[ end trace 46c6467e2db7bcd4 ]---
>>
>> Pass -ENOBUFS to seq_commit to signal an overflow condition and a
>> negative value for all other errors.
>
> Excuse me, _what_ other errors?  Please, explain what errors can
> be returned by d_path/__d_path/dentry_path.  Normally all of those
> return ERR_PTR(-ENAMETOOLONG) if the pathname to be generated would
> not fit into a buffer.  In which case the only sane behaviour is
> to use a bigger buffer.

d_path expects the pathname to be less than 64 bytes. If its more than
64, it returns -ENAMETOOLONG.

File:fs/dcache.c

char *dynamic_dname(struct dentry *dentry, char *buffer, int buflen,
                        const char *fmt, ...)
{
        va_list args;
        char temp[64];
        int sz;

        va_start(args, fmt);
        sz = vsnprintf(temp, sizeof(temp), fmt, args) + 1;
        va_end(args);

        if (sz > sizeof(temp) || sz > buflen)
                return ERR_PTR(-ENAMETOOLONG);

        buffer += buflen - sz;
        return memcpy(buffer, temp, sz);
}

The string name which give error is,
"dev/ashmem/CursorWindow:
/data/data/com.android.providers.settings/databases/settings.db"

when sz>sizeof(temp), dynamic_dname returns -ENAMETOOLONG.
In this instance, sz is coming as 100.

This always happens from shared mem, after shmem_dname function was
added by the following commit.

commit 3451538a114d738a6528b1da58e19e7d8964c647
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Thu Feb 14 22:38:02 2013 -0500

    shmem_setup_file(): use d_alloc_pseudo() instead of d_alloc()

    Note that provided ->d_dname() reproduces what we used to get for
    those guys in e.g. /proc/self/maps; it might be a good idea to change
    that to something less ugly, but for now let's keep the existing
    user-visible behaviour

    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>


Thanks,
Arun


>
> Could you please post the reproducer for that trace of yours?
> I might be missing something obvious, of course, but AFAICS you
> are just papering over a bug somewhere else.  If you really
> manage to get d_path() with output that wouldn't fit into 128Mb,
> you have a whole lot of unpleasant problems beyond a warning in
> seq_read().  And silent truncation of pathnames that don't happen
> to fit into what's left of the default buffer is simply wrong -
> 4095-byte pathnames are perfectly fine.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] seq_file: Fix overflow condition in seq_commit

[Al Viro]

On Tue, Aug 20, 2013 at 12:51:56PM +0530, Arun KS wrote:

> d_path expects the pathname to be less than 64 bytes. If its more than
> 64, it returns -ENAMETOOLONG.

?!?!?  d_path() does not expect anything of that sort - it can very
well produce names much longer, without ENAMETOOLONG.

> char *dynamic_dname(struct dentry *dentry, char *buffer, int buflen,
>                         const char *fmt, ...)
> {
>         va_list args;
>         char temp[64];
>         int sz;
> 
>         va_start(args, fmt);
>         sz = vsnprintf(temp, sizeof(temp), fmt, args) + 1;
>         va_end(args);
> 
>         if (sz > sizeof(temp) || sz > buflen)
>                 return ERR_PTR(-ENAMETOOLONG);
> 
>         buffer += buflen - sz;
>         return memcpy(buffer, temp, sz);
> }

Aha.  _That_ is a bug, all right - dynamic_dname() is simply not suitable
for that kind of uses.  ashmem.c is certainly abusing shmem_file_setup();
feeding that kind of mess as dentry name is a Bad Idea(tm).  Anyway,
I'd suggest this for a fix:

	va_list args;
	size_t sz;
	va_start(args, fmt);
	sz = vsnprintf(NULL, 0, fmt, args) + 1;
	va_end(args);
	if (sz > buflen)
		return ERR_PTR(-ENAMETOOLONG);
	va_start(args, fmt);
	buffer += buflen - sz;
	vsprintf(buffer, fmt, args);
	va_end(args);
	return buffer;

Either right in dynamic_dname(), or as possibly_fucking_long_dname(),
to be used by shmem.c...

Re: [PATCH v1] seq_file: Fix overflow condition in seq_commit

[Arun KS]

Hi Al Viro,

On Tue, Aug 20, 2013 at 1:06 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Aug 20, 2013 at 12:51:56PM +0530, Arun KS wrote:
>
>> d_path expects the pathname to be less than 64 bytes. If its more than
>> 64, it returns -ENAMETOOLONG.
>
> ?!?!?  d_path() does not expect anything of that sort - it can very
> well produce names much longer, without ENAMETOOLONG.
>
>> char *dynamic_dname(struct dentry *dentry, char *buffer, int buflen,
>>                         const char *fmt, ...)
>> {
>>         va_list args;
>>         char temp[64];
>>         int sz;
>>
>>         va_start(args, fmt);
>>         sz = vsnprintf(temp, sizeof(temp), fmt, args) + 1;
>>         va_end(args);
>>
>>         if (sz > sizeof(temp) || sz > buflen)
>>                 return ERR_PTR(-ENAMETOOLONG);
>>
>>         buffer += buflen - sz;
>>         return memcpy(buffer, temp, sz);
>> }
>
> Aha.  _That_ is a bug, all right - dynamic_dname() is simply not suitable
> for that kind of uses.  ashmem.c is certainly abusing shmem_file_setup();
> feeding that kind of mess as dentry name is a Bad Idea(tm).  Anyway,
> I'd suggest this for a fix:
>
>         va_list args;
>         size_t sz;
>         va_start(args, fmt);
>         sz = vsnprintf(NULL, 0, fmt, args) + 1;
>         va_end(args);
>         if (sz > buflen)
>                 return ERR_PTR(-ENAMETOOLONG);
>         va_start(args, fmt);
>         buffer += buflen - sz;
>         vsprintf(buffer, fmt, args);
>         va_end(args);
>         return buffer;
>
> Either right in dynamic_dname(), or as possibly_fucking_long_dname(),
> to be used by shmem.c...

This fixes the problem.

Thanks,
Arun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] seq_file: Fix overflow condition in seq_commit

[Al Viro]

On Tue, Aug 20, 2013 at 08:36:22AM +0100, Al Viro wrote:

> Aha.  _That_ is a bug, all right - dynamic_dname() is simply not suitable
> for that kind of uses.  ashmem.c is certainly abusing shmem_file_setup();
> feeding that kind of mess as dentry name is a Bad Idea(tm).  Anyway,
> I'd suggest this for a fix:
> 
> 	va_list args;
> 	size_t sz;
> 	va_start(args, fmt);
> 	sz = vsnprintf(NULL, 0, fmt, args) + 1;
> 	va_end(args);
> 	if (sz > buflen)
> 		return ERR_PTR(-ENAMETOOLONG);
> 	va_start(args, fmt);
> 	buffer += buflen - sz;
> 	vsprintf(buffer, fmt, args);
> 	va_end(args);
> 	return buffer;
> 
> Either right in dynamic_dname(), or as possibly_fucking_long_dname(),
> to be used by shmem.c...

Actually, looking at the users of dynamic_dname()...  Most of them are
fine with dynamic_dname() as it is, with 2 possible exceptions:
hugetlb_dname() and shmem_dname().  Both are using "/%s (deleted)",
dentry->d_name.name as format...  So the solution above may very
well be an overkill; something like

char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
{
	char *end = buffer + buflen;
	/* these dentries are never renamed, so d_lock is not needed */
	if (prepend(&end, &buflen, " (deleted)", 11) ||
	    prepend_name(&end, &buflen, &dentry->d_name) ||
	    prepend(&end, &buflen, "/", 1))
		end = ERR_PTR(-ENAMETOOLONG);
	return end;
}

will do for those two.  Care to test the diff below?

cope with potentially long ->d_dname() output for shmem/hugetlb

dynamic_dname() is both too much and too little for those - the
output may be well in excess of 64 bytes dynamic_dname() assumes
to be enough (thanks to ashmem feeding really long names to
shmem_file_setup()) and vsnprintf() is an overkill for those
guys.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/dcache.c b/fs/dcache.c
index 87bdb53..83cfb83 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2724,6 +2724,17 @@ char *dynamic_dname(struct dentry *dentry, char *buffer, int buflen,
 	return memcpy(buffer, temp, sz);
 }
 
+char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
+{
+	char *end = buffer + buflen;
+	/* these dentries are never renamed, so d_lock is not needed */
+	if (prepend(&end, &buflen, " (deleted)", 11) ||
+	    prepend_name(&end, &buflen, &dentry->d_name) ||
+	    prepend(&end, &buflen, "/", 1))  
+		end = ERR_PTR(-ENAMETOOLONG);
+	return end;  
+}
+
 /*
  * Write full pathname from the root of the filesystem into the buffer.
  */
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index a3f868a..4e5f332 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -916,14 +916,8 @@ static int get_hstate_idx(int page_size_log)
 	return h - hstates;
 }
 
-static char *hugetlb_dname(struct dentry *dentry, char *buffer, int buflen)
-{
-	return dynamic_dname(dentry, buffer, buflen, "/%s (deleted)",
-				dentry->d_name.name);
-}
-
 static struct dentry_operations anon_ops = {
-	.d_dname = hugetlb_dname
+	.d_dname = simple_dname
 };
 
 /*
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index b90337c..4a12532 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -336,6 +336,7 @@ extern int d_validate(struct dentry *, struct dentry *);
  * helper function for dentry_operations.d_dname() members
  */
 extern char *dynamic_dname(struct dentry *, char *, int, const char *, ...);
+extern char *simple_dname(struct dentry *, char *, int);
 
 extern char *__d_path(const struct path *, const struct path *, char *, int);
 extern char *d_absolute_path(const struct path *, char *, int);
diff --git a/mm/shmem.c b/mm/shmem.c
index 8335dbd..e43dc55 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2909,14 +2909,8 @@ EXPORT_SYMBOL_GPL(shmem_truncate_range);
 
 /* common code */
 
-static char *shmem_dname(struct dentry *dentry, char *buffer, int buflen)
-{
-	return dynamic_dname(dentry, buffer, buflen, "/%s (deleted)",
-				dentry->d_name.name);
-}
-
 static struct dentry_operations anon_ops = {
-	.d_dname = shmem_dname
+	.d_dname = simple_dname
 };
 
 /**

Re: [PATCH v1] seq_file: Fix overflow condition in seq_commit

[Arun KS]

Hi Al Viro,

On Tue, Aug 20, 2013 at 7:34 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Aug 20, 2013 at 08:36:22AM +0100, Al Viro wrote:
>
>> Aha.  _That_ is a bug, all right - dynamic_dname() is simply not suitable
>> for that kind of uses.  ashmem.c is certainly abusing shmem_file_setup();
>> feeding that kind of mess as dentry name is a Bad Idea(tm).  Anyway,
>> I'd suggest this for a fix:
>>
>>       va_list args;
>>       size_t sz;
>>       va_start(args, fmt);
>>       sz = vsnprintf(NULL, 0, fmt, args) + 1;
>>       va_end(args);
>>       if (sz > buflen)
>>               return ERR_PTR(-ENAMETOOLONG);
>>       va_start(args, fmt);
>>       buffer += buflen - sz;
>>       vsprintf(buffer, fmt, args);
>>       va_end(args);
>>       return buffer;
>>
>> Either right in dynamic_dname(), or as possibly_fucking_long_dname(),
>> to be used by shmem.c...
>
> Actually, looking at the users of dynamic_dname()...  Most of them are
> fine with dynamic_dname() as it is, with 2 possible exceptions:
> hugetlb_dname() and shmem_dname().  Both are using "/%s (deleted)",
> dentry->d_name.name as format...  So the solution above may very
> well be an overkill; something like
>
> char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
> {
>         char *end = buffer + buflen;
>         /* these dentries are never renamed, so d_lock is not needed */
>         if (prepend(&end, &buflen, " (deleted)", 11) ||
>             prepend_name(&end, &buflen, &dentry->d_name) ||
>             prepend(&end, &buflen, "/", 1))
>                 end = ERR_PTR(-ENAMETOOLONG);
>         return end;
> }
>
> will do for those two.  Care to test the diff below?
Patch works fine.

Tested-by: Arun KS<arunks.linux@gmail.com>

Thanks,
Arun
>
> cope with potentially long ->d_dname() output for shmem/hugetlb
>
> dynamic_dname() is both too much and too little for those - the
> output may be well in excess of 64 bytes dynamic_dname() assumes
> to be enough (thanks to ashmem feeding really long names to
> shmem_file_setup()) and vsnprintf() is an overkill for those
> guys.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 87bdb53..83cfb83 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2724,6 +2724,17 @@ char *dynamic_dname(struct dentry *dentry, char *buffer, int buflen,
>         return memcpy(buffer, temp, sz);
>  }
>
> +char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
> +{
> +       char *end = buffer + buflen;
> +       /* these dentries are never renamed, so d_lock is not needed */
> +       if (prepend(&end, &buflen, " (deleted)", 11) ||
> +           prepend_name(&end, &buflen, &dentry->d_name) ||
> +           prepend(&end, &buflen, "/", 1))
> +               end = ERR_PTR(-ENAMETOOLONG);
> +       return end;
> +}
> +
>  /*
>   * Write full pathname from the root of the filesystem into the buffer.
>   */
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index a3f868a..4e5f332 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -916,14 +916,8 @@ static int get_hstate_idx(int page_size_log)
>         return h - hstates;
>  }
>
> -static char *hugetlb_dname(struct dentry *dentry, char *buffer, int buflen)
> -{
> -       return dynamic_dname(dentry, buffer, buflen, "/%s (deleted)",
> -                               dentry->d_name.name);
> -}
> -
>  static struct dentry_operations anon_ops = {
> -       .d_dname = hugetlb_dname
> +       .d_dname = simple_dname
>  };
>
>  /*
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index b90337c..4a12532 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -336,6 +336,7 @@ extern int d_validate(struct dentry *, struct dentry *);
>   * helper function for dentry_operations.d_dname() members
>   */
>  extern char *dynamic_dname(struct dentry *, char *, int, const char *, ...);
> +extern char *simple_dname(struct dentry *, char *, int);
>
>  extern char *__d_path(const struct path *, const struct path *, char *, int);
>  extern char *d_absolute_path(const struct path *, char *, int);
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 8335dbd..e43dc55 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2909,14 +2909,8 @@ EXPORT_SYMBOL_GPL(shmem_truncate_range);
>
>  /* common code */
>
> -static char *shmem_dname(struct dentry *dentry, char *buffer, int buflen)
> -{
> -       return dynamic_dname(dentry, buffer, buflen, "/%s (deleted)",
> -                               dentry->d_name.name);
> -}
> -
>  static struct dentry_operations anon_ops = {
> -       .d_dname = shmem_dname
> +       .d_dname = simple_dname
>  };
>
>  /**
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1] seq_file: Fix overflow condition in seq_commit

[Colin Cross]

On Tue, Aug 20, 2013 at 7:04 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Aug 20, 2013 at 08:36:22AM +0100, Al Viro wrote:
>
>> Aha.  _That_ is a bug, all right - dynamic_dname() is simply not suitable
>> for that kind of uses.  ashmem.c is certainly abusing shmem_file_setup();
>> feeding that kind of mess as dentry name is a Bad Idea(tm).  Anyway,
>> I'd suggest this for a fix:
>>
>>       va_list args;
>>       size_t sz;
>>       va_start(args, fmt);
>>       sz = vsnprintf(NULL, 0, fmt, args) + 1;
>>       va_end(args);
>>       if (sz > buflen)
>>               return ERR_PTR(-ENAMETOOLONG);
>>       va_start(args, fmt);
>>       buffer += buflen - sz;
>>       vsprintf(buffer, fmt, args);
>>       va_end(args);
>>       return buffer;
>>
>> Either right in dynamic_dname(), or as possibly_fucking_long_dname(),
>> to be used by shmem.c...
>
> Actually, looking at the users of dynamic_dname()...  Most of them are
> fine with dynamic_dname() as it is, with 2 possible exceptions:
> hugetlb_dname() and shmem_dname().  Both are using "/%s (deleted)",
> dentry->d_name.name as format...  So the solution above may very
> well be an overkill; something like
>
> char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
> {
>         char *end = buffer + buflen;
>         /* these dentries are never renamed, so d_lock is not needed */
>         if (prepend(&end, &buflen, " (deleted)", 11) ||
>             prepend_name(&end, &buflen, &dentry->d_name) ||
>             prepend(&end, &buflen, "/", 1))
>                 end = ERR_PTR(-ENAMETOOLONG);
>         return end;
> }
>
> will do for those two.  Care to test the diff below?
>
> cope with potentially long ->d_dname() output for shmem/hugetlb
>
> dynamic_dname() is both too much and too little for those - the
> output may be well in excess of 64 bytes dynamic_dname() assumes
> to be enough (thanks to ashmem feeding really long names to
> shmem_file_setup()) and vsnprintf() is an overkill for those
> guys.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 87bdb53..83cfb83 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2724,6 +2724,17 @@ char *dynamic_dname(struct dentry *dentry, char *buffer, int buflen,
>         return memcpy(buffer, temp, sz);
>  }
>
> +char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
> +{
> +       char *end = buffer + buflen;
> +       /* these dentries are never renamed, so d_lock is not needed */
> +       if (prepend(&end, &buflen, " (deleted)", 11) ||
> +           prepend_name(&end, &buflen, &dentry->d_name) ||
> +           prepend(&end, &buflen, "/", 1))
> +               end = ERR_PTR(-ENAMETOOLONG);
> +       return end;
> +}
> +
>  /*
>   * Write full pathname from the root of the filesystem into the buffer.
>   */
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index a3f868a..4e5f332 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -916,14 +916,8 @@ static int get_hstate_idx(int page_size_log)
>         return h - hstates;
>  }
>
> -static char *hugetlb_dname(struct dentry *dentry, char *buffer, int buflen)
> -{
> -       return dynamic_dname(dentry, buffer, buflen, "/%s (deleted)",
> -                               dentry->d_name.name);
> -}
> -
>  static struct dentry_operations anon_ops = {
> -       .d_dname = hugetlb_dname
> +       .d_dname = simple_dname
>  };
>
>  /*
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index b90337c..4a12532 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -336,6 +336,7 @@ extern int d_validate(struct dentry *, struct dentry *);
>   * helper function for dentry_operations.d_dname() members
>   */
>  extern char *dynamic_dname(struct dentry *, char *, int, const char *, ...);
> +extern char *simple_dname(struct dentry *, char *, int);
>
>  extern char *__d_path(const struct path *, const struct path *, char *, int);
>  extern char *d_absolute_path(const struct path *, char *, int);
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 8335dbd..e43dc55 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2909,14 +2909,8 @@ EXPORT_SYMBOL_GPL(shmem_truncate_range);
>
>  /* common code */
>
> -static char *shmem_dname(struct dentry *dentry, char *buffer, int buflen)
> -{
> -       return dynamic_dname(dentry, buffer, buflen, "/%s (deleted)",
> -                               dentry->d_name.name);
> -}
> -
>  static struct dentry_operations anon_ops = {
> -       .d_dname = shmem_dname
> +       .d_dname = simple_dname
>  };
>
>  /**
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Can this patch (committed as 118b23022512eb2f41ce42db70dc0568d00be4ba
upstream) go in stable?  It fixes a regression introduced in 3.9
dumping /proc/pid/maps for processes with ashmem regions with long
names.

Re: [PATCH v1] seq_file: Fix overflow condition in seq_commit

[Greg KH]

On Mon, Oct 14, 2013 at 03:57:04PM -0700, Colin Cross wrote:
> On Tue, Aug 20, 2013 at 7:04 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Tue, Aug 20, 2013 at 08:36:22AM +0100, Al Viro wrote:
> >
> >> Aha.  _That_ is a bug, all right - dynamic_dname() is simply not suitable
> >> for that kind of uses.  ashmem.c is certainly abusing shmem_file_setup();
> >> feeding that kind of mess as dentry name is a Bad Idea(tm).  Anyway,
> >> I'd suggest this for a fix:
> >>
> >>       va_list args;
> >>       size_t sz;
> >>       va_start(args, fmt);
> >>       sz = vsnprintf(NULL, 0, fmt, args) + 1;
> >>       va_end(args);
> >>       if (sz > buflen)
> >>               return ERR_PTR(-ENAMETOOLONG);
> >>       va_start(args, fmt);
> >>       buffer += buflen - sz;
> >>       vsprintf(buffer, fmt, args);
> >>       va_end(args);
> >>       return buffer;
> >>
> >> Either right in dynamic_dname(), or as possibly_fucking_long_dname(),
> >> to be used by shmem.c...
> >
> > Actually, looking at the users of dynamic_dname()...  Most of them are
> > fine with dynamic_dname() as it is, with 2 possible exceptions:
> > hugetlb_dname() and shmem_dname().  Both are using "/%s (deleted)",
> > dentry->d_name.name as format...  So the solution above may very
> > well be an overkill; something like
> >
> > char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
> > {
> >         char *end = buffer + buflen;
> >         /* these dentries are never renamed, so d_lock is not needed */
> >         if (prepend(&end, &buflen, " (deleted)", 11) ||
> >             prepend_name(&end, &buflen, &dentry->d_name) ||
> >             prepend(&end, &buflen, "/", 1))
> >                 end = ERR_PTR(-ENAMETOOLONG);
> >         return end;
> > }
> >
> > will do for those two.  Care to test the diff below?
> >
> > cope with potentially long ->d_dname() output for shmem/hugetlb
> >
> > dynamic_dname() is both too much and too little for those - the
> > output may be well in excess of 64 bytes dynamic_dname() assumes
> > to be enough (thanks to ashmem feeding really long names to
> > shmem_file_setup()) and vsnprintf() is an overkill for those
> > guys.
> >
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index 87bdb53..83cfb83 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -2724,6 +2724,17 @@ char *dynamic_dname(struct dentry *dentry, char *buffer, int buflen,
> >         return memcpy(buffer, temp, sz);
> >  }
> >
> > +char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
> > +{
> > +       char *end = buffer + buflen;
> > +       /* these dentries are never renamed, so d_lock is not needed */
> > +       if (prepend(&end, &buflen, " (deleted)", 11) ||
> > +           prepend_name(&end, &buflen, &dentry->d_name) ||
> > +           prepend(&end, &buflen, "/", 1))
> > +               end = ERR_PTR(-ENAMETOOLONG);
> > +       return end;
> > +}
> > +
> >  /*
> >   * Write full pathname from the root of the filesystem into the buffer.
> >   */
> > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > index a3f868a..4e5f332 100644
> > --- a/fs/hugetlbfs/inode.c
> > +++ b/fs/hugetlbfs/inode.c
> > @@ -916,14 +916,8 @@ static int get_hstate_idx(int page_size_log)
> >         return h - hstates;
> >  }
> >
> > -static char *hugetlb_dname(struct dentry *dentry, char *buffer, int buflen)
> > -{
> > -       return dynamic_dname(dentry, buffer, buflen, "/%s (deleted)",
> > -                               dentry->d_name.name);
> > -}
> > -
> >  static struct dentry_operations anon_ops = {
> > -       .d_dname = hugetlb_dname
> > +       .d_dname = simple_dname
> >  };
> >
> >  /*
> > diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> > index b90337c..4a12532 100644
> > --- a/include/linux/dcache.h
> > +++ b/include/linux/dcache.h
> > @@ -336,6 +336,7 @@ extern int d_validate(struct dentry *, struct dentry *);
> >   * helper function for dentry_operations.d_dname() members
> >   */
> >  extern char *dynamic_dname(struct dentry *, char *, int, const char *, ...);
> > +extern char *simple_dname(struct dentry *, char *, int);
> >
> >  extern char *__d_path(const struct path *, const struct path *, char *, int);
> >  extern char *d_absolute_path(const struct path *, char *, int);
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 8335dbd..e43dc55 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -2909,14 +2909,8 @@ EXPORT_SYMBOL_GPL(shmem_truncate_range);
> >
> >  /* common code */
> >
> > -static char *shmem_dname(struct dentry *dentry, char *buffer, int buflen)
> > -{
> > -       return dynamic_dname(dentry, buffer, buflen, "/%s (deleted)",
> > -                               dentry->d_name.name);
> > -}
> > -
> >  static struct dentry_operations anon_ops = {
> > -       .d_dname = shmem_dname
> > +       .d_dname = simple_dname
> >  };
> >
> >  /**
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> Can this patch (committed as 118b23022512eb2f41ce42db70dc0568d00be4ba
> upstream) go in stable?  It fixes a regression introduced in 3.9
> dumping /proc/pid/maps for processes with ashmem regions with long
> names.

Looks good to me, now applied to 3.10-stable, thanks.

greg k-h