[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240807062300.GU5334@ZenIV>
Date: Wed, 7 Aug 2024 07:23:00 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: brauner@...nel.org, jack@...e.cz, linux-kernel@...r.kernel.org,
linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] vfs: avoid spurious dentry ref/unref cycle on open
After having looked at the problem, how about the following
series:
1/5) lift path_get() *AND* path_put() out of do_dentry_open()
into the callers. The latter - conditional upon "do_dentry_open()
has not set FMODE_OPENED". Equivalent transformation.
2/5) move path_get() we'd lifted into the callers past the
call of do_dentry_open(), conditionally collapse it with path_put().
You'd get e.g.
int vfs_open(const struct path *path, struct file *file)
{
int ret;
file->f_path = *path;
ret = do_dentry_open(file, NULL);
if (!ret) {
/*
* Once we return a file with FMODE_OPENED, __fput() will call
* fsnotify_close(), so we need fsnotify_open() here for
* symmetry.
*/
fsnotify_open(file);
}
if (file->f_mode & FMODE_OPENED)
path_get(path);
return ret;
}
Equivalent transformation, provided that nobody is playing silly
buggers with reassigning ->f_path in their ->open() instances.
They *really* should not - if anyone does, we'd better catch them
and fix them^Wtheir code. Incidentally, if we find any such,
we have a damn good reason to add asserts in the callers. As
in, "if do_dentry_open() has set FMODE_OPENED, it would bloody
better *not* modify ->f_path". <greps> Nope, nobody is that
insane.
3/5) split vfs_open_consume() out of vfs_open() (possibly
named vfs_open_borrow()), replace the call in do_open() with
calling the new function.
Trivially equivalent transformation.
4/5) Remove conditional path_get() from vfs_open_consume()
and finish_open(). Add
if (file->f_mode & FMODE_OPENED)
path_get(&nd->path);
before terminate_walk(nd); in path_openat().
Equivalent transformation - see
if (file->f_mode & (FMODE_OPENED | FMODE_CREATED)) {
dput(nd->path.dentry);
nd->path.dentry = dentry;
return NULL;
}
in lookup_open() (which is where nd->path gets in sync with what
had been given to do_dentry_open() in finish_open()); in case
of vfs_open_consume() in do_open() it's in sync from the very
beginning. And we never modify nd->path after those points.
So we can move grabbing it downstream, keeping it under the
same condition (which also happens to be true only if we'd
called do_dentry_open(), so for all other paths through the
whole thing it's a no-op.
5/5) replace
if (file->f_mode & FMODE_OPENED)
path_get(&nd->path);
terminate_walk(nd);
with
if (file->f_mode & FMODE_OPENED) {
nd->path.mnt = NULL;
nd->path.dentry = NULL;
}
terminate_walk(nd);
Again, an obvious equivalent transformation.
Powered by blists - more mailing lists