>From 932a134abeac597f18c86c513704709ad154994b Mon Sep 17 00:00:00 2001
From: Arun KS <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
> 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.
Hi Al Viro,
On Tue, Aug 20, 2013 at 11:21 AM, Al Viro <[email protected]> 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 <[email protected]>
>> 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 <[email protected]>
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 <[email protected]>
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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...
Hi Al Viro,
On Tue, Aug 20, 2013 at 1:06 PM, Al Viro <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
---
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
};
/**
Hi Al Viro,
On Tue, Aug 20, 2013 at 7:34 PM, Al Viro <[email protected]> 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<[email protected]>
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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Tue, Aug 20, 2013 at 7:04 AM, Al Viro <[email protected]> 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 <[email protected]>
> ---
> 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 [email protected]
> 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.
On Mon, Oct 14, 2013 at 03:57:04PM -0700, Colin Cross wrote:
> On Tue, Aug 20, 2013 at 7:04 AM, Al Viro <[email protected]> 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 <[email protected]>
> > ---
> > 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 [email protected]
> > 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