Re: [PATCH v2 1/1] dcache: Translating dentry into pathname withouttaking rename_lock

[Linus Torvalds]

On Thu, Sep 5, 2013 at 11:55 AM, Waiman Long <Waiman.Long@xxxxxx> wrote:
> +static int prepend_name(char **buffer, int *buflen, struct dentry *dentry)
>  {
> -       return prepend(buffer, buflen, name->name, name->len);
> +       /*
> +        * With RCU path tracing, it may race with rename. Use
> +        * ACCESS_ONCE() to make sure that it is either the old or
> +        * the new name pointer. The length does not really matter as
> +        * the sequence number check will eventually catch any ongoing
> +        * rename operation.
> +        */
> +       const char *dname = ACCESS_ONCE(dentry->d_name.name);
> +       u32 dlen = dentry->d_name.len;
> +       int error;
> +
> +       if (likely(dname == (const char *)dentry->d_iname)) {
> +               /*
> +                * Internal dname, the string memory is valid as long
> +                * as its length is not over the limit.
> +                */
> +               if (unlikely(dlen > sizeof(dentry->d_iname)))
> +                       return -EINVAL;
> +       } else if (!dname)
> +               return -EINVAL;
> +       else {
> +               const char *ptr;
> +               u32 len;
> +
> +               /*
> +                * External dname, need to fetch name pointer and length
> +                * again under d_lock to get a consistent set and avoid
> +                * racing with d_move() which will take d_lock before
> +                * acting on the dentries.
> +                */
> +               spin_lock(&dentry->d_lock);
> +               dname = dentry->d_name.name;
> +               dlen  = dentry->d_name.len;
> +               spin_unlock(&dentry->d_lock);
> +
> +               if (unlikely(!dname || !dlen))
> +                       return -EINVAL;
> +               /*
> +                * As the length and the content of the string may not be
> +                * valid, need to scan the string and return EINVAL if there
> +                * is embedded null byte within the length of the string.
> +                */
> +               for (ptr = dname, len = dlen; len; len--, ptr++) {
> +                       if (*ptr == '\0')
> +                               return -EINVAL;
> +               }
> +       }
> +       error = prepend(buffer, buflen, dname, dlen);

No. Stop all these stupid games.

No d_lock, no trying to make d_name/d_len consistent.

No "compare d_name against d_iname".

No EINVAL.

No idiotic racy "let's fetch each byte one-by one and test them
against NUL", which is just racy and stupid.

Just do what I told you to do: *copy* the name one byte at a time, and
stop copying if you hit NUL. IOW, make prepend() just use "strncpy()"
instead of "memcpy()". You don't even need to check the end result -
if it's bogus, the sequence count will fix it.

No special cases. No games. No crap. Just make "prepend()" able to
handle the special case of "oops, the name length didn't match, but we
must not ever go past the end of the buffer".

We can optimize strncpy() to use word accesses if it ends up ever
being a performance issue. I suspect it never even shows up, but it's
not hard to do if if does.

            Linus
--
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/