On Wed, Aug 07, 2024 at 07:23:00AM +0100, Al Viro wrote:
> 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.
BTW, similar to that, with that we could turn do_o_path()
into
struct path path;
int error = path_lookupat(nd, flags, &path);
if (!error) {
audit_inode(nd->name, path.dentry, 0);
error = vfs_open_borrow(&path, file);
if (!(file->f_mode & FMODE_OPENED))
path_put(&path);
}
return error;
}
and perhaps do something similar in the vicinity of
vfs_tmpfile() / do_o_tmpfile().
