linux-kernel - Re: [PATCH v2] vfs: Don't exchange "short" filenames unconditionally.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFyw8U94tYVB9Xbo7gh1SC_f9ecKSoFL509bXM122dC46Q@mail.gmail.com>
Date:	Sat, 27 Sep 2014 10:56:57 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Al Viro <viro@...iv.linux.org.uk>
Cc:	Mikhail Efremov <sem@...linux.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Miklos Szeredi <mszeredi@...e.cz>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	stable <stable@...r.kernel.org>
Subject: Re: [PATCH v2] vfs: Don't exchange "short" filenames unconditionally.

On Fri, Sep 26, 2014 at 9:45 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
>
> Linus, it's a real userland regression.

That's not the part I'm arguing against. I think the patch itself was
too ugly to live.

Yes, we had that hack before, but we didn't make it conditional. It
historically was more of a "it's easier to just memcpy the name" than
switch things around. Then that became accidental semantics, and
that's all normal. But then when we make this explicit and
intentional, I really think we should do it *right*, and either
switch() the names around or just copy it.

Having a "switch_names()" function that *neither* switches *nor*
copies, and giving it an argument to decide which, but not even do it
*right*? That's just too f*cking disgusting for words.

So seriously, I think we should:

 - keep the "switch_names()" as is, since it actually does what the
name says it does, and some callers want to statically do exactly
that.

 - make a new "move_name(src,dst)" that explicitly moves names, and
explicitly knows that the source needs to be kept alive (although it
*could* also look at the source dentry refcount to decide whether it
needs to be careful or not). And do it right for the long names too,
not just the inline case.

 - make that __d_move() caller just pick one or the other explicitly.

See what my complaint is? Not this half-assery that used to be a small
random detail, and that the patch makes into an institutionalized and
explicit half-assery.

(And Mikhail - I'm not ragging on you, even if I'm ragging on the
patch. I understand why you did it the way you did, and it makes sense
exactly in the "let's reinstate old hackery" model. I just think we
can and should do better than that, now that the "exchange" vs "move
over" semantics are so explicit).

Al (or Mikhail) - willing to do that extra cleanup? Please?

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