O_CREAT is stripped when create_error is set in lookup_open(), so when lookup does not return an inode, the case if (!dentry->d_inode && (open_flag & O_CREAT)) is always skipped. We can get rid of this cognitive step by handling the error case first. Reviewed-by: NeilBrown Signed-off-by: Jori Koolstra --- fs/namei.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/fs/namei.c b/fs/namei.c index 19ce43c9a6e6..447eda7cbe34 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -4491,6 +4491,11 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file, } } + if (unlikely(create_error) && !dentry->d_inode) { + error = create_error; + goto out_dput; + } + /* Negative dentry, just create the file */ if (!dentry->d_inode && (open_flag & O_CREAT)) { /* but break the directory lease first! */ @@ -4510,10 +4515,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file, if (error) goto out_dput; } - if (unlikely(create_error) && !dentry->d_inode) { - error = create_error; - goto out_dput; - } + return dentry; out_dput: -- 2.55.0 audit_inode_child() is called in may_create_dentry() so that failed filesystem operations still register an audit entry. On success, the entry is overwritten when, for instance, fsnotify_create() is called. This is the calling convention in vfs_create() and vfs_mkdir(). In lookup_open(), however, when atomic_open() should have created a file but didn't, no call to audit_inode_child() is made. The same is true for the regular ->create() path. Fix the calling of audit_inode_child() in lookup_open() to match the vfs_create() path. For the ->atomic_open() filesystems this logic has been pushed into atomic_open(). This function is also reordered a bit to make the case distinction of the possible returns from ->atomic_open() more explicit (i.e. finish_open() or finish_no_open()). When retrying delegation breaking, audit_inode_child() could be called more than once, but this is OK because those entries are reused. Signed-off-by: Jori Koolstra --- fs/namei.c | 92 ++++++++++++++++++++++++++++++++---------------------- 1 file changed, 55 insertions(+), 37 deletions(-) diff --git a/fs/namei.c b/fs/namei.c index 447eda7cbe34..fe5d59c6f925 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -4351,35 +4351,55 @@ static int may_o_create(struct mnt_idmap *idmap, */ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry, struct file *file, - int open_flag, umode_t mode) + int open_flag, umode_t mode, int create_error) { struct dentry *const DENTRY_NOT_SET = (void *) -1UL; - struct inode *dir = path->dentry->d_inode; + struct inode *dir_inode = path->dentry->d_inode; int error; file->__f_path.dentry = DENTRY_NOT_SET; file->__f_path.mnt = path->mnt; - error = dir->i_op->atomic_open(dir, dentry, file, + error = dir_inode->i_op->atomic_open(dir_inode, dentry, file, open_to_namei_flags(open_flag), mode); d_lookup_done(dentry); + if (!error) { if (file->f_mode & FMODE_OPENED) { - if (unlikely(dentry != file->f_path.dentry)) { + /* finish_open() called */ + struct dentry *opened = file->f_path.dentry; + if (unlikely(opened != dentry)) { dput(dentry); - dentry = dget(file->f_path.dentry); + dentry = dget(opened); } - } else if (WARN_ON(file->f_path.dentry == DENTRY_NOT_SET)) { - error = -EIO; - } else { - if (file->f_path.dentry) { + } else if (likely(file->f_path.dentry != DENTRY_NOT_SET)) { + /* finish_no_open() called */ + struct dentry *replaced = file->f_path.dentry; + if (replaced) { dput(dentry); - dentry = file->f_path.dentry; + dentry = replaced; } if (unlikely(d_is_negative(dentry))) error = -ENOENT; + } else { + const char *fsname = dentry->d_sb->s_type->name; + WARN(1, "%s: ->atomic_open() left file->f_path.dentry unset!\n", + fsname); + error = -EIO; } } + if (error) { + if (unlikely(create_error) && error == -ENOENT) { + /* + * Should have done a create, but errored before. + * Some filesystems return -ENOENT directly instead of + * calling finish_no_open() with a negative dentry; + * either way it should only mean the child doesn't exist, + * so a refused create is safe to record here. + */ + audit_inode_child(dir_inode, dentry, AUDIT_TYPE_CHILD_CREATE); + error = create_error; + } dput(dentry); dentry = ERR_PTR(error); } @@ -4437,7 +4457,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file, dentry = NULL; } if (dentry->d_inode) { - /* Cached positive dentry: will open in f_op->open */ + /* Cached positive dentry: will open in do_open(). */ return dentry; } @@ -4471,10 +4491,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file, if (dir_inode->i_op->atomic_open) { if (nd->flags & LOOKUP_DIRECTORY) open_flag |= O_DIRECTORY; - dentry = atomic_open(&nd->path, dentry, file, open_flag, mode); - if (unlikely(create_error) && dentry == ERR_PTR(-ENOENT)) - dentry = ERR_PTR(create_error); - return dentry; + return atomic_open(&nd->path, dentry, file, open_flag, mode, create_error); } if (d_in_lookup(dentry)) { @@ -4490,32 +4507,36 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file, dentry = res; } } + if (dentry->d_inode || !(op->open_flag & O_CREAT)) { + /* No need to create a file. If lookup returned a positive + * dentry, the file will be opened in do_open(). */ + return dentry; + } + + /* Negative dentry with O_CREAT flag set */ + audit_inode_child(dir_inode, dentry, AUDIT_TYPE_CHILD_CREATE); - if (unlikely(create_error) && !dentry->d_inode) { + if (unlikely(create_error)) { + /* should have done a create, but we already errored */ error = create_error; goto out_dput; } - /* Negative dentry, just create the file */ - if (!dentry->d_inode && (open_flag & O_CREAT)) { - /* but break the directory lease first! */ - error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, delegated_inode); - if (error) - goto out_dput; - - file->f_mode |= FMODE_CREATED; - audit_inode_child(dir_inode, dentry, AUDIT_TYPE_CHILD_CREATE); - if (!dir_inode->i_op->create) { - error = -EACCES; - goto out_dput; - } + error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, delegated_inode); + if (error) + goto out_dput; - error = dir_inode->i_op->create(idmap, dir_inode, dentry, - mode, open_flag & O_EXCL); - if (error) - goto out_dput; + file->f_mode |= FMODE_CREATED; + if (!dir_inode->i_op->create) { + error = -EACCES; + goto out_dput; } + error = dir_inode->i_op->create(idmap, dir_inode, dentry, + mode, open_flag & O_EXCL); + if (error) + goto out_dput; + return dentry; out_dput: @@ -5053,7 +5074,7 @@ struct file *dentry_create(struct path *path, int flags, umode_t mode, /* atomic_open will dput(dentry) on error */ dget(orig_dentry); - dentry = atomic_open(path, dentry, file, flags, mode); + dentry = atomic_open(path, dentry, file, flags, mode, create_error); error = PTR_ERR_OR_ZERO(dentry); if (IS_ERR(dentry)) @@ -5063,9 +5084,6 @@ struct file *dentry_create(struct path *path, int flags, umode_t mode, /* Drop the extra reference */ dput(orig_dentry); - if (unlikely(create_error) && error == -ENOENT) - error = create_error; - if (!error) { if (file->f_mode & FMODE_CREATED) fsnotify_create(dir->d_inode, dentry); -- 2.55.0 The comments above atomic_open() contain several errors: - atomic_open() does not return 0 if successful - @path is not updated Fix those and be more explicit about when FMODE_OPENED and FMODE_CREATED are set. Change to a full kerneldoc. Signed-off-by: Jori Koolstra --- fs/namei.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/fs/namei.c b/fs/namei.c index fe5d59c6f925..2eef2a49d3f4 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -4336,18 +4336,26 @@ static int may_o_create(struct mnt_idmap *idmap, return security_inode_create(dir->dentry->d_inode, dentry, mode); } -/* - * Attempt to atomically look up, create and open a file from a negative - * dentry. - * - * Returns 0 if successful. The file will have been created and attached to - * @file by the filesystem calling finish_open(). - * - * If the file was looked up only or didn't need creating, FMODE_OPENED won't - * be set. The caller will need to perform the open themselves. @path will - * have been updated to point to the new dentry. This may be negative. - * - * Returns an error code otherwise. +/** + * atomic_open() - attempt to atomically look up, create and open a file + * from a negative dentry. + * @path: parent directory path + * @dentry: child to ->atomic_open() + * @file: file to attach child to + * @open_flag: open flags + * @mode: create mode + * @create_error: return value from may_o_create() + * + * If a non-error dentry is returned then: when FMODE_OPENED is set, + * the file will have been attached to @file by the filesystem calling + * finish_open(). If FMODE_OPENED isn't set, the filesystem instead called + * finish_no_open() and the caller will need to perform the open themselves. + * + * FMODE_CREATED is set when the call to ->atomic_open() actually created + * the file. + * + * Returns the opened/looked-up dentry on success or ERR_PTR(-E) on failure. + * On error, atomic_open() consumes @dentry. */ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry, struct file *file, -- 2.55.0