Re: [PATCH] Fix seq_read dead loop and trigger memory allocation failure.

[Fengwei Yin]

On Fri, Apr 25, 2014 at 12:29 AM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Apr 24, 2014 at 10:26:50PM +0800, Fengwei Yin wrote:
>> On Thu, Apr 24, 2014 at 5:58 AM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>> > On Mon, Apr 21, 2014 at 10:12:42PM +0800, Fengwei Yin wrote:
>> >> When dump /proc/xxx/maps, if d_path return error in seq_path, the
>> >> buffer will be exhaust and trigger dead loop in seq_read. Till
>> >> kmalloc fails with -ENOMEM.
>> >
>> > *WHAT* d_path error?  -ENAMETOOLONG, aka. "you've got too little space"?
>> >
>> I could check it and get you back. But I suppose it's not this one
>> because it still  fails even I have buffer with 4M size.
>
> Please, do.  One thing to watch out for is bogus ->d_dname() return
> value; instances of ->d_dname() are only allowed to return valid pointers or
> ERR_PTR(-ENAMETOOLONG), the latter - only if the buffer really is too short
> (i.e. disappearing on sufficiently large one).  That holds in mainline, but
> that's the most likely vector by which the breakage could be introduced in
> some out-of-tree code.
>
> Here's the braindump on that bunch (basically, everything in fs/dcache.c
> from prepend() to dentry_path()):
>         * their char * arguments are never ERR_PTR(...)
>         * their char ** arguments never point to ERR_PTR(...) - not on entry
> to function and not on return from it, regardless of return value.
>         * prepend(), prepend_name() and prepend_unreachable() always return
> either 0 or -ENAMETOOLONG.
>         * return value of prepend_path() and path_with_deleted() can only be
> 0, 1, 2 or -ENAMETOOLONG.
>         * __d_path() returns NULL, a pointer to string or
> ERR_PTR(-ENAMETOOLONG).
>         * d_absolute_path() returns a pointer to string, ERR_PTR(-ENAMETOOLONG)
> or ERR_PTR(-EINVAL), the last one being for the case when its path argument
> points into an unmounted vfsmount.
>         * d_path(), __dentry_path(), dentry_path_raw(), dentry_path() and
> ->d_dname() instances return a pointer to string or ERR_PTR(-ENAMETOOLONG).
>         * all in-tree instances of ->d_dname() are either simple_dname() or
> trivial wrappers for dynamic_dname(), so for mainline it's enough to check
> those two helpers; out-of-tree code providing ->d_dname() instances needs
> to be checked, of course.
>         * given sufficiently large buffer ->d_dname() should succeed.
> Persistent ERR_PTR(-ENAMETOOLONG) from it is a bug.  Note that use of
> dynamic_dname() with format that might exceed 64 characters of output
> is wrong; that's the reason why e.g. mm/shmem.c uses simple_dname() instead.
> AFAICS, all remaining callers of dynamic_dname() in mainline are guaranteed
> to stay within its limitations (either <short string>[<unsigned long decimal>]
> or anon_inode:<short string passed to anon_inode_get{file,fd}>).  Out-of-tree
> code needs to be checked, of course.

Cool. I just found that you already fixed the issue (long name in
ashmem) by change
to simple_dname from dynamic_dname on tip. Android still stick to old
kernel and hit
this issue.

Thanks a lot.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/