[PATCH v2] vfs: Don't exchange "short" filenames unconditionally.

[PATCH v2] vfs: Don't exchange "short" filenames unconditionally.

[Mikhail Efremov]

Only exchange source and destination filenames
if flags contain RENAME_EXCHANGE.
In case if executable file was running and replaced by
other file /proc/PID/exe should still show correct file name,
not the old name of the file by which it was replaced.

The scenario when this bug manifests itself was like this:
* ALT Linux uses rpm and start-stop-daemon;
* during a package upgrade rpm creates a temporary file
  for an executable to rename it upon successful unpacking;
* start-stop-daemon is run subsequently and it obtains
  the (nonexistant) temporary filename via /proc/PID/exe
  thus failing to identify the running process.

Note that "long" filenames (> DNAiME_INLINE_LEN) are still
exchanged without RENAME_EXCHANGE and this behaviour exists
long enough (should be fixed too apparently).
So this patch is just an interim workaround that restores
behavior for "short" names as it was before changes
introduced by commit da1ce0670c14 ("vfs: add cross-rename").

See https://lkml.org/lkml/2014/9/7/6 for details.

Acked-by: Miklos Szeredi <mszeredi@suse.cz>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Cc: stable@vger.kernel.org
Fixes: da1ce0670c14 "vfs: add cross-rename"
Signed-off-by: Mikhail Efremov <sem@altlinux.org>
---
 fs/dcache.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 7a5b514..3218570 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2372,7 +2372,8 @@ void dentry_update_name_case(struct dentry *dentry, struct qstr *name)
 }
 EXPORT_SYMBOL(dentry_update_name_case);
 
-static void switch_names(struct dentry *dentry, struct dentry *target)
+static void switch_names(struct dentry *dentry, struct dentry *target,
+			 bool exchange)
 {
 	if (dname_external(target)) {
 		if (dname_external(dentry)) {
@@ -2404,11 +2405,21 @@ static void switch_names(struct dentry *dentry, struct dentry *target)
 			/*
 			 * Both are internal.
 			 */
-			unsigned int i;
-			BUILD_BUG_ON(!IS_ALIGNED(DNAME_INLINE_LEN, sizeof(long)));
-			for (i = 0; i < DNAME_INLINE_LEN / sizeof(long); i++) {
-				swap(((long *) &dentry->d_iname)[i],
-				     ((long *) &target->d_iname)[i]);
+			if (exchange) {
+				unsigned int i;
+
+				BUILD_BUG_ON(!IS_ALIGNED(DNAME_INLINE_LEN,
+							 sizeof(long)));
+				for (i = 0; i < DNAME_INLINE_LEN / sizeof(long);
+									i++) {
+					swap(((long *) &dentry->d_iname)[i],
+					     ((long *) &target->d_iname)[i]);
+				}
+			} else {
+				memcpy(dentry->d_iname, target->d_name.name,
+						target->d_name.len + 1);
+				dentry->d_name.len = target->d_name.len;
+				return;
 			}
 		}
 	}
@@ -2510,7 +2521,7 @@ static void __d_move(struct dentry *dentry, struct dentry *target,
 	list_del(&target->d_u.d_child);
 
 	/* Switch the names.. */
-	switch_names(dentry, target);
+	switch_names(dentry, target, exchange);
 	swap(dentry->d_name.hash, target->d_name.hash);
 
 	/* ... and switch the parents */
@@ -2649,7 +2660,7 @@ static void __d_materialise_dentry(struct dentry *dentry, struct dentry *anon)
 
 	dparent = dentry->d_parent;
 
-	switch_names(dentry, anon);
+	switch_names(dentry, anon, false);
 	swap(dentry->d_name.hash, anon->d_name.hash);
 
 	dentry->d_parent = dentry;
-- 
1.8.5.5

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

[Al Viro]

On Wed, Sep 24, 2014 at 10:14:33PM +0400, Mikhail Efremov wrote:
> Only exchange source and destination filenames
> if flags contain RENAME_EXCHANGE.
> In case if executable file was running and replaced by
> other file /proc/PID/exe should still show correct file name,
> not the old name of the file by which it was replaced.

Yecchhhh...  Applied, but it's very ugly.  Oh, well - regression is
regression, and I don't see a cleaner fix at the moment.  If I don't
manage to come up with anything prettier, to Linus it goes in tonight
pull request ;-/

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

[Linus Torvalds]

On Wed, Sep 24, 2014 at 11:55 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Yecchhhh...  Applied, but it's very ugly.  Oh, well - regression is
> regression, and I don't see a cleaner fix at the moment.  If I don't
> manage to come up with anything prettier, to Linus it goes in tonight
> pull request ;-/

Please don't. That thing is too ugly to exist. It also looks
completely and utterly buggy. There's no way I'm taking it. If
switch-names is suddenly conditional, what the f*ck happens to the
name hash which is unconditionally done with a swap() right
afterwards.

There's no way that patch is correct.

           Linus

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

[Linus Torvalds]

On Wed, Sep 24, 2014 at 12:20 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> There's no way that patch is correct.

Something like the appended might be a good idea regardless. And might
have made people notice that the len and the hash go together with the
name.

             Linus

----

    diff --git a/fs/dcache.c b/fs/dcache.c
    index 7a5b51440afa..aa1b045b997c 100644
    --- a/fs/dcache.c
    +++ b/fs/dcache.c
    @@ -2412,7 +2412,7 @@ static void switch_names(struct dentry
*dentry, struct dentry *target)
                            }
                    }
            }
    -       swap(dentry->d_name.len, target->d_name.len);
    +       swap(dentry->d_name.hash_len, target->d_name.hash_len);
     }

     static void dentry_lock_for_move(struct dentry *dentry, struct
dentry *target)
    @@ -2511,7 +2511,6 @@ static void __d_move(struct dentry *dentry,
struct dentry *target,

            /* Switch the names.. */
            switch_names(dentry, target);
    -       swap(dentry->d_name.hash, target->d_name.hash);

            /* ... and switch the parents */
            if (IS_ROOT(dentry)) {
    @@ -2650,7 +2649,6 @@ static void __d_materialise_dentry(struct
dentry *dentry, struct dentry *anon)
            dparent = dentry->d_parent;

            switch_names(dentry, anon);
    -       swap(dentry->d_name.hash, anon->d_name.hash);

            dentry->d_parent = dentry;
            list_del_init(&dentry->d_u.d_child);

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

[Al Viro]

On Wed, Sep 24, 2014 at 12:20:38PM -0700, Linus Torvalds wrote:
> On Wed, Sep 24, 2014 at 11:55 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > Yecchhhh...  Applied, but it's very ugly.  Oh, well - regression is
> > regression, and I don't see a cleaner fix at the moment.  If I don't
> > manage to come up with anything prettier, to Linus it goes in tonight
> > pull request ;-/
> 
> Please don't. That thing is too ugly to exist. It also looks
> completely and utterly buggy. There's no way I'm taking it. If
> switch-names is suddenly conditional, what the f*ck happens to the
> name hash which is unconditionally done with a swap() right
> afterwards.

The sucker's unhashed after that...  And yes, I agree that it's fucking
ugly.  Still looking for saner ways to do that...

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

[Al Viro]

On Wed, Sep 24, 2014 at 09:18:13PM +0100, Al Viro wrote:
> On Wed, Sep 24, 2014 at 12:20:38PM -0700, Linus Torvalds wrote:
> > On Wed, Sep 24, 2014 at 11:55 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > >
> > > Yecchhhh...  Applied, but it's very ugly.  Oh, well - regression is
> > > regression, and I don't see a cleaner fix at the moment.  If I don't
> > > manage to come up with anything prettier, to Linus it goes in tonight
> > > pull request ;-/
> > 
> > Please don't. That thing is too ugly to exist. It also looks
> > completely and utterly buggy. There's no way I'm taking it. If
> > switch-names is suddenly conditional, what the f*ck happens to the
> > name hash which is unconditionally done with a swap() right
> > afterwards.
> 
> The sucker's unhashed after that...  And yes, I agree that it's fucking
> ugly.  Still looking for saner ways to do that...

I really wonder if it's possible to get d_rehash() hitting the victim of
(non-exchange) __d_move().  _Then_ this patch (as well as the historical
behaviour it restores, all way back to 2.5, if not 2.3) would, indeed,
be buggy.

I'd probably better sleep on that and finish YACrawlingTFS tomorrow morning
- it's nearly 1am here and I've got only 5 hours of sleep left until it's time
to get the kids up for school ;-/

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

[Al Viro]

On Thu, Sep 25, 2014 at 05:46:01AM +0100, Al Viro wrote:
> On Wed, Sep 24, 2014 at 09:18:13PM +0100, Al Viro wrote:
> > On Wed, Sep 24, 2014 at 12:20:38PM -0700, Linus Torvalds wrote:
> > > On Wed, Sep 24, 2014 at 11:55 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > > >
> > > > Yecchhhh...  Applied, but it's very ugly.  Oh, well - regression is
> > > > regression, and I don't see a cleaner fix at the moment.  If I don't
> > > > manage to come up with anything prettier, to Linus it goes in tonight
> > > > pull request ;-/
> > > 
> > > Please don't. That thing is too ugly to exist. It also looks
> > > completely and utterly buggy. There's no way I'm taking it. If
> > > switch-names is suddenly conditional, what the f*ck happens to the
> > > name hash which is unconditionally done with a swap() right
> > > afterwards.
> > 
> > The sucker's unhashed after that...  And yes, I agree that it's fucking
> > ugly.  Still looking for saner ways to do that...
> 
> I really wonder if it's possible to get d_rehash() hitting the victim of
> (non-exchange) __d_move().  _Then_ this patch (as well as the historical
> behaviour it restores, all way back to 2.5, if not 2.3) would, indeed,
> be buggy.
> 
> I'd probably better sleep on that and finish YACrawlingTFS tomorrow morning
> - it's nearly 1am here and I've got only 5 hours of sleep left until it's time
> to get the kids up for school ;-/

FWIW, the longer I'm looking at that, the more it seems that __d_move() and
__d_materialise_dentry() ought to be merged.  The thing is, both callers of
the latter are followed by the same sequence:
                        write_sequnlock(&rename_lock);
			_d_rehash(anon);
			spin_unlock(&anon->d_lock);
(in one case - verbatim, in another - with goto thrown in before _d_rehash()).
Now, there's no good reason for doing that write_sequnlock() first; both
_d_rehash() and spin_unlock() are fast enough to make contention on
rename_lock a non-issue.  And if we pull those before write_sequnlock() and
into __d_materialise_dentry(dentry, anon), we get rid not only of code
duplication, but of the "it returns with anon->d_lock held" caution as well,
*and* __d_materialise_dentry() becomes very similar to __d_move().

	Note that __d_move(x, y) depends on !IS_ROOT(y), while
__d_materialise_dentry(x, y) assumes (and is obviously guaranteed by
callers) IS_ROOT(y).  IOW, even leaving aside the "is it an exchange" argument
fed to __d_move(), we could distinguish between these guys by IS_ROOT(y)
alone.  And there's a fuckload of duplication between them even now, let alone
after pulling the rehash in.

And d_splice_alias() definitely ought to be merged with d_materialise_unique().
Making it accept hashed dentries had been a mistake - there are _very_ few
places where that happens.  One in d_add_ci() is my mistake (and essentially
a dead code - case-insensitive XFS doesn't get hashed negative dentries,
so this codepath in d_add_ci() never triggers), the one in gfs2_create_inode()
is trivially avoided - it should be "fail with EISDIR if that inode is a
directory, do d_instantiate() otherwise" and one in __gfs2_lookup() can only
be reached for a hashed dentry from gfs2_atomic_open(), which has no business
calling gfs2_lookup() if dentry it got from caller is hashed.  And that's
it - only 3 callsites where we could call d_splice_alias() for hashed dentry.

With those taken care of, we really have no reason not to fold
d_materialise_unique() into d_splice_alias() - just teach the latter to
do __d_unalias() in case when existing directory alias is not IS_ROOT.
d_splice_alias() fails with EIO in that case, mostly since on a local fs
that can only happen due to fs corruption.  No reason not to do __d_unalias()
instead.

What's more, d_add_unique() and d_instantiate_unique() are also very
dubious.  The use of the latter in proc_ns_get_dentry() is complete BS,
since there the dentry passed to it is an IS_ROOT() one we'd just obtained
from d_alloc_pseudo(), so the loop over aliases in __d_instantiate_unique()
has no chance whatsoever to find anything it would like - none of them
could possibly have the same ->d_parent value.  IOW, that call is plain
and simple pessimization of d_instantiate().  Which leaves us with exactly
one caller - d_add_unique().  Which itself has exactly one caller.  This:
        dentry = opendata->dentry;
        if (dentry->d_inode == NULL) {
                /* FIXME: Is this d_drop() ever needed? */
                d_drop(dentry);
                dentry = d_add_unique(dentry, igrab(state->inode));
                if (dentry == NULL) {
                        dentry = opendata->dentry;
                } else if (dentry != ctx->dentry) {
                        dput(ctx->dentry);
                        ctx->dentry = dget(dentry); 
                }
First of all, *is* this d_drop() needed, indeed?  Can that dentry be
already hashed?  FWIW, in case it has been hashed we can't expect any aliases
to satisfy d_instantiate_unique() - for that we would need to have a really
weird state just before d_drop(); positive and negative dentries with the
same parent and the same name, which obviously shouldn't happen.  So in
case when dentry has been hashed when we got there (if that's possible at
all), we might as well just call d_instatiate() and be done with that.
And when it's not hashed...  What's wrong with just using d_materialise_unique()
there?  I'm not familiar enough with the guts of NFS4 (and I've had more than
enough of digging through the really nasty callchains during the last few
days), so some comments from NFS folks would be very welcome...

AFAICS, we have a good chance to fold that bunch of suckers into one
primitive.  Moreover, the same primitive ought to be used through all
->lookup() instances, even on non-exportable filesystems where we are
still using d_add().  The thing is, d_splice_alias() will only hit the
fastpath on those, which does the same thing d_add() does.  That's
the only reason why d_add() is valid there in the first place.  And
checking for that fastpath is cheap - we need to check that inode is
non-NULL anyway (to decide whether we need to take ->i_lock) and we
need to take ->i_lock and at least fetch ->i_alias.next in case when
inode isn't NULL.  The only thing remaining to decide that d_splice_alias()
can take the fastpath is checking that inode isn't a directory one.

That basically leaves d_add() to be used only when something is populating
a synthetic tree.  And even there I'm not sure it makes sense to keep it
a separate primitive - inodes in those remaining callers tend to be
brand-new, which would send d_splice_alias() on the fastpath simply because
->i_alias will be empty.  So in theory we might get away with *all* that
stuff merged into one primitive: d_add/d_splice_alias/d_materialise_unique/
d_add_unique to be used when we want hashed result.  FWIW, all those guys
had started as forked-off variants of d_add(), so we would just be folding
them all back...

As an aside, I really *hate* the way lustre crowd has spewed out the wonders
like ll_d_hlist_empty() and its ilk.  The whole thing is a major eye-bleeder
as it is, but this "If you want to grep for something, you'll just have to
grep for our pointless wrapper separately.  Give us respect, bitch - we are
reeeal special!" kind of attitude is extremely annoying.  Could we please
take all that shite out?  Not drivers/staging/lustre, tempting as it might be,
just those wrappers?

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

[Al Viro]

On Thu, Sep 25, 2014 at 05:46:01AM +0100, Al Viro wrote:

> I really wonder if it's possible to get d_rehash() hitting the victim of
> (non-exchange) __d_move().  _Then_ this patch (as well as the historical
> behaviour it restores, all way back to 2.5, if not 2.3) would, indeed,
> be buggy.

More fun: what's going on in ceph_handle_notrace_create()?  AFAICS, this
        struct dentry *result = ceph_lookup(dir, dentry, 0);

        if (result && !IS_ERR(result)) {
                /*
                 * We created the item, then did a lookup, and found
                 * it was already linked to another inode we already
                 * had in our cache (and thus got spliced).  Link our
                 * dentry to that inode, but don't hash it, just in  
                 * case the VFS wants to dereference it.
                 */
                BUG_ON(!result->d_inode);
                d_instantiate(dentry, result->d_inode);
                return 0;
        }
is bogus.  What will happen if server goes nuts and that existing alias picked
by lookup turns out to be a directory?  And while we are at it, what's to
prevent a leak if we ever hit that codepath, directory or no directory?
Sage?

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

[Al Viro]

On Fri, Sep 26, 2014 at 05:44:42PM +0100, Al Viro wrote:

> FWIW, the longer I'm looking at that, the more it seems that __d_move() and
> __d_materialise_dentry() ought to be merged.  The thing is, both callers of
> the latter are followed by the same sequence:
>                         write_sequnlock(&rename_lock);
> 			_d_rehash(anon);
> 			spin_unlock(&anon->d_lock);
> (in one case - verbatim, in another - with goto thrown in before _d_rehash()).
> Now, there's no good reason for doing that write_sequnlock() first; both
> _d_rehash() and spin_unlock() are fast enough to make contention on
> rename_lock a non-issue.  And if we pull those before write_sequnlock() and
> into __d_materialise_dentry(dentry, anon), we get rid not only of code
> duplication, but of the "it returns with anon->d_lock held" caution as well,
> *and* __d_materialise_dentry() becomes very similar to __d_move().
> 
> 	Note that __d_move(x, y) depends on !IS_ROOT(y), while
> __d_materialise_dentry(x, y) assumes (and is obviously guaranteed by
> callers) IS_ROOT(y).  IOW, even leaving aside the "is it an exchange" argument
> fed to __d_move(), we could distinguish between these guys by IS_ROOT(y)
> alone.  And there's a fuckload of duplication between them even now, let alone
> after pulling the rehash in.

It's actually even better.  __d_materialise_dentry() has its arguments in
the wrong order.  Flip them around and it's very nearly an exact copy of
what we do in __d_move() when the first argument is IS_ROOT().  Actually,
that case in __d_move() had been added back in 2002 exactly for the needs
of d_splice_alias().  And it's been unreachable since this Bruce has
switched d_splice_alias() to use of __d_materialise_dentry().  In fact,
both d_splice_alias() and d_materialise_unique() should've been switched
to __d_move() instead.

I've done that massage and removal of __d_materialise_dentry().  The things
look a lot saner now that all dentry moving is done in one place, IMO.  Not to
mention that it trims ~50 lines off fs/dcache.c, driving it down to the 3rd
place in fs/*.c ;-)

Anyway, I've put the branch with fixes + that stuff in vfs.git#for-linus;
I have *NOT* put the "keep names on overwriting rename" horror in there,
but I believe that we will need something of that sort.

Linus, it's a real userland regression.  Yes, it affects only shitty scripts,
and they had never been reliable for long names anyway, but we have broken
real-world userland code by that.  What happened there is that old behaviour
of removal-by-rename wrt d_path() used to be similar to that on unlink.
cd /tmp; touch foo; touch bar
exec 42<bar
mv foo bar
ls -l /proc/self/fd/42
would give you
lr-x------ 1 al al 64 Sep 26 23:44 /proc/self/fd/42 -> /tmp/bar (deleted)
Now it gives "/tmp/foo (deleted)".  The trouble being, the same goes for
/proc/*/exe.  If you upgraded a package and that had replaced a running binary,
you used to be able to find process still running that binary.  After the
upgrade.  And yes, it was an unreliable kludge and the value of that was
in the best case dubious, but there really are people who used to do it.
And it broke after we'd added RENAME_EXCHANGE support.  The old behaviour
of switch_names() in short names case used to be "copy the name/length
of target to source dentry, swap hash keys and piss on hash key of target
not matching target's name anymore - the sucker is unhashed anyway".  For
RENAME_EXCHANGE we certainly needed to swap the names in all cases -
precisely because both suckers are hashed in the end.  And since we had
done that in exchange case, it was simpler to do it in all cases.  Especially
since when either name had been a long one we'd always swapped them, so
anything relying on old behaviour had been unreliable anyway.

Except that it turns out that old behaviour (keep the last component of
victim on normal rename) was relied upon...  That ucking fugly patch
reverts the non-swapping renames to the old behaviour.  I don't like it
any more than you do - it *is* ugly as hell and I still can't swear that
we don't have a bogus codepath leading to d_rehash() of rename() victim
(which would break things very badly with the old behaviour).  Still,
it is a real-world userland regression.

Up to you, but I'm afraid that it's on the "we get to keep supporting that"
side of things.

Anyway, what I've got in vfs.git#for-linus right now seems to survive the
obvious tests so far, still running through full xfstests; please, take
a look at that pile.  The last one should go with you as author - it was
just faster to do the change manually than deal with git-am failure when
applying your patch.  I'll take the headers from your mail and update that
commit before sending the pull request.

Shortlog:
Al Viro (8):
      ufs: deal with nfsd/iget races
      pull rehashing and unlocking the target dentry into __d_materialise_dentry()
      don't open-code d_rehash() in d_materialise_unique()
      __d_move(): fold manipulations with ->d_child/->d_subdirs
      __d_materialise_dentry(): flip the order of arguments
      kill __d_materialise_dentry()
      fold unlocking the children into dentry_unlock_parents_for_move()
      fold swapping ->d_name.hash into switch_names()

Miklos Szeredi (2):
      shmem: fix nlink for rename overwrite directory
      fuse: honour max_read and max_write in direct_io mode

Diffstat:
 fs/dcache.c         |   85 +++++++++++----------------------------------------
 fs/direct-io.c      |    2 +-
 fs/fuse/file.c      |    1 +
 fs/ufs/ialloc.c     |    6 +++-
 fs/ufs/namei.c      |    4 +++
 include/linux/uio.h |    2 +-
 mm/iov_iter.c       |   14 ++++++---
 mm/shmem.c          |    4 ++-
 8 files changed, 42 insertions(+), 76 deletions(-)

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

[Linus Torvalds]

On Fri, Sep 26, 2014 at 9:45 PM, Al Viro <viro@zeniv.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

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

[Al Viro]

On Sat, Sep 27, 2014 at 10:56:57AM -0700, Linus Torvalds wrote:

>  - 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.

Er?  Source dentry refcount doesn't matter at all - *that* one gets
rehashed, so we must get its name right, no matter what.  The target
one is where it becomes interesting.  And I doubt it's worth bothering
with refcount checks on that either, TBH...

The reason why we really can't get it right for long names without more
serious surgery is that currently the lifetime of external name is
the same as of dentry having that name.  And that makes "just copy them
over, long or not" a problem - we suddenly get two dentries sharing the
external name.  And we hit that under a shitload of spinlocks, so
allocating a separate copy isn't attractive.  Mikhail (IIRC - I hadn't
watched this thread all that closely back then, so I might be misattributing)
had proposed refcounting the external names.  That would work, but we'll
need to get it right.  Sure, we can make that refcount atomic_t; it's not
as if we'll be playing with it a lot.  However, if the *source* name had
been external, we need to drop the refcount on that, and we need to
RCU-delay actual freeing.  Right now the delay comes from RCU-delaying
__d_free(), with some optimisations for dentries that had never been hashed;
this will be somewhat hairier.

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

See above.  I can do that, but it will be less trivial than you seem to
expect it to be.  Anyway, the situation right now: vfs.git#for-linus
survives the testing (and I'd like your S-o-b on the last one-liner there;
if you have saner commit message - even better).  That can go in right
now, as far as I'm concerned.

The minimal port of Mikhail's patch on top of that is below; it doesn't
include what you are asking for, it's just an update to the situation past
merging __d_move() and __d_materialise_dentry().

We can get the long name cases right, and I agree that it'll make the
things nicer, but it might take a couple of days to get right.  The thing
I'm concerned about is not screwing DCACHE_RCUACCESS up.

BTW, the comments about being more careful with ->d_name.hash than with
->d_name.name are from back in 2.3.40s; they became obsolete by 2.3.60s,
when we started to unhash the target instead of swapping hash chain positions
followed by d_delete() as we used to do when dcache was first introduced.
It's a really old piece of detritus...

diff --git a/fs/dcache.c b/fs/dcache.c
index 7599d35..cb25a1a 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2372,7 +2372,8 @@ void dentry_update_name_case(struct dentry *dentry, struct qstr *name)
 }
 EXPORT_SYMBOL(dentry_update_name_case);
 
-static void switch_names(struct dentry *dentry, struct dentry *target)
+static void switch_names(struct dentry *dentry, struct dentry *target,
+			 bool exchange)
 {
 	if (dname_external(target)) {
 		if (dname_external(dentry)) {
@@ -2406,6 +2407,12 @@ static void switch_names(struct dentry *dentry, struct dentry *target)
 			 */
 			unsigned int i;
 			BUILD_BUG_ON(!IS_ALIGNED(DNAME_INLINE_LEN, sizeof(long)));
+			if (!exchange) {
+				memcpy(dentry->d_iname, target->d_name.name,
+						target->d_name.len + 1);
+				dentry->d_name.hash_len = target->d_name.hash_len;
+				return;
+			}
 			for (i = 0; i < DNAME_INLINE_LEN / sizeof(long); i++) {
 				swap(((long *) &dentry->d_iname)[i],
 				     ((long *) &target->d_iname)[i]);
@@ -2456,12 +2463,15 @@ static void dentry_unlock_for_move(struct dentry *dentry, struct dentry *target)
  * When switching names, the actual string doesn't strictly have to
  * be preserved in the target - because we're dropping the target
  * anyway. As such, we can just do a simple memcpy() to copy over
- * the new name before we switch.
- *
- * Note that we have to be a lot more careful about getting the hash
- * switched - we have to switch the hash value properly even if it
- * then no longer matches the actual (corrupted) string of the target.
- * The hash value has to match the hash queue that the dentry is on..
+ * the new name before we switch, unless we are going to rehash
+ * it.  Note that if we *do* unhash the target, we are not allowed
+ * to rehash it without giving it a new name/hash key - whether
+ * we swap or overwrite the names here, resulting name won't match
+ * the reality in filesystem; it's only there for d_path() purposes.
+ * Note that all of this is happening under rename_lock, so the
+ * any hash lookup seeing it in the middle of manipulations will
+ * be discarded anyway.  So we do not care what happens to the hash
+ * key in that case.
  */
 /*
  * __d_move - move a dentry
@@ -2507,9 +2517,8 @@ static void __d_move(struct dentry *dentry, struct dentry *target,
 			   d_hash(dentry->d_parent, dentry->d_name.hash));
 	}
 
-
 	/* Switch the names.. */
-	switch_names(dentry, target);
+	switch_names(dentry, target, exchange);
 
 	/* ... and switch them in the tree */
 	if (IS_ROOT(dentry)) {

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

[Al Viro]

On Sat, Sep 27, 2014 at 07:31:39PM +0100, Al Viro wrote:

> We can get the long name cases right, and I agree that it'll make the
> things nicer, but it might take a couple of days to get right.  The thing
> I'm concerned about is not screwing DCACHE_RCUACCESS up.

FWIW, I suspect that the right approach is to put refcount + rcu_head in
front of external name and do the following:
	* __d_free() checks if we have an external name, gets its containing
structure and does if (atomic_dec_and_test(&name->count)) kfree(name);
	* switch_names() in non-exchange case (I'd probably call it copy_name,
not move_names, but anyway) sets DCACHE_RCUACCESS on target (source has
already gotten it from __d_rehash()), increments refcount on target's name
if external and, if the source old name is external, decrements its refcount
and calls kfree_rcu() if it has hit zero.

AFAICS, it guarantees that we'll schedule an RCU callback on name's rch_head
at most once, that we won't free it while RCU callback on it is scheduled
and we won't free it until a grace period has expired since the last time
it had been referenced by observable dentries.  Do you see any holes in that?

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

[Linus Torvalds]

On Sat, Sep 27, 2014 at 12:16 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> FWIW, I suspect that the right approach is to put refcount + rcu_head in
> front of external name and do the following:

That's actually fancier than I was expecting. I was just thinking
doing a whole new allocation and freeing the old one using RCU. But I
guess you're right, you do need the rcu_head even for that, and once
you start adding fields you might as well just add a refcount too, and
then you don't have the annoyance of a potential memory allocation in
that code.

So your approach is better and doesn't sound too painful at all.

But yeah, I guess we can/should do the trivial ugly thing for 3.17.
Send me a proper pull-request,

                  Linus

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

[Linus Torvalds]

On Sat, Sep 27, 2014 at 12:37 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But yeah, I guess we can/should do the trivial ugly thing for 3.17.
> Send me a proper pull-request,

.. and if it can happen before tomorrow, that would be good. I had to
give up on my wish to release 3.17 tomorrow due to the current size of
changes anyway, so there will be a rc7 (and my travel inconveniences
be damned), but I'd like this to be in that rc7. Works for you?

             Linus

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

[Al Viro]

On Sat, Sep 27, 2014 at 08:16:57PM +0100, Al Viro wrote:
> On Sat, Sep 27, 2014 at 07:31:39PM +0100, Al Viro wrote:
> 
> > We can get the long name cases right, and I agree that it'll make the
> > things nicer, but it might take a couple of days to get right.  The thing
> > I'm concerned about is not screwing DCACHE_RCUACCESS up.
> 
> FWIW, I suspect that the right approach is to put refcount + rcu_head in
> front of external name and do the following:
> 	* __d_free() checks if we have an external name, gets its containing
> structure and does if (atomic_dec_and_test(&name->count)) kfree(name);
> 	* switch_names() in non-exchange case (I'd probably call it copy_name,
> not move_names, but anyway) sets DCACHE_RCUACCESS on target (source has
> already gotten it from __d_rehash()), increments refcount on target's name
> if external and, if the source old name is external, decrements its refcount
> and calls kfree_rcu() if it has hit zero.
> 
> AFAICS, it guarantees that we'll schedule an RCU callback on name's rch_head
> at most once, that we won't free it while RCU callback on it is scheduled
> and we won't free it until a grace period has expired since the last time
> it had been referenced by observable dentries.  Do you see any holes in that?

We probably want to put a union of refcount and rcu_head there, actually...
Gives the right alignment without padding.  As in
struct ext_name {
	union {
		atomic_t count;
		struct rcu_head head;
	};
	char name[0];
};
->count corresponds to the number of dentries that have ->d_name.name
pointing to the sucker's ->name.  And we use ->head only when it reaches
zero in __d_move().  That's 2 words per external name; somewhat unpleasant
on 64bit, but I don't see how to avoid an rcu_head in there...  The cutoff
for external names is 32 bytes on 64bit boxen.  That way we get 16 bytes
of overhead per long-named dentry...  OTOH, we allocate them with kmalloc(),
so it means that 32-character names lead to 64-bytes actual allocation.

Hmmm...  So the old behaviour is
 32--63 	=> 64 byte allocation
 64--95 	=> 96
 96--127	=> 128
and the new one
 32--47 	=> 64 byte allocation
 48--79 	=> 96
 80--111	=> 128
112--127	=> 192
(components longer than 128 characters are definitely too rare to worry about)
IOW, the main worry is about the names in range from 48 to 64 characters;
for those we push the allocation from size-64 to size-96...

Note, BTW, that git hits external name case on everything except 32-bit UP;
a _lot_ of 38-character names there.  And IIRC there had been some plans for
possible replacement of SHA1 with something wider, right?

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

[Al Viro]

On Sat, Sep 27, 2014 at 12:39:02PM -0700, Linus Torvalds wrote:
> On Sat, Sep 27, 2014 at 12:37 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > But yeah, I guess we can/should do the trivial ugly thing for 3.17.
> > Send me a proper pull-request,
> 
> .. and if it can happen before tomorrow, that would be good. I had to
> give up on my wish to release 3.17 tomorrow due to the current size of
> changes anyway, so there will be a rc7 (and my travel inconveniences
> be damned), but I'd like this to be in that rc7. Works for you?

Fine by me.  How about that s-o-b?  Right now it's

commit 4f78a56cd96a3d421852b5a03e10355b0cbe764b
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed Sep 24 12:27:39 2014 -0700

    fold swapping ->d_name.hash into switch_names()
    
    and do it along with ->d_name.len there
    
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/fs/dcache.c b/fs/dcache.c
index 92f7d76..7599d35 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2412,7 +2412,7 @@ static void switch_names(struct dentry *dentry, struct dentry *target)
                        }
                }
        }
-       swap(dentry->d_name.len, target->d_name.len);
+       swap(dentry->d_name.hash_len, target->d_name.hash_len);
 }
 
 static void dentry_lock_for_move(struct dentry *dentry, struct dentry *target)
@@ -2510,7 +2510,6 @@ static void __d_move(struct dentry *dentry, struct dentry *target,
 
        /* Switch the names.. */
        switch_names(dentry, target);
-       swap(dentry->d_name.hash, target->d_name.hash);
 
        /* ... and switch them in the tree */
        if (IS_ROOT(dentry)) {

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

[Linus Torvalds]

On Sat, Sep 27, 2014 at 12:49 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Fine by me.  How about that s-o-b?  Right now it's

Sure, add my sign-off too. Not that I feel I even need the credit, so
you could just have done it as yourself. But I'll take it, so..

  Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Thanks,
        Linus

[git pull] vfs.git for 3.17-rc7

[Al Viro]

Assorted fixes + unifying __d_move() and __d_materialise_dentry() +
minimal regression fix for d_path() of victims of overwriting rename()
ported on top of that.  Please, pull from

git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus

Shortlog:
Al Viro (7):
      ufs: deal with nfsd/iget races
      pull rehashing and unlocking the target dentry into __d_materialise_dentry()
      don't open-code d_rehash() in d_materialise_unique()
      __d_move(): fold manipulations with ->d_child/->d_subdirs
      __d_materialise_dentry(): flip the order of arguments
      kill __d_materialise_dentry()
      fold unlocking the children into dentry_unlock_parents_for_move()

Linus Torvalds (1):
      fold swapping ->d_name.hash into switch_names()

Mikhail Efremov (1):
      vfs: Don't exchange "short" filenames unconditionally.

Miklos Szeredi (2):
      shmem: fix nlink for rename overwrite directory
      fuse: honour max_read and max_write in direct_io mode

Diffstat:
 fs/dcache.c         |  112 +++++++++++++++++----------------------------------
 fs/direct-io.c      |    2 +-
 fs/fuse/file.c      |    1 +
 fs/ufs/ialloc.c     |    6 ++-
 fs/ufs/namei.c      |    4 ++
 include/linux/uio.h |    2 +-
 mm/iov_iter.c       |   14 ++++---
 mm/shmem.c          |    4 +-
 8 files changed, 60 insertions(+), 85 deletions(-)

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

[Mikhail Efremov]

On Sat, 27 Sep 2014 10:56:57 -0700 Linus Torvalds wrote:
> (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).

Well, I can't argue with the fact that this patch is ugly, especially
after all this discussion.
I just hope that the right solution will be implemented soon and this
ugly hack will be dropped.

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

I'll definetly take a look at this, but I doubt that I will be able to
implement the right solution soon. Actually this is the first time when
I look at the kernel code so deeply.
I hope that someone with much more knowledge of the kernel code than my
own (Al or Miklos may be) will do that.

-- 
WBR, Mikhail Efremov