On Fri, Jan 13, 2023 at 02:05:56PM +0400, Konstantin Komarov wrote: > Hello. > > We have added a patch with this check just before the New Year. (here https://lore.kernel.org/lkml/ee705b24-865b-26ff-157d-4cb2a303a962@xxxxxxxxxxxxxxxxxxxx/) See upthread for the reasons why that's wrong. Incidentally, mixing logical change with a pile of whitespace changes is bad idea - it's very easy for reviewers to miss... Other observation from the cursory look through your namei.c: ntfs_create_inode() has no reason to return inode; the reference it creates goes into dentry. Make it return int, the callers will be happier. While we are at it, use d_instantiate_new() instead of d_instantiate() + unlock_new_inode() there. Incidentally, control flow in there is harder to follow that it needs to be: * everything that reaches out{3,4,5,6,7} is guaranteed to have err != 0; * fallthrough into out2 is guaranteed to have err != 0; direct branch to it - err == 0. * direct branch to out1 is guaranteed to have err != 0. I would suggest something along the lines of the following (completely untested) delta; the callers are clearly better off that way and failure paths are separated from the success one - they didn't share anywhere near enough to have it worth bothering. diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c index 51f9542de7b0..3ae4ad329b51 100644 --- a/fs/ntfs3/inode.c +++ b/fs/ntfs3/inode.c @@ -1197,11 +1197,11 @@ ntfs_create_reparse_buffer(struct ntfs_sb_info *sbi, const char *symname, * * NOTE: if fnd != NULL (ntfs_atomic_open) then @dir is locked */ -struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, - struct inode *dir, struct dentry *dentry, - const struct cpu_str *uni, umode_t mode, - dev_t dev, const char *symname, u32 size, - struct ntfs_fnd *fnd) +int ntfs_create_inode(struct user_namespace *mnt_userns, + struct inode *dir, struct dentry *dentry, + const struct cpu_str *uni, umode_t mode, + dev_t dev, const char *symname, u32 size, + struct ntfs_fnd *fnd) { int err; struct super_block *sb = dir->i_sb; @@ -1642,18 +1642,19 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, goto out7; } - /* - * Call 'd_instantiate' after inode->i_op is set - * but before finish_open. - */ - d_instantiate(dentry, inode); - ntfs_save_wsl_perm(inode); mark_inode_dirty(dir); mark_inode_dirty(inode); /* Normal exit. */ - goto out2; + __putname(new_de); + kfree(rp); + /* + * Call 'd_instantiate_new' after inode->i_op is set + * but before finish_open. + */ + d_instantiate_new(dentry, inode); + return 0; out7: @@ -1678,21 +1679,13 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, discard_new_inode(inode); out3: ntfs_mark_rec_free(sbi, ino, false); - out2: __putname(new_de); kfree(rp); - out1: - if (err) { - if (!fnd) - ni_unlock(dir_ni); - return ERR_PTR(err); - } - - unlock_new_inode(inode); - - return inode; + if (!fnd) + ni_unlock(dir_ni); + return err; } int ntfs_link_inode(struct inode *inode, struct dentry *dentry) diff --git a/fs/ntfs3/namei.c b/fs/ntfs3/namei.c index 53ddea219e37..f98d0252a785 100644 --- a/fs/ntfs3/namei.c +++ b/fs/ntfs3/namei.c @@ -107,12 +107,8 @@ static struct dentry *ntfs_lookup(struct inode *dir, struct dentry *dentry, static int ntfs_create(struct user_namespace *mnt_userns, struct inode *dir, struct dentry *dentry, umode_t mode, bool excl) { - struct inode *inode; - - inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFREG | mode, + return ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFREG | mode, 0, NULL, 0, NULL); - - return IS_ERR(inode) ? PTR_ERR(inode) : 0; } /* @@ -123,12 +119,8 @@ static int ntfs_create(struct user_namespace *mnt_userns, struct inode *dir, static int ntfs_mknod(struct user_namespace *mnt_userns, struct inode *dir, struct dentry *dentry, umode_t mode, dev_t rdev) { - struct inode *inode; - - inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, mode, rdev, + return ntfs_create_inode(mnt_userns, dir, dentry, NULL, mode, rdev, NULL, 0, NULL); - - return IS_ERR(inode) ? PTR_ERR(inode) : 0; } /* @@ -196,13 +188,8 @@ static int ntfs_unlink(struct inode *dir, struct dentry *dentry) static int ntfs_symlink(struct user_namespace *mnt_userns, struct inode *dir, struct dentry *dentry, const char *symname) { - u32 size = strlen(symname); - struct inode *inode; - - inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFLNK | 0777, - 0, symname, size, NULL); - - return IS_ERR(inode) ? PTR_ERR(inode) : 0; + return ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFLNK | 0777, + 0, symname, strlen(symname), NULL); } /* @@ -211,12 +198,8 @@ static int ntfs_symlink(struct user_namespace *mnt_userns, struct inode *dir, static int ntfs_mkdir(struct user_namespace *mnt_userns, struct inode *dir, struct dentry *dentry, umode_t mode) { - struct inode *inode; - - inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFDIR | mode, + return ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFDIR | mode, 0, NULL, 0, NULL); - - return IS_ERR(inode) ? PTR_ERR(inode) : 0; } /* @@ -358,7 +341,6 @@ static int ntfs_atomic_open(struct inode *dir, struct dentry *dentry, struct file *file, u32 flags, umode_t mode) { int err; - struct inode *inode; struct ntfs_fnd *fnd = NULL; struct ntfs_inode *ni = ntfs_i(dir); struct dentry *d = NULL; @@ -431,10 +413,10 @@ static int ntfs_atomic_open(struct inode *dir, struct dentry *dentry, * Looks like ntfs_atomic_open must accept 'struct user_namespace *mnt_userns' as argument. */ - inode = ntfs_create_inode(&init_user_ns, dir, dentry, uni, mode, 0, + err = ntfs_create_inode(&init_user_ns, dir, dentry, uni, mode, 0, NULL, 0, fnd); - err = IS_ERR(inode) ? PTR_ERR(inode) - : finish_open(file, dentry, ntfs_file_open); + if (!err) + err = finish_open(file, dentry, ntfs_file_open); dput(d); out2: diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h index 2050eb3f6a5a..cea0b8a3a38a 100644 --- a/fs/ntfs3/ntfs_fs.h +++ b/fs/ntfs3/ntfs_fs.h @@ -707,7 +707,7 @@ int ntfs_sync_inode(struct inode *inode); int ntfs_flush_inodes(struct super_block *sb, struct inode *i1, struct inode *i2); int inode_write_data(struct inode *inode, const void *data, size_t bytes); -struct inode *ntfs_create_inode(struct user_namespace *mnt_userns, +int ntfs_create_inode(struct user_namespace *mnt_userns, struct inode *dir, struct dentry *dentry, const struct cpu_str *uni, umode_t mode, dev_t dev, const char *symname, u32 size,
