Re: [PATCH v2] vfs: avoid recopying file names in getname_flags

[Al Viro <viro@ZenIV.linux.org.uk>]

On Sun, Apr 12, 2015 at 12:56:55AM +0100, Al Viro wrote:

> What for?  It's not as if userland memory had been communicated with by
> IP over carrier pigeons, after all, and the cost of 4Kb worth of
> (essentially) memcpy() is going to be
> 	a) incurred in extremely rare case
> and
> 	b) be dwarfed by the work we need to _do_ with what we'd copied.
> After all, that pathname is going to be parsed and traversed - all 4Kb
> worth of it.
> 
> So what's the point?

BTW, looking at the __getname() callers...  Lustre one sure as hell looks
bogus:
        char *tmp = __getname();

        if (!tmp)
                return ERR_PTR(-ENOMEM);

        len = strncpy_from_user(tmp, filename, PATH_MAX);
        if (len == 0)
                ret = -ENOENT;
        else if (len > PATH_MAX)
                ret = -ENAMETOOLONG;

        if (ret) {
                __putname(tmp);
                tmp =  ERR_PTR(ret);
        }
        return tmp;

Note that
	* strncpy_from_user(p, u, n) can return a negative (-EFAULT)
	* strncpy_from_user(p, u, n) cannot return a positive greater than
n.  In case of missing NUL among the n bytes starting at u (and no faults
accessing those) we get exactly n bytes copied and n returned.  In case
when NUL is there, we copy everything up to and including that NUL and
return number of non-NUL bytes copied.

IOW, these failure cases had never been tested.  Name being too long ends up
with non-NUL-terminated array of characters returned, and the very first
caller of ll_getname() feeds it to strlen().  Fault ends up with uninitialized
array...

AFAICS, the damn thing should just use getname() and quit reinventing the
wheel, badly.

As for your question in another thread - AFAICS, it's impossible with the
current code, but not too robust.  Fortunately, it's trivial to make
independent on allocator details - all it takes is
	result = kzalloc(offsetof(struct filename, iname[1]), GFP_KERNEL);
and we are done - result->iname+1 will be within the allocated object,
no matter what.