On Thu, Sep 21, 2023 at 12:48 AM Bernd Schubert <bschubert@xxxxxxx> wrote:
>
> This was suggested by Miklos based on review from the previous
> patch that introduced atomic open for positive dentries.
> In open_last_lookups() the dentry was not used anymore when atomic
> revalidate was available, which required to release the dentry,
> then code fall through to lookup_open was done, which resulted
> in another d_lookup and also d_revalidate. All of that can
> be avoided by the new atomic_revalidate_open() function.
>
> Another included change is the introduction of an enum as
> d_revalidate return code.
>
> Signed-off-by: Bernd Schubert <bschubert@xxxxxxx>
> Cc: Miklos Szeredi <miklos@xxxxxxxxxx>
> Cc: Dharmendra Singh <dsingh@xxxxxxx>
> Cc: Christian Brauner <brauner@xxxxxxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: linux-fsdevel@xxxxxxxxxxxxxxx
> ---
> fs/namei.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index f01b278ac0ef..8ad7a0dfa596 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3388,6 +3388,44 @@ static struct dentry *atomic_open(struct nameidata *nd, struct dentry *dentry,
> return dentry;
> }
>
> +static struct dentry *atomic_revalidate_open(struct dentry *dentry,
> + struct nameidata *nd,
> + struct file *file,
> + const struct open_flags *op,
> + bool *got_write)
> +{
> + struct mnt_idmap *idmap;
> + struct dentry *dir = nd->path.dentry;
> + struct inode *dir_inode = dir->d_inode;
> + int open_flag = op->open_flag;
> + umode_t mode = op->mode;
> +
> + if (unlikely(IS_DEADDIR(dir_inode)))
> + return ERR_PTR(-ENOENT);
> +
> + file->f_mode &= ~FMODE_CREATED;
> +
> + if (unlikely(open_flag & O_CREAT)) {
> + WARN_ON(1);
if (WARN_ON_ONCE(open_flag & O_CREAT)) {
is more compact and produces a nicer print
> + return ERR_PTR(-EINVAL);
> + }
> +
> + if (open_flag & (O_TRUNC | O_WRONLY | O_RDWR))
> + *got_write = !mnt_want_write(nd->path.mnt);
> + else
> + *got_write = false;
> +
> + if (!got_write)
> + open_flag &= ~O_TRUNC;
> +
> + inode_lock_shared(dir->d_inode);
> + dentry = atomic_open(nd, dentry, file, open_flag, mode);
> + inode_unlock_shared(dir->d_inode);
> +
> + return dentry;
> +
> +}
> +
> /*
> * Look up and maybe create and open the last component.
> *
> @@ -3541,8 +3579,10 @@ static const char *open_last_lookups(struct nameidata *nd,
> if (IS_ERR(dentry))
> return ERR_CAST(dentry);
> if (dentry && unlikely(atomic_revalidate)) {
> - dput(dentry);
> - dentry = NULL;
> + BUG_ON(nd->flags & LOOKUP_RCU);
The assertion means that someone wrote bad code
it does not mean that the kernel internal state is hopelessly corrupted.
Please avoid BUG_ON and use WARN_ON_ONCE and if possible
(seems to be the case here) also take the graceful error path.
It's better to fail an open than to crash the kernel.
Thanks,
Amir.
> + dentry = atomic_revalidate_open(dentry, nd, file, op,
> + &got_write);
> + goto drop_write;
> }
> if (likely(dentry))
> goto finish_lookup;
> @@ -3580,6 +3620,7 @@ static const char *open_last_lookups(struct nameidata *nd,
> else
> inode_unlock_shared(dir->d_inode);
>
> +drop_write:
> if (got_write)
> mnt_drop_write(nd->path.mnt);
>
> --
> 2.39.2
>
