| Message ID | 20230710061021.2303432-1-chao@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev] f2fs: fix potential deadlock by reordering w/ i_sem | expand |
On 07/10, Chao Yu wrote: > syzbot reports deadlock bug as below: > > -> #1 (&fi->i_sem){+.+.}-{3:3}: > down_write+0x3a/0x50 kernel/locking/rwsem.c:1573 > f2fs_down_write fs/f2fs/f2fs.h:2133 [inline] > f2fs_add_inline_entry+0x3a8/0x760 fs/f2fs/inline.c:644 > f2fs_add_dentry+0xba/0x1e0 fs/f2fs/dir.c:784 > f2fs_do_add_link+0x21e/0x340 fs/f2fs/dir.c:827 > f2fs_add_link fs/f2fs/f2fs.h:3554 [inline] > f2fs_create+0x32c/0x530 fs/f2fs/namei.c:377 > lookup_open fs/namei.c:3492 [inline] > open_last_lookups fs/namei.c:3560 [inline] > path_openat+0x13e7/0x3180 fs/namei.c:3790 > do_filp_open+0x234/0x490 fs/namei.c:3820 > do_sys_openat2+0x13e/0x1d0 fs/open.c:1407 > do_sys_open fs/open.c:1422 [inline] > __do_sys_open fs/open.c:1430 [inline] > __se_sys_open fs/open.c:1426 [inline] > __x64_sys_open+0x225/0x270 fs/open.c:1426 > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 > entry_SYSCALL_64_after_hwframe+0x63/0xcd > > -> #0 (&fi->i_xattr_sem){.+.+}-{3:3}: > check_prev_add kernel/locking/lockdep.c:3142 [inline] > check_prevs_add kernel/locking/lockdep.c:3261 [inline] > validate_chain kernel/locking/lockdep.c:3876 [inline] > __lock_acquire+0x39ff/0x7f70 kernel/locking/lockdep.c:5144 > lock_acquire+0x1e3/0x520 kernel/locking/lockdep.c:5761 > down_read+0x47/0x2f0 kernel/locking/rwsem.c:1520 > f2fs_down_read fs/f2fs/f2fs.h:2108 [inline] > f2fs_getxattr+0xb8/0x1460 fs/f2fs/xattr.c:532 > __f2fs_get_acl+0x52/0x8e0 fs/f2fs/acl.c:179 > f2fs_acl_create fs/f2fs/acl.c:377 [inline] > f2fs_init_acl+0xd7/0x9a0 fs/f2fs/acl.c:420 > f2fs_init_inode_metadata+0x824/0x1190 fs/f2fs/dir.c:558 > f2fs_do_tmpfile+0x34/0x170 fs/f2fs/dir.c:839 > __f2fs_tmpfile+0x1f9/0x380 fs/f2fs/namei.c:884 > f2fs_ioc_start_atomic_write+0x4a3/0x9e0 fs/f2fs/file.c:2099 > __f2fs_ioctl+0x1b5c/0xb770 > vfs_ioctl fs/ioctl.c:51 [inline] > __do_sys_ioctl fs/ioctl.c:870 [inline] > __se_sys_ioctl+0xf8/0x170 fs/ioctl.c:856 > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 > entry_SYSCALL_64_after_hwframe+0x63/0xcd > > The root cause is as below reversed lock order: > - f2fs_create > - f2fs_add_dentry > - f2fs_down_write(&F2FS_I(dir)->i_xattr_sem) > - f2fs_ioc_start_atomic_write > - __f2fs_tmpfile > - f2fs_do_tmpfile > - f2fs_down_write(&F2FS_I(inode)->i_sem) > - f2fs_init_inode_metadata > - f2fs_init_acl > - __f2fs_get_acl > - f2fs_getxattr > - f2fs_down_read(&F2FS_I(dir)->i_xattr_sem) > - f2fs_add_inline_entry > - f2fs_down_write(&F2FS_I(inode)->i_sem) How is it possible to get two inode are identical? > > We can break the dependency of deadlock by below change: > - use i_sem to keep order of {get,set}xattr instead of i_xattr_sem > - keep below lock order in inode operation to avoid deadlock: > dir->i_sem -> inode->i_sem > dir->i_sem -> dpage_lock > > Fixes: 5eda1ad1aaff ("f2fs: fix deadlock in i_xattr_sem and inode page lock") > Reported-by: syzbot+e5600587fa9cbf8e3826@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/linux-f2fs-devel/00000000000096797d06001a359d@google.com > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/acl.c | 36 +++++++++++++++++++----------------- > fs/f2fs/acl.h | 4 ++-- > fs/f2fs/dir.c | 22 ++++++++++++++++------ > fs/f2fs/f2fs.h | 1 - > fs/f2fs/super.c | 5 ++--- > fs/f2fs/verity.c | 5 +++-- > fs/f2fs/xattr.c | 37 +++++++++++++++++++++---------------- > fs/f2fs/xattr.h | 19 +++++++++++++------ > 8 files changed, 76 insertions(+), 53 deletions(-) > > diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c > index ec2aeccb69a3..af58b38e953c 100644 > --- a/fs/f2fs/acl.c > +++ b/fs/f2fs/acl.c > @@ -166,7 +166,7 @@ static void *f2fs_acl_to_disk(struct f2fs_sb_info *sbi, > } > > static struct posix_acl *__f2fs_get_acl(struct inode *inode, int type, > - struct page *dpage) > + struct page *dpage, bool locked) > { > int name_index = F2FS_XATTR_INDEX_POSIX_ACL_DEFAULT; > void *value = NULL; > @@ -176,13 +176,13 @@ static struct posix_acl *__f2fs_get_acl(struct inode *inode, int type, > if (type == ACL_TYPE_ACCESS) > name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS; > > - retval = f2fs_getxattr(inode, name_index, "", NULL, 0, dpage); > + retval = f2fs_getxattr(inode, name_index, "", NULL, 0, dpage, locked); > if (retval > 0) { > value = f2fs_kmalloc(F2FS_I_SB(inode), retval, GFP_F2FS_ZERO); > if (!value) > return ERR_PTR(-ENOMEM); > retval = f2fs_getxattr(inode, name_index, "", value, > - retval, dpage); > + retval, dpage, locked); > } > > if (retval > 0) > @@ -201,7 +201,7 @@ struct posix_acl *f2fs_get_acl(struct inode *inode, int type, bool rcu) > if (rcu) > return ERR_PTR(-ECHILD); > > - return __f2fs_get_acl(inode, type, NULL); > + return __f2fs_get_acl(inode, type, NULL, false); > } > > static int f2fs_acl_update_mode(struct mnt_idmap *idmap, > @@ -226,9 +226,9 @@ static int f2fs_acl_update_mode(struct mnt_idmap *idmap, > return 0; > } > > -static int __f2fs_set_acl(struct mnt_idmap *idmap, > - struct inode *inode, int type, > - struct posix_acl *acl, struct page *ipage) > +static int __f2fs_set_acl(struct mnt_idmap *idmap, struct inode *inode, > + int type, struct posix_acl *acl, > + struct page *ipage, bool locked) > { > int name_index; > void *value = NULL; > @@ -266,7 +266,8 @@ static int __f2fs_set_acl(struct mnt_idmap *idmap, > } > } > > - error = f2fs_setxattr(inode, name_index, "", value, size, ipage, 0); > + error = f2fs_setxattr(inode, name_index, "", value, size, > + ipage, 0, locked); > > kfree(value); > if (!error) > @@ -284,7 +285,7 @@ int f2fs_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, > if (unlikely(f2fs_cp_error(F2FS_I_SB(inode)))) > return -EIO; > > - return __f2fs_set_acl(idmap, inode, type, acl, NULL); > + return __f2fs_set_acl(idmap, inode, type, acl, NULL, false); > } > > /* > @@ -362,7 +363,7 @@ static int f2fs_acl_create_masq(struct posix_acl *acl, umode_t *mode_p) > > static int f2fs_acl_create(struct inode *dir, umode_t *mode, > struct posix_acl **default_acl, struct posix_acl **acl, > - struct page *dpage) > + struct page *dpage, bool locked) > { > struct posix_acl *p; > struct posix_acl *clone; > @@ -374,7 +375,7 @@ static int f2fs_acl_create(struct inode *dir, umode_t *mode, > if (S_ISLNK(*mode) || !IS_POSIXACL(dir)) > return 0; > > - p = __f2fs_get_acl(dir, ACL_TYPE_DEFAULT, dpage); > + p = __f2fs_get_acl(dir, ACL_TYPE_DEFAULT, dpage, locked); > if (!p || p == ERR_PTR(-EOPNOTSUPP)) { > *mode &= ~current_umask(); > return 0; > @@ -412,28 +413,29 @@ static int f2fs_acl_create(struct inode *dir, umode_t *mode, > } > > int f2fs_init_acl(struct inode *inode, struct inode *dir, struct page *ipage, > - struct page *dpage) > + struct page *dpage, bool locked) > { > struct posix_acl *default_acl = NULL, *acl = NULL; > int error; > > - error = f2fs_acl_create(dir, &inode->i_mode, &default_acl, &acl, dpage); > + error = f2fs_acl_create(dir, &inode->i_mode, &default_acl, &acl, > + dpage, locked); > if (error) > return error; > > f2fs_mark_inode_dirty_sync(inode, true); > > if (default_acl) { > - error = __f2fs_set_acl(NULL, inode, ACL_TYPE_DEFAULT, default_acl, > - ipage); > + error = __f2fs_set_acl(NULL, inode, ACL_TYPE_DEFAULT, > + default_acl, ipage, locked); > posix_acl_release(default_acl); > } else { > inode->i_default_acl = NULL; > } > if (acl) { > if (!error) > - error = __f2fs_set_acl(NULL, inode, ACL_TYPE_ACCESS, acl, > - ipage); > + error = __f2fs_set_acl(NULL, inode, ACL_TYPE_ACCESS, > + acl, ipage, locked); > posix_acl_release(acl); > } else { > inode->i_acl = NULL; > diff --git a/fs/f2fs/acl.h b/fs/f2fs/acl.h > index 94ebfbfbdc6f..9c14b6f549c6 100644 > --- a/fs/f2fs/acl.h > +++ b/fs/f2fs/acl.h > @@ -37,13 +37,13 @@ extern struct posix_acl *f2fs_get_acl(struct inode *, int, bool); > extern int f2fs_set_acl(struct mnt_idmap *, struct dentry *, > struct posix_acl *, int); > extern int f2fs_init_acl(struct inode *, struct inode *, struct page *, > - struct page *); > + struct page *, bool); > #else > #define f2fs_get_acl NULL > #define f2fs_set_acl NULL > > static inline int f2fs_init_acl(struct inode *inode, struct inode *dir, > - struct page *ipage, struct page *dpage) > + struct page *ipage, struct page *dpage, bool locked) > { > return 0; > } > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > index d635c58cf5a3..4b5c62e18d67 100644 > --- a/fs/f2fs/dir.c > +++ b/fs/f2fs/dir.c > @@ -540,6 +540,8 @@ struct page *f2fs_init_inode_metadata(struct inode *inode, struct inode *dir, > int err; > > if (is_inode_flag_set(inode, FI_NEW_INODE)) { > + struct f2fs_xattr_arg xarg; > + > page = f2fs_new_inode_page(inode); > if (IS_ERR(page)) > return page; > @@ -555,12 +557,16 @@ struct page *f2fs_init_inode_metadata(struct inode *inode, struct inode *dir, > put_page(page); > } > > - err = f2fs_init_acl(inode, dir, page, dpage); > + err = f2fs_init_acl(inode, dir, page, dpage, true); > if (err) > goto put_error; > > + xarg.page = page; > + xarg.locked = true; > + > err = f2fs_init_security(inode, dir, > - fname ? fname->usr_fname : NULL, page); > + fname ? fname->usr_fname : NULL, > + &xarg); > if (err) > goto put_error; > > @@ -775,18 +781,20 @@ int f2fs_add_dentry(struct inode *dir, const struct f2fs_filename *fname, > { > int err = -EAGAIN; > > + f2fs_down_read(&F2FS_I(dir)->i_sem); > + > if (f2fs_has_inline_dentry(dir)) { > /* > - * Should get i_xattr_sem to keep the lock order: > - * i_xattr_sem -> inode_page lock used by f2fs_setxattr. > + * Should get i_sem to keep the lock order: > + * i_sem -> inode_page lock used by f2fs_setxattr. > */ > - f2fs_down_read(&F2FS_I(dir)->i_xattr_sem); > err = f2fs_add_inline_entry(dir, fname, inode, ino, mode); > - f2fs_up_read(&F2FS_I(dir)->i_xattr_sem); > } > if (err == -EAGAIN) > err = f2fs_add_regular_entry(dir, fname, inode, ino, mode); > > + f2fs_up_read(&F2FS_I(dir)->i_sem); > + > f2fs_update_time(F2FS_I_SB(dir), REQ_TIME); > return err; > } > @@ -835,6 +843,7 @@ int f2fs_do_tmpfile(struct inode *inode, struct inode *dir) > struct page *page; > int err = 0; > > + f2fs_down_write(&F2FS_I(dir)->i_sem); > f2fs_down_write(&F2FS_I(inode)->i_sem); > page = f2fs_init_inode_metadata(inode, dir, NULL, NULL); > if (IS_ERR(page)) { > @@ -847,6 +856,7 @@ int f2fs_do_tmpfile(struct inode *inode, struct inode *dir) > f2fs_update_time(F2FS_I_SB(inode), REQ_TIME); > fail: > f2fs_up_write(&F2FS_I(inode)->i_sem); > + f2fs_up_write(&F2FS_I(dir)->i_sem); > return err; > } > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index c7cb2177b252..60ec032be48d 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -838,7 +838,6 @@ struct f2fs_inode_info { > > /* avoid racing between foreground op and gc */ > struct f2fs_rwsem i_gc_rwsem[2]; > - struct f2fs_rwsem i_xattr_sem; /* avoid racing between reading and changing EAs */ > > int i_extra_isize; /* size of extra space located in i_addr */ > kprojid_t i_projid; /* id for project quota */ > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index ca31163da00a..c72fda24cffd 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -1418,7 +1418,6 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb) > INIT_LIST_HEAD(&fi->gdirty_list); > init_f2fs_rwsem(&fi->i_gc_rwsem[READ]); > init_f2fs_rwsem(&fi->i_gc_rwsem[WRITE]); > - init_f2fs_rwsem(&fi->i_xattr_sem); > > /* Will be used by directory only */ > fi->i_dir_level = F2FS_SB(sb)->dir_level; > @@ -3163,7 +3162,7 @@ static int f2fs_get_context(struct inode *inode, void *ctx, size_t len) > { > return f2fs_getxattr(inode, F2FS_XATTR_INDEX_ENCRYPTION, > F2FS_XATTR_NAME_ENCRYPTION_CONTEXT, > - ctx, len, NULL); > + ctx, len, NULL, false); > } > > static int f2fs_set_context(struct inode *inode, const void *ctx, size_t len, > @@ -3183,7 +3182,7 @@ static int f2fs_set_context(struct inode *inode, const void *ctx, size_t len, > > return f2fs_setxattr(inode, F2FS_XATTR_INDEX_ENCRYPTION, > F2FS_XATTR_NAME_ENCRYPTION_CONTEXT, > - ctx, len, fs_data, XATTR_CREATE); > + ctx, len, fs_data, XATTR_CREATE, true); > } > > static const union fscrypt_policy *f2fs_get_dummy_policy(struct super_block *sb) > diff --git a/fs/f2fs/verity.c b/fs/f2fs/verity.c > index 4fc95f353a7a..e181528b1f26 100644 > --- a/fs/f2fs/verity.c > +++ b/fs/f2fs/verity.c > @@ -181,7 +181,7 @@ static int f2fs_end_enable_verity(struct file *filp, const void *desc, > /* Set the verity xattr. */ > err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_VERITY, > F2FS_XATTR_NAME_VERITY, &dloc, sizeof(dloc), > - NULL, XATTR_CREATE); > + NULL, XATTR_CREATE, false); > if (err) > goto cleanup; > > @@ -226,7 +226,8 @@ static int f2fs_get_verity_descriptor(struct inode *inode, void *buf, > > /* Get the descriptor location */ > res = f2fs_getxattr(inode, F2FS_XATTR_INDEX_VERITY, > - F2FS_XATTR_NAME_VERITY, &dloc, sizeof(dloc), NULL); > + F2FS_XATTR_NAME_VERITY, &dloc, sizeof(dloc), > + NULL, false); > if (res < 0 && res != -ERANGE) > return res; > if (res != sizeof(dloc) || dloc.version != cpu_to_le32(F2FS_VERIFY_VER)) { > diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c > index 476b186b90a6..a6a611f8a771 100644 > --- a/fs/f2fs/xattr.c > +++ b/fs/f2fs/xattr.c > @@ -61,7 +61,7 @@ static int f2fs_xattr_generic_get(const struct xattr_handler *handler, > return -EINVAL; > } > return f2fs_getxattr(inode, handler->flags, name, > - buffer, size, NULL); > + buffer, size, NULL, false); > } > > static int f2fs_xattr_generic_set(const struct xattr_handler *handler, > @@ -84,7 +84,7 @@ static int f2fs_xattr_generic_set(const struct xattr_handler *handler, > return -EINVAL; > } > return f2fs_setxattr(inode, handler->flags, name, > - value, size, NULL, flags); > + value, size, NULL, flags, false); > } > > static bool f2fs_xattr_user_list(struct dentry *dentry) > @@ -136,15 +136,16 @@ static int f2fs_xattr_advise_set(const struct xattr_handler *handler, > > #ifdef CONFIG_F2FS_FS_SECURITY > static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array, > - void *page) > + void *fs_data) > { > + struct f2fs_xattr_arg *xarg = (struct f2fs_xattr_arg *)fs_data; > const struct xattr *xattr; > int err = 0; > > for (xattr = xattr_array; xattr->name != NULL; xattr++) { > err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY, > xattr->name, xattr->value, > - xattr->value_len, (struct page *)page, 0); > + xattr->value_len, xarg->page, 0, xarg->locked); > if (err < 0) > break; > } > @@ -152,10 +153,11 @@ static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array, > } > > int f2fs_init_security(struct inode *inode, struct inode *dir, > - const struct qstr *qstr, struct page *ipage) > + const struct qstr *qstr, > + struct f2fs_xattr_arg *xarg) > { > return security_inode_init_security(inode, dir, qstr, > - &f2fs_initxattrs, ipage); > + &f2fs_initxattrs, xarg); > } > #endif > > @@ -512,7 +514,8 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize, > } > > int f2fs_getxattr(struct inode *inode, int index, const char *name, > - void *buffer, size_t buffer_size, struct page *ipage) > + void *buffer, size_t buffer_size, > + struct page *ipage, bool locked) > { > struct f2fs_xattr_entry *entry = NULL; > int error; > @@ -528,12 +531,12 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name, > if (len > F2FS_NAME_LEN) > return -ERANGE; > > - if (!ipage) > - f2fs_down_read(&F2FS_I(inode)->i_xattr_sem); > + if (!locked) > + f2fs_down_read(&F2FS_I(inode)->i_sem); > error = lookup_all_xattrs(inode, ipage, index, len, name, > &entry, &base_addr, &base_size, &is_inline); > - if (!ipage) > - f2fs_up_read(&F2FS_I(inode)->i_xattr_sem); > + if (!locked) > + f2fs_up_read(&F2FS_I(inode)->i_sem); > if (error) > return error; > > @@ -567,9 +570,9 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) > int error; > size_t rest = buffer_size; > > - f2fs_down_read(&F2FS_I(inode)->i_xattr_sem); > + f2fs_down_read(&F2FS_I(inode)->i_sem); > error = read_all_xattrs(inode, NULL, &base_addr); > - f2fs_up_read(&F2FS_I(inode)->i_xattr_sem); > + f2fs_up_read(&F2FS_I(inode)->i_sem); > if (error) > return error; > > @@ -775,7 +778,7 @@ static int __f2fs_setxattr(struct inode *inode, int index, > > int f2fs_setxattr(struct inode *inode, int index, const char *name, > const void *value, size_t size, > - struct page *ipage, int flags) > + struct page *ipage, int flags, bool locked) > { > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > int err; > @@ -796,9 +799,11 @@ int f2fs_setxattr(struct inode *inode, int index, const char *name, > f2fs_balance_fs(sbi, true); > > f2fs_lock_op(sbi); > - f2fs_down_write(&F2FS_I(inode)->i_xattr_sem); > + if (!locked) > + f2fs_down_write(&F2FS_I(inode)->i_sem); > err = __f2fs_setxattr(inode, index, name, value, size, ipage, flags); > - f2fs_up_write(&F2FS_I(inode)->i_xattr_sem); > + if (!locked) > + f2fs_up_write(&F2FS_I(inode)->i_sem); > f2fs_unlock_op(sbi); > > f2fs_update_time(sbi, REQ_TIME); > diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h > index b1811c392e6f..170e4a49af31 100644 > --- a/fs/f2fs/xattr.h > +++ b/fs/f2fs/xattr.h > @@ -52,6 +52,11 @@ struct f2fs_xattr_entry { > char e_name[]; /* attribute name */ > }; > > +struct f2fs_xattr_arg { > + struct page *page; /* inode page */ > + bool locked; /* indicate i_sem is locked */ > +}; > + > #define XATTR_HDR(ptr) ((struct f2fs_xattr_header *)(ptr)) > #define XATTR_ENTRY(ptr) ((struct f2fs_xattr_entry *)(ptr)) > #define XATTR_FIRST_ENTRY(ptr) (XATTR_ENTRY(XATTR_HDR(ptr) + 1)) > @@ -128,9 +133,9 @@ extern const struct xattr_handler f2fs_xattr_security_handler; > extern const struct xattr_handler *f2fs_xattr_handlers[]; > > extern int f2fs_setxattr(struct inode *, int, const char *, > - const void *, size_t, struct page *, int); > + const void *, size_t, struct page *, int, bool); > extern int f2fs_getxattr(struct inode *, int, const char *, void *, > - size_t, struct page *); > + size_t, struct page *, bool locked); > extern ssize_t f2fs_listxattr(struct dentry *, char *, size_t); > extern int f2fs_init_xattr_caches(struct f2fs_sb_info *); > extern void f2fs_destroy_xattr_caches(struct f2fs_sb_info *); > @@ -140,13 +145,13 @@ extern void f2fs_destroy_xattr_caches(struct f2fs_sb_info *); > #define f2fs_listxattr NULL > static inline int f2fs_setxattr(struct inode *inode, int index, > const char *name, const void *value, size_t size, > - struct page *page, int flags) > + struct page *page, int flags, bool locked) > { > return -EOPNOTSUPP; > } > static inline int f2fs_getxattr(struct inode *inode, int index, > const char *name, void *buffer, > - size_t buffer_size, struct page *dpage) > + size_t buffer_size, struct page *dpage, bool locked) > { > return -EOPNOTSUPP; > } > @@ -156,10 +161,12 @@ static inline void f2fs_destroy_xattr_caches(struct f2fs_sb_info *sbi) { } > > #ifdef CONFIG_F2FS_FS_SECURITY > extern int f2fs_init_security(struct inode *, struct inode *, > - const struct qstr *, struct page *); > + const struct qstr *, > + struct f2fs_xattr_arg *xarg); > #else > static inline int f2fs_init_security(struct inode *inode, struct inode *dir, > - const struct qstr *qstr, struct page *ipage) > + const struct qstr *qstr, > + struct f2fs_xattr_arg *xarg) > { > return 0; > } > -- > 2.40.1
On 2023/7/11 1:18, Jaegeuk Kim wrote: > On 07/10, Chao Yu wrote: >> syzbot reports deadlock bug as below: >> >> -> #1 (&fi->i_sem){+.+.}-{3:3}: >> down_write+0x3a/0x50 kernel/locking/rwsem.c:1573 >> f2fs_down_write fs/f2fs/f2fs.h:2133 [inline] >> f2fs_add_inline_entry+0x3a8/0x760 fs/f2fs/inline.c:644 >> f2fs_add_dentry+0xba/0x1e0 fs/f2fs/dir.c:784 >> f2fs_do_add_link+0x21e/0x340 fs/f2fs/dir.c:827 >> f2fs_add_link fs/f2fs/f2fs.h:3554 [inline] >> f2fs_create+0x32c/0x530 fs/f2fs/namei.c:377 >> lookup_open fs/namei.c:3492 [inline] >> open_last_lookups fs/namei.c:3560 [inline] >> path_openat+0x13e7/0x3180 fs/namei.c:3790 >> do_filp_open+0x234/0x490 fs/namei.c:3820 >> do_sys_openat2+0x13e/0x1d0 fs/open.c:1407 >> do_sys_open fs/open.c:1422 [inline] >> __do_sys_open fs/open.c:1430 [inline] >> __se_sys_open fs/open.c:1426 [inline] >> __x64_sys_open+0x225/0x270 fs/open.c:1426 >> do_syscall_x64 arch/x86/entry/common.c:50 [inline] >> do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 >> entry_SYSCALL_64_after_hwframe+0x63/0xcd >> >> -> #0 (&fi->i_xattr_sem){.+.+}-{3:3}: >> check_prev_add kernel/locking/lockdep.c:3142 [inline] >> check_prevs_add kernel/locking/lockdep.c:3261 [inline] >> validate_chain kernel/locking/lockdep.c:3876 [inline] >> __lock_acquire+0x39ff/0x7f70 kernel/locking/lockdep.c:5144 >> lock_acquire+0x1e3/0x520 kernel/locking/lockdep.c:5761 >> down_read+0x47/0x2f0 kernel/locking/rwsem.c:1520 >> f2fs_down_read fs/f2fs/f2fs.h:2108 [inline] >> f2fs_getxattr+0xb8/0x1460 fs/f2fs/xattr.c:532 >> __f2fs_get_acl+0x52/0x8e0 fs/f2fs/acl.c:179 >> f2fs_acl_create fs/f2fs/acl.c:377 [inline] >> f2fs_init_acl+0xd7/0x9a0 fs/f2fs/acl.c:420 >> f2fs_init_inode_metadata+0x824/0x1190 fs/f2fs/dir.c:558 >> f2fs_do_tmpfile+0x34/0x170 fs/f2fs/dir.c:839 >> __f2fs_tmpfile+0x1f9/0x380 fs/f2fs/namei.c:884 >> f2fs_ioc_start_atomic_write+0x4a3/0x9e0 fs/f2fs/file.c:2099 >> __f2fs_ioctl+0x1b5c/0xb770 >> vfs_ioctl fs/ioctl.c:51 [inline] >> __do_sys_ioctl fs/ioctl.c:870 [inline] >> __se_sys_ioctl+0xf8/0x170 fs/ioctl.c:856 >> do_syscall_x64 arch/x86/entry/common.c:50 [inline] >> do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 >> entry_SYSCALL_64_after_hwframe+0x63/0xcd >> >> The root cause is as below reversed lock order: >> - f2fs_create >> - f2fs_add_dentry >> - f2fs_down_write(&F2FS_I(dir)->i_xattr_sem) >> - f2fs_ioc_start_atomic_write >> - __f2fs_tmpfile >> - f2fs_do_tmpfile >> - f2fs_down_write(&F2FS_I(inode)->i_sem) >> - f2fs_init_inode_metadata >> - f2fs_init_acl >> - __f2fs_get_acl >> - f2fs_getxattr >> - f2fs_down_read(&F2FS_I(dir)->i_xattr_sem) >> - f2fs_add_inline_entry >> - f2fs_down_write(&F2FS_I(inode)->i_sem) > > How is it possible to get two inode are identical? Oh, so, it looks like a false positive? BTW, except commit message, what do you think of the change itself? We can using i_sem to instead i_xattr_sem, so that 1) it can shrink inode's size, 2) it's more clear that use common i_sem lock to protect inode's {,x}attr access. Thanks, > >> >> We can break the dependency of deadlock by below change: >> - use i_sem to keep order of {get,set}xattr instead of i_xattr_sem >> - keep below lock order in inode operation to avoid deadlock: >> dir->i_sem -> inode->i_sem >> dir->i_sem -> dpage_lock >> >> Fixes: 5eda1ad1aaff ("f2fs: fix deadlock in i_xattr_sem and inode page lock") >> Reported-by: syzbot+e5600587fa9cbf8e3826@syzkaller.appspotmail.com >> Closes: https://lore.kernel.org/linux-f2fs-devel/00000000000096797d06001a359d@google.com >> Signed-off-by: Chao Yu <chao@kernel.org> >> --- >> fs/f2fs/acl.c | 36 +++++++++++++++++++----------------- >> fs/f2fs/acl.h | 4 ++-- >> fs/f2fs/dir.c | 22 ++++++++++++++++------ >> fs/f2fs/f2fs.h | 1 - >> fs/f2fs/super.c | 5 ++--- >> fs/f2fs/verity.c | 5 +++-- >> fs/f2fs/xattr.c | 37 +++++++++++++++++++++---------------- >> fs/f2fs/xattr.h | 19 +++++++++++++------ >> 8 files changed, 76 insertions(+), 53 deletions(-) >> >> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c >> index ec2aeccb69a3..af58b38e953c 100644 >> --- a/fs/f2fs/acl.c >> +++ b/fs/f2fs/acl.c >> @@ -166,7 +166,7 @@ static void *f2fs_acl_to_disk(struct f2fs_sb_info *sbi, >> } >> >> static struct posix_acl *__f2fs_get_acl(struct inode *inode, int type, >> - struct page *dpage) >> + struct page *dpage, bool locked) >> { >> int name_index = F2FS_XATTR_INDEX_POSIX_ACL_DEFAULT; >> void *value = NULL; >> @@ -176,13 +176,13 @@ static struct posix_acl *__f2fs_get_acl(struct inode *inode, int type, >> if (type == ACL_TYPE_ACCESS) >> name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS; >> >> - retval = f2fs_getxattr(inode, name_index, "", NULL, 0, dpage); >> + retval = f2fs_getxattr(inode, name_index, "", NULL, 0, dpage, locked); >> if (retval > 0) { >> value = f2fs_kmalloc(F2FS_I_SB(inode), retval, GFP_F2FS_ZERO); >> if (!value) >> return ERR_PTR(-ENOMEM); >> retval = f2fs_getxattr(inode, name_index, "", value, >> - retval, dpage); >> + retval, dpage, locked); >> } >> >> if (retval > 0) >> @@ -201,7 +201,7 @@ struct posix_acl *f2fs_get_acl(struct inode *inode, int type, bool rcu) >> if (rcu) >> return ERR_PTR(-ECHILD); >> >> - return __f2fs_get_acl(inode, type, NULL); >> + return __f2fs_get_acl(inode, type, NULL, false); >> } >> >> static int f2fs_acl_update_mode(struct mnt_idmap *idmap, >> @@ -226,9 +226,9 @@ static int f2fs_acl_update_mode(struct mnt_idmap *idmap, >> return 0; >> } >> >> -static int __f2fs_set_acl(struct mnt_idmap *idmap, >> - struct inode *inode, int type, >> - struct posix_acl *acl, struct page *ipage) >> +static int __f2fs_set_acl(struct mnt_idmap *idmap, struct inode *inode, >> + int type, struct posix_acl *acl, >> + struct page *ipage, bool locked) >> { >> int name_index; >> void *value = NULL; >> @@ -266,7 +266,8 @@ static int __f2fs_set_acl(struct mnt_idmap *idmap, >> } >> } >> >> - error = f2fs_setxattr(inode, name_index, "", value, size, ipage, 0); >> + error = f2fs_setxattr(inode, name_index, "", value, size, >> + ipage, 0, locked); >> >> kfree(value); >> if (!error) >> @@ -284,7 +285,7 @@ int f2fs_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, >> if (unlikely(f2fs_cp_error(F2FS_I_SB(inode)))) >> return -EIO; >> >> - return __f2fs_set_acl(idmap, inode, type, acl, NULL); >> + return __f2fs_set_acl(idmap, inode, type, acl, NULL, false); >> } >> >> /* >> @@ -362,7 +363,7 @@ static int f2fs_acl_create_masq(struct posix_acl *acl, umode_t *mode_p) >> >> static int f2fs_acl_create(struct inode *dir, umode_t *mode, >> struct posix_acl **default_acl, struct posix_acl **acl, >> - struct page *dpage) >> + struct page *dpage, bool locked) >> { >> struct posix_acl *p; >> struct posix_acl *clone; >> @@ -374,7 +375,7 @@ static int f2fs_acl_create(struct inode *dir, umode_t *mode, >> if (S_ISLNK(*mode) || !IS_POSIXACL(dir)) >> return 0; >> >> - p = __f2fs_get_acl(dir, ACL_TYPE_DEFAULT, dpage); >> + p = __f2fs_get_acl(dir, ACL_TYPE_DEFAULT, dpage, locked); >> if (!p || p == ERR_PTR(-EOPNOTSUPP)) { >> *mode &= ~current_umask(); >> return 0; >> @@ -412,28 +413,29 @@ static int f2fs_acl_create(struct inode *dir, umode_t *mode, >> } >> >> int f2fs_init_acl(struct inode *inode, struct inode *dir, struct page *ipage, >> - struct page *dpage) >> + struct page *dpage, bool locked) >> { >> struct posix_acl *default_acl = NULL, *acl = NULL; >> int error; >> >> - error = f2fs_acl_create(dir, &inode->i_mode, &default_acl, &acl, dpage); >> + error = f2fs_acl_create(dir, &inode->i_mode, &default_acl, &acl, >> + dpage, locked); >> if (error) >> return error; >> >> f2fs_mark_inode_dirty_sync(inode, true); >> >> if (default_acl) { >> - error = __f2fs_set_acl(NULL, inode, ACL_TYPE_DEFAULT, default_acl, >> - ipage); >> + error = __f2fs_set_acl(NULL, inode, ACL_TYPE_DEFAULT, >> + default_acl, ipage, locked); >> posix_acl_release(default_acl); >> } else { >> inode->i_default_acl = NULL; >> } >> if (acl) { >> if (!error) >> - error = __f2fs_set_acl(NULL, inode, ACL_TYPE_ACCESS, acl, >> - ipage); >> + error = __f2fs_set_acl(NULL, inode, ACL_TYPE_ACCESS, >> + acl, ipage, locked); >> posix_acl_release(acl); >> } else { >> inode->i_acl = NULL; >> diff --git a/fs/f2fs/acl.h b/fs/f2fs/acl.h >> index 94ebfbfbdc6f..9c14b6f549c6 100644 >> --- a/fs/f2fs/acl.h >> +++ b/fs/f2fs/acl.h >> @@ -37,13 +37,13 @@ extern struct posix_acl *f2fs_get_acl(struct inode *, int, bool); >> extern int f2fs_set_acl(struct mnt_idmap *, struct dentry *, >> struct posix_acl *, int); >> extern int f2fs_init_acl(struct inode *, struct inode *, struct page *, >> - struct page *); >> + struct page *, bool); >> #else >> #define f2fs_get_acl NULL >> #define f2fs_set_acl NULL >> >> static inline int f2fs_init_acl(struct inode *inode, struct inode *dir, >> - struct page *ipage, struct page *dpage) >> + struct page *ipage, struct page *dpage, bool locked) >> { >> return 0; >> } >> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c >> index d635c58cf5a3..4b5c62e18d67 100644 >> --- a/fs/f2fs/dir.c >> +++ b/fs/f2fs/dir.c >> @@ -540,6 +540,8 @@ struct page *f2fs_init_inode_metadata(struct inode *inode, struct inode *dir, >> int err; >> >> if (is_inode_flag_set(inode, FI_NEW_INODE)) { >> + struct f2fs_xattr_arg xarg; >> + >> page = f2fs_new_inode_page(inode); >> if (IS_ERR(page)) >> return page; >> @@ -555,12 +557,16 @@ struct page *f2fs_init_inode_metadata(struct inode *inode, struct inode *dir, >> put_page(page); >> } >> >> - err = f2fs_init_acl(inode, dir, page, dpage); >> + err = f2fs_init_acl(inode, dir, page, dpage, true); >> if (err) >> goto put_error; >> >> + xarg.page = page; >> + xarg.locked = true; >> + >> err = f2fs_init_security(inode, dir, >> - fname ? fname->usr_fname : NULL, page); >> + fname ? fname->usr_fname : NULL, >> + &xarg); >> if (err) >> goto put_error; >> >> @@ -775,18 +781,20 @@ int f2fs_add_dentry(struct inode *dir, const struct f2fs_filename *fname, >> { >> int err = -EAGAIN; >> >> + f2fs_down_read(&F2FS_I(dir)->i_sem); >> + >> if (f2fs_has_inline_dentry(dir)) { >> /* >> - * Should get i_xattr_sem to keep the lock order: >> - * i_xattr_sem -> inode_page lock used by f2fs_setxattr. >> + * Should get i_sem to keep the lock order: >> + * i_sem -> inode_page lock used by f2fs_setxattr. >> */ >> - f2fs_down_read(&F2FS_I(dir)->i_xattr_sem); >> err = f2fs_add_inline_entry(dir, fname, inode, ino, mode); >> - f2fs_up_read(&F2FS_I(dir)->i_xattr_sem); >> } >> if (err == -EAGAIN) >> err = f2fs_add_regular_entry(dir, fname, inode, ino, mode); >> >> + f2fs_up_read(&F2FS_I(dir)->i_sem); >> + >> f2fs_update_time(F2FS_I_SB(dir), REQ_TIME); >> return err; >> } >> @@ -835,6 +843,7 @@ int f2fs_do_tmpfile(struct inode *inode, struct inode *dir) >> struct page *page; >> int err = 0; >> >> + f2fs_down_write(&F2FS_I(dir)->i_sem); >> f2fs_down_write(&F2FS_I(inode)->i_sem); >> page = f2fs_init_inode_metadata(inode, dir, NULL, NULL); >> if (IS_ERR(page)) { >> @@ -847,6 +856,7 @@ int f2fs_do_tmpfile(struct inode *inode, struct inode *dir) >> f2fs_update_time(F2FS_I_SB(inode), REQ_TIME); >> fail: >> f2fs_up_write(&F2FS_I(inode)->i_sem); >> + f2fs_up_write(&F2FS_I(dir)->i_sem); >> return err; >> } >> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >> index c7cb2177b252..60ec032be48d 100644 >> --- a/fs/f2fs/f2fs.h >> +++ b/fs/f2fs/f2fs.h >> @@ -838,7 +838,6 @@ struct f2fs_inode_info { >> >> /* avoid racing between foreground op and gc */ >> struct f2fs_rwsem i_gc_rwsem[2]; >> - struct f2fs_rwsem i_xattr_sem; /* avoid racing between reading and changing EAs */ >> >> int i_extra_isize; /* size of extra space located in i_addr */ >> kprojid_t i_projid; /* id for project quota */ >> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >> index ca31163da00a..c72fda24cffd 100644 >> --- a/fs/f2fs/super.c >> +++ b/fs/f2fs/super.c >> @@ -1418,7 +1418,6 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb) >> INIT_LIST_HEAD(&fi->gdirty_list); >> init_f2fs_rwsem(&fi->i_gc_rwsem[READ]); >> init_f2fs_rwsem(&fi->i_gc_rwsem[WRITE]); >> - init_f2fs_rwsem(&fi->i_xattr_sem); >> >> /* Will be used by directory only */ >> fi->i_dir_level = F2FS_SB(sb)->dir_level; >> @@ -3163,7 +3162,7 @@ static int f2fs_get_context(struct inode *inode, void *ctx, size_t len) >> { >> return f2fs_getxattr(inode, F2FS_XATTR_INDEX_ENCRYPTION, >> F2FS_XATTR_NAME_ENCRYPTION_CONTEXT, >> - ctx, len, NULL); >> + ctx, len, NULL, false); >> } >> >> static int f2fs_set_context(struct inode *inode, const void *ctx, size_t len, >> @@ -3183,7 +3182,7 @@ static int f2fs_set_context(struct inode *inode, const void *ctx, size_t len, >> >> return f2fs_setxattr(inode, F2FS_XATTR_INDEX_ENCRYPTION, >> F2FS_XATTR_NAME_ENCRYPTION_CONTEXT, >> - ctx, len, fs_data, XATTR_CREATE); >> + ctx, len, fs_data, XATTR_CREATE, true); >> } >> >> static const union fscrypt_policy *f2fs_get_dummy_policy(struct super_block *sb) >> diff --git a/fs/f2fs/verity.c b/fs/f2fs/verity.c >> index 4fc95f353a7a..e181528b1f26 100644 >> --- a/fs/f2fs/verity.c >> +++ b/fs/f2fs/verity.c >> @@ -181,7 +181,7 @@ static int f2fs_end_enable_verity(struct file *filp, const void *desc, >> /* Set the verity xattr. */ >> err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_VERITY, >> F2FS_XATTR_NAME_VERITY, &dloc, sizeof(dloc), >> - NULL, XATTR_CREATE); >> + NULL, XATTR_CREATE, false); >> if (err) >> goto cleanup; >> >> @@ -226,7 +226,8 @@ static int f2fs_get_verity_descriptor(struct inode *inode, void *buf, >> >> /* Get the descriptor location */ >> res = f2fs_getxattr(inode, F2FS_XATTR_INDEX_VERITY, >> - F2FS_XATTR_NAME_VERITY, &dloc, sizeof(dloc), NULL); >> + F2FS_XATTR_NAME_VERITY, &dloc, sizeof(dloc), >> + NULL, false); >> if (res < 0 && res != -ERANGE) >> return res; >> if (res != sizeof(dloc) || dloc.version != cpu_to_le32(F2FS_VERIFY_VER)) { >> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c >> index 476b186b90a6..a6a611f8a771 100644 >> --- a/fs/f2fs/xattr.c >> +++ b/fs/f2fs/xattr.c >> @@ -61,7 +61,7 @@ static int f2fs_xattr_generic_get(const struct xattr_handler *handler, >> return -EINVAL; >> } >> return f2fs_getxattr(inode, handler->flags, name, >> - buffer, size, NULL); >> + buffer, size, NULL, false); >> } >> >> static int f2fs_xattr_generic_set(const struct xattr_handler *handler, >> @@ -84,7 +84,7 @@ static int f2fs_xattr_generic_set(const struct xattr_handler *handler, >> return -EINVAL; >> } >> return f2fs_setxattr(inode, handler->flags, name, >> - value, size, NULL, flags); >> + value, size, NULL, flags, false); >> } >> >> static bool f2fs_xattr_user_list(struct dentry *dentry) >> @@ -136,15 +136,16 @@ static int f2fs_xattr_advise_set(const struct xattr_handler *handler, >> >> #ifdef CONFIG_F2FS_FS_SECURITY >> static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array, >> - void *page) >> + void *fs_data) >> { >> + struct f2fs_xattr_arg *xarg = (struct f2fs_xattr_arg *)fs_data; >> const struct xattr *xattr; >> int err = 0; >> >> for (xattr = xattr_array; xattr->name != NULL; xattr++) { >> err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY, >> xattr->name, xattr->value, >> - xattr->value_len, (struct page *)page, 0); >> + xattr->value_len, xarg->page, 0, xarg->locked); >> if (err < 0) >> break; >> } >> @@ -152,10 +153,11 @@ static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array, >> } >> >> int f2fs_init_security(struct inode *inode, struct inode *dir, >> - const struct qstr *qstr, struct page *ipage) >> + const struct qstr *qstr, >> + struct f2fs_xattr_arg *xarg) >> { >> return security_inode_init_security(inode, dir, qstr, >> - &f2fs_initxattrs, ipage); >> + &f2fs_initxattrs, xarg); >> } >> #endif >> >> @@ -512,7 +514,8 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize, >> } >> >> int f2fs_getxattr(struct inode *inode, int index, const char *name, >> - void *buffer, size_t buffer_size, struct page *ipage) >> + void *buffer, size_t buffer_size, >> + struct page *ipage, bool locked) >> { >> struct f2fs_xattr_entry *entry = NULL; >> int error; >> @@ -528,12 +531,12 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name, >> if (len > F2FS_NAME_LEN) >> return -ERANGE; >> >> - if (!ipage) >> - f2fs_down_read(&F2FS_I(inode)->i_xattr_sem); >> + if (!locked) >> + f2fs_down_read(&F2FS_I(inode)->i_sem); >> error = lookup_all_xattrs(inode, ipage, index, len, name, >> &entry, &base_addr, &base_size, &is_inline); >> - if (!ipage) >> - f2fs_up_read(&F2FS_I(inode)->i_xattr_sem); >> + if (!locked) >> + f2fs_up_read(&F2FS_I(inode)->i_sem); >> if (error) >> return error; >> >> @@ -567,9 +570,9 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) >> int error; >> size_t rest = buffer_size; >> >> - f2fs_down_read(&F2FS_I(inode)->i_xattr_sem); >> + f2fs_down_read(&F2FS_I(inode)->i_sem); >> error = read_all_xattrs(inode, NULL, &base_addr); >> - f2fs_up_read(&F2FS_I(inode)->i_xattr_sem); >> + f2fs_up_read(&F2FS_I(inode)->i_sem); >> if (error) >> return error; >> >> @@ -775,7 +778,7 @@ static int __f2fs_setxattr(struct inode *inode, int index, >> >> int f2fs_setxattr(struct inode *inode, int index, const char *name, >> const void *value, size_t size, >> - struct page *ipage, int flags) >> + struct page *ipage, int flags, bool locked) >> { >> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >> int err; >> @@ -796,9 +799,11 @@ int f2fs_setxattr(struct inode *inode, int index, const char *name, >> f2fs_balance_fs(sbi, true); >> >> f2fs_lock_op(sbi); >> - f2fs_down_write(&F2FS_I(inode)->i_xattr_sem); >> + if (!locked) >> + f2fs_down_write(&F2FS_I(inode)->i_sem); >> err = __f2fs_setxattr(inode, index, name, value, size, ipage, flags); >> - f2fs_up_write(&F2FS_I(inode)->i_xattr_sem); >> + if (!locked) >> + f2fs_up_write(&F2FS_I(inode)->i_sem); >> f2fs_unlock_op(sbi); >> >> f2fs_update_time(sbi, REQ_TIME); >> diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h >> index b1811c392e6f..170e4a49af31 100644 >> --- a/fs/f2fs/xattr.h >> +++ b/fs/f2fs/xattr.h >> @@ -52,6 +52,11 @@ struct f2fs_xattr_entry { >> char e_name[]; /* attribute name */ >> }; >> >> +struct f2fs_xattr_arg { >> + struct page *page; /* inode page */ >> + bool locked; /* indicate i_sem is locked */ >> +}; >> + >> #define XATTR_HDR(ptr) ((struct f2fs_xattr_header *)(ptr)) >> #define XATTR_ENTRY(ptr) ((struct f2fs_xattr_entry *)(ptr)) >> #define XATTR_FIRST_ENTRY(ptr) (XATTR_ENTRY(XATTR_HDR(ptr) + 1)) >> @@ -128,9 +133,9 @@ extern const struct xattr_handler f2fs_xattr_security_handler; >> extern const struct xattr_handler *f2fs_xattr_handlers[]; >> >> extern int f2fs_setxattr(struct inode *, int, const char *, >> - const void *, size_t, struct page *, int); >> + const void *, size_t, struct page *, int, bool); >> extern int f2fs_getxattr(struct inode *, int, const char *, void *, >> - size_t, struct page *); >> + size_t, struct page *, bool locked); >> extern ssize_t f2fs_listxattr(struct dentry *, char *, size_t); >> extern int f2fs_init_xattr_caches(struct f2fs_sb_info *); >> extern void f2fs_destroy_xattr_caches(struct f2fs_sb_info *); >> @@ -140,13 +145,13 @@ extern void f2fs_destroy_xattr_caches(struct f2fs_sb_info *); >> #define f2fs_listxattr NULL >> static inline int f2fs_setxattr(struct inode *inode, int index, >> const char *name, const void *value, size_t size, >> - struct page *page, int flags) >> + struct page *page, int flags, bool locked) >> { >> return -EOPNOTSUPP; >> } >> static inline int f2fs_getxattr(struct inode *inode, int index, >> const char *name, void *buffer, >> - size_t buffer_size, struct page *dpage) >> + size_t buffer_size, struct page *dpage, bool locked) >> { >> return -EOPNOTSUPP; >> } >> @@ -156,10 +161,12 @@ static inline void f2fs_destroy_xattr_caches(struct f2fs_sb_info *sbi) { } >> >> #ifdef CONFIG_F2FS_FS_SECURITY >> extern int f2fs_init_security(struct inode *, struct inode *, >> - const struct qstr *, struct page *); >> + const struct qstr *, >> + struct f2fs_xattr_arg *xarg); >> #else >> static inline int f2fs_init_security(struct inode *inode, struct inode *dir, >> - const struct qstr *qstr, struct page *ipage) >> + const struct qstr *qstr, >> + struct f2fs_xattr_arg *xarg) >> { >> return 0; >> } >> -- >> 2.40.1
On 07/11, Chao Yu wrote: > On 2023/7/11 1:18, Jaegeuk Kim wrote: > > On 07/10, Chao Yu wrote: > > > syzbot reports deadlock bug as below: > > > > > > -> #1 (&fi->i_sem){+.+.}-{3:3}: > > > down_write+0x3a/0x50 kernel/locking/rwsem.c:1573 > > > f2fs_down_write fs/f2fs/f2fs.h:2133 [inline] > > > f2fs_add_inline_entry+0x3a8/0x760 fs/f2fs/inline.c:644 > > > f2fs_add_dentry+0xba/0x1e0 fs/f2fs/dir.c:784 > > > f2fs_do_add_link+0x21e/0x340 fs/f2fs/dir.c:827 > > > f2fs_add_link fs/f2fs/f2fs.h:3554 [inline] > > > f2fs_create+0x32c/0x530 fs/f2fs/namei.c:377 > > > lookup_open fs/namei.c:3492 [inline] > > > open_last_lookups fs/namei.c:3560 [inline] > > > path_openat+0x13e7/0x3180 fs/namei.c:3790 > > > do_filp_open+0x234/0x490 fs/namei.c:3820 > > > do_sys_openat2+0x13e/0x1d0 fs/open.c:1407 > > > do_sys_open fs/open.c:1422 [inline] > > > __do_sys_open fs/open.c:1430 [inline] > > > __se_sys_open fs/open.c:1426 [inline] > > > __x64_sys_open+0x225/0x270 fs/open.c:1426 > > > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > > > do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 > > > entry_SYSCALL_64_after_hwframe+0x63/0xcd > > > > > > -> #0 (&fi->i_xattr_sem){.+.+}-{3:3}: > > > check_prev_add kernel/locking/lockdep.c:3142 [inline] > > > check_prevs_add kernel/locking/lockdep.c:3261 [inline] > > > validate_chain kernel/locking/lockdep.c:3876 [inline] > > > __lock_acquire+0x39ff/0x7f70 kernel/locking/lockdep.c:5144 > > > lock_acquire+0x1e3/0x520 kernel/locking/lockdep.c:5761 > > > down_read+0x47/0x2f0 kernel/locking/rwsem.c:1520 > > > f2fs_down_read fs/f2fs/f2fs.h:2108 [inline] > > > f2fs_getxattr+0xb8/0x1460 fs/f2fs/xattr.c:532 > > > __f2fs_get_acl+0x52/0x8e0 fs/f2fs/acl.c:179 > > > f2fs_acl_create fs/f2fs/acl.c:377 [inline] > > > f2fs_init_acl+0xd7/0x9a0 fs/f2fs/acl.c:420 > > > f2fs_init_inode_metadata+0x824/0x1190 fs/f2fs/dir.c:558 > > > f2fs_do_tmpfile+0x34/0x170 fs/f2fs/dir.c:839 > > > __f2fs_tmpfile+0x1f9/0x380 fs/f2fs/namei.c:884 > > > f2fs_ioc_start_atomic_write+0x4a3/0x9e0 fs/f2fs/file.c:2099 > > > __f2fs_ioctl+0x1b5c/0xb770 > > > vfs_ioctl fs/ioctl.c:51 [inline] > > > __do_sys_ioctl fs/ioctl.c:870 [inline] > > > __se_sys_ioctl+0xf8/0x170 fs/ioctl.c:856 > > > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > > > do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 > > > entry_SYSCALL_64_after_hwframe+0x63/0xcd > > > > > > The root cause is as below reversed lock order: > > > - f2fs_create > > > - f2fs_add_dentry > > > - f2fs_down_write(&F2FS_I(dir)->i_xattr_sem) > > > - f2fs_ioc_start_atomic_write > > > - __f2fs_tmpfile > > > - f2fs_do_tmpfile > > > - f2fs_down_write(&F2FS_I(inode)->i_sem) > > > - f2fs_init_inode_metadata > > > - f2fs_init_acl > > > - __f2fs_get_acl > > > - f2fs_getxattr > > > - f2fs_down_read(&F2FS_I(dir)->i_xattr_sem) > > > - f2fs_add_inline_entry > > > - f2fs_down_write(&F2FS_I(inode)->i_sem) > > > > How is it possible to get two inode are identical? > > Oh, so, it looks like a false positive? > > BTW, except commit message, what do you think of the change itself? > We can using i_sem to instead i_xattr_sem, so that 1) it can shrink > inode's size, 2) it's more clear that use common i_sem lock to protect > inode's {,x}attr access. If then, xattr can be blocked by unnecessary i_sem lock? > > Thanks, > > > > > > > > > We can break the dependency of deadlock by below change: > > > - use i_sem to keep order of {get,set}xattr instead of i_xattr_sem > > > - keep below lock order in inode operation to avoid deadlock: > > > dir->i_sem -> inode->i_sem > > > dir->i_sem -> dpage_lock > > > > > > Fixes: 5eda1ad1aaff ("f2fs: fix deadlock in i_xattr_sem and inode page lock") > > > Reported-by: syzbot+e5600587fa9cbf8e3826@syzkaller.appspotmail.com > > > Closes: https://lore.kernel.org/linux-f2fs-devel/00000000000096797d06001a359d@google.com > > > Signed-off-by: Chao Yu <chao@kernel.org> > > > --- > > > fs/f2fs/acl.c | 36 +++++++++++++++++++----------------- > > > fs/f2fs/acl.h | 4 ++-- > > > fs/f2fs/dir.c | 22 ++++++++++++++++------ > > > fs/f2fs/f2fs.h | 1 - > > > fs/f2fs/super.c | 5 ++--- > > > fs/f2fs/verity.c | 5 +++-- > > > fs/f2fs/xattr.c | 37 +++++++++++++++++++++---------------- > > > fs/f2fs/xattr.h | 19 +++++++++++++------ > > > 8 files changed, 76 insertions(+), 53 deletions(-) > > > > > > diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c > > > index ec2aeccb69a3..af58b38e953c 100644 > > > --- a/fs/f2fs/acl.c > > > +++ b/fs/f2fs/acl.c > > > @@ -166,7 +166,7 @@ static void *f2fs_acl_to_disk(struct f2fs_sb_info *sbi, > > > } > > > static struct posix_acl *__f2fs_get_acl(struct inode *inode, int type, > > > - struct page *dpage) > > > + struct page *dpage, bool locked) > > > { > > > int name_index = F2FS_XATTR_INDEX_POSIX_ACL_DEFAULT; > > > void *value = NULL; > > > @@ -176,13 +176,13 @@ static struct posix_acl *__f2fs_get_acl(struct inode *inode, int type, > > > if (type == ACL_TYPE_ACCESS) > > > name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS; > > > - retval = f2fs_getxattr(inode, name_index, "", NULL, 0, dpage); > > > + retval = f2fs_getxattr(inode, name_index, "", NULL, 0, dpage, locked); > > > if (retval > 0) { > > > value = f2fs_kmalloc(F2FS_I_SB(inode), retval, GFP_F2FS_ZERO); > > > if (!value) > > > return ERR_PTR(-ENOMEM); > > > retval = f2fs_getxattr(inode, name_index, "", value, > > > - retval, dpage); > > > + retval, dpage, locked); > > > } > > > if (retval > 0) > > > @@ -201,7 +201,7 @@ struct posix_acl *f2fs_get_acl(struct inode *inode, int type, bool rcu) > > > if (rcu) > > > return ERR_PTR(-ECHILD); > > > - return __f2fs_get_acl(inode, type, NULL); > > > + return __f2fs_get_acl(inode, type, NULL, false); > > > } > > > static int f2fs_acl_update_mode(struct mnt_idmap *idmap, > > > @@ -226,9 +226,9 @@ static int f2fs_acl_update_mode(struct mnt_idmap *idmap, > > > return 0; > > > } > > > -static int __f2fs_set_acl(struct mnt_idmap *idmap, > > > - struct inode *inode, int type, > > > - struct posix_acl *acl, struct page *ipage) > > > +static int __f2fs_set_acl(struct mnt_idmap *idmap, struct inode *inode, > > > + int type, struct posix_acl *acl, > > > + struct page *ipage, bool locked) > > > { > > > int name_index; > > > void *value = NULL; > > > @@ -266,7 +266,8 @@ static int __f2fs_set_acl(struct mnt_idmap *idmap, > > > } > > > } > > > - error = f2fs_setxattr(inode, name_index, "", value, size, ipage, 0); > > > + error = f2fs_setxattr(inode, name_index, "", value, size, > > > + ipage, 0, locked); > > > kfree(value); > > > if (!error) > > > @@ -284,7 +285,7 @@ int f2fs_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, > > > if (unlikely(f2fs_cp_error(F2FS_I_SB(inode)))) > > > return -EIO; > > > - return __f2fs_set_acl(idmap, inode, type, acl, NULL); > > > + return __f2fs_set_acl(idmap, inode, type, acl, NULL, false); > > > } > > > /* > > > @@ -362,7 +363,7 @@ static int f2fs_acl_create_masq(struct posix_acl *acl, umode_t *mode_p) > > > static int f2fs_acl_create(struct inode *dir, umode_t *mode, > > > struct posix_acl **default_acl, struct posix_acl **acl, > > > - struct page *dpage) > > > + struct page *dpage, bool locked) > > > { > > > struct posix_acl *p; > > > struct posix_acl *clone; > > > @@ -374,7 +375,7 @@ static int f2fs_acl_create(struct inode *dir, umode_t *mode, > > > if (S_ISLNK(*mode) || !IS_POSIXACL(dir)) > > > return 0; > > > - p = __f2fs_get_acl(dir, ACL_TYPE_DEFAULT, dpage); > > > + p = __f2fs_get_acl(dir, ACL_TYPE_DEFAULT, dpage, locked); > > > if (!p || p == ERR_PTR(-EOPNOTSUPP)) { > > > *mode &= ~current_umask(); > > > return 0; > > > @@ -412,28 +413,29 @@ static int f2fs_acl_create(struct inode *dir, umode_t *mode, > > > } > > > int f2fs_init_acl(struct inode *inode, struct inode *dir, struct page *ipage, > > > - struct page *dpage) > > > + struct page *dpage, bool locked) > > > { > > > struct posix_acl *default_acl = NULL, *acl = NULL; > > > int error; > > > - error = f2fs_acl_create(dir, &inode->i_mode, &default_acl, &acl, dpage); > > > + error = f2fs_acl_create(dir, &inode->i_mode, &default_acl, &acl, > > > + dpage, locked); > > > if (error) > > > return error; > > > f2fs_mark_inode_dirty_sync(inode, true); > > > if (default_acl) { > > > - error = __f2fs_set_acl(NULL, inode, ACL_TYPE_DEFAULT, default_acl, > > > - ipage); > > > + error = __f2fs_set_acl(NULL, inode, ACL_TYPE_DEFAULT, > > > + default_acl, ipage, locked); > > > posix_acl_release(default_acl); > > > } else { > > > inode->i_default_acl = NULL; > > > } > > > if (acl) { > > > if (!error) > > > - error = __f2fs_set_acl(NULL, inode, ACL_TYPE_ACCESS, acl, > > > - ipage); > > > + error = __f2fs_set_acl(NULL, inode, ACL_TYPE_ACCESS, > > > + acl, ipage, locked); > > > posix_acl_release(acl); > > > } else { > > > inode->i_acl = NULL; > > > diff --git a/fs/f2fs/acl.h b/fs/f2fs/acl.h > > > index 94ebfbfbdc6f..9c14b6f549c6 100644 > > > --- a/fs/f2fs/acl.h > > > +++ b/fs/f2fs/acl.h > > > @@ -37,13 +37,13 @@ extern struct posix_acl *f2fs_get_acl(struct inode *, int, bool); > > > extern int f2fs_set_acl(struct mnt_idmap *, struct dentry *, > > > struct posix_acl *, int); > > > extern int f2fs_init_acl(struct inode *, struct inode *, struct page *, > > > - struct page *); > > > + struct page *, bool); > > > #else > > > #define f2fs_get_acl NULL > > > #define f2fs_set_acl NULL > > > static inline int f2fs_init_acl(struct inode *inode, struct inode *dir, > > > - struct page *ipage, struct page *dpage) > > > + struct page *ipage, struct page *dpage, bool locked) > > > { > > > return 0; > > > } > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > > > index d635c58cf5a3..4b5c62e18d67 100644 > > > --- a/fs/f2fs/dir.c > > > +++ b/fs/f2fs/dir.c > > > @@ -540,6 +540,8 @@ struct page *f2fs_init_inode_metadata(struct inode *inode, struct inode *dir, > > > int err; > > > if (is_inode_flag_set(inode, FI_NEW_INODE)) { > > > + struct f2fs_xattr_arg xarg; > > > + > > > page = f2fs_new_inode_page(inode); > > > if (IS_ERR(page)) > > > return page; > > > @@ -555,12 +557,16 @@ struct page *f2fs_init_inode_metadata(struct inode *inode, struct inode *dir, > > > put_page(page); > > > } > > > - err = f2fs_init_acl(inode, dir, page, dpage); > > > + err = f2fs_init_acl(inode, dir, page, dpage, true); > > > if (err) > > > goto put_error; > > > + xarg.page = page; > > > + xarg.locked = true; > > > + > > > err = f2fs_init_security(inode, dir, > > > - fname ? fname->usr_fname : NULL, page); > > > + fname ? fname->usr_fname : NULL, > > > + &xarg); > > > if (err) > > > goto put_error; > > > @@ -775,18 +781,20 @@ int f2fs_add_dentry(struct inode *dir, const struct f2fs_filename *fname, > > > { > > > int err = -EAGAIN; > > > + f2fs_down_read(&F2FS_I(dir)->i_sem); > > > + > > > if (f2fs_has_inline_dentry(dir)) { > > > /* > > > - * Should get i_xattr_sem to keep the lock order: > > > - * i_xattr_sem -> inode_page lock used by f2fs_setxattr. > > > + * Should get i_sem to keep the lock order: > > > + * i_sem -> inode_page lock used by f2fs_setxattr. > > > */ > > > - f2fs_down_read(&F2FS_I(dir)->i_xattr_sem); > > > err = f2fs_add_inline_entry(dir, fname, inode, ino, mode); > > > - f2fs_up_read(&F2FS_I(dir)->i_xattr_sem); > > > } > > > if (err == -EAGAIN) > > > err = f2fs_add_regular_entry(dir, fname, inode, ino, mode); > > > + f2fs_up_read(&F2FS_I(dir)->i_sem); > > > + > > > f2fs_update_time(F2FS_I_SB(dir), REQ_TIME); > > > return err; > > > } > > > @@ -835,6 +843,7 @@ int f2fs_do_tmpfile(struct inode *inode, struct inode *dir) > > > struct page *page; > > > int err = 0; > > > + f2fs_down_write(&F2FS_I(dir)->i_sem); > > > f2fs_down_write(&F2FS_I(inode)->i_sem); > > > page = f2fs_init_inode_metadata(inode, dir, NULL, NULL); > > > if (IS_ERR(page)) { > > > @@ -847,6 +856,7 @@ int f2fs_do_tmpfile(struct inode *inode, struct inode *dir) > > > f2fs_update_time(F2FS_I_SB(inode), REQ_TIME); > > > fail: > > > f2fs_up_write(&F2FS_I(inode)->i_sem); > > > + f2fs_up_write(&F2FS_I(dir)->i_sem); > > > return err; > > > } > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > > index c7cb2177b252..60ec032be48d 100644 > > > --- a/fs/f2fs/f2fs.h > > > +++ b/fs/f2fs/f2fs.h > > > @@ -838,7 +838,6 @@ struct f2fs_inode_info { > > > /* avoid racing between foreground op and gc */ > > > struct f2fs_rwsem i_gc_rwsem[2]; > > > - struct f2fs_rwsem i_xattr_sem; /* avoid racing between reading and changing EAs */ > > > int i_extra_isize; /* size of extra space located in i_addr */ > > > kprojid_t i_projid; /* id for project quota */ > > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > > > index ca31163da00a..c72fda24cffd 100644 > > > --- a/fs/f2fs/super.c > > > +++ b/fs/f2fs/super.c > > > @@ -1418,7 +1418,6 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb) > > > INIT_LIST_HEAD(&fi->gdirty_list); > > > init_f2fs_rwsem(&fi->i_gc_rwsem[READ]); > > > init_f2fs_rwsem(&fi->i_gc_rwsem[WRITE]); > > > - init_f2fs_rwsem(&fi->i_xattr_sem); > > > /* Will be used by directory only */ > > > fi->i_dir_level = F2FS_SB(sb)->dir_level; > > > @@ -3163,7 +3162,7 @@ static int f2fs_get_context(struct inode *inode, void *ctx, size_t len) > > > { > > > return f2fs_getxattr(inode, F2FS_XATTR_INDEX_ENCRYPTION, > > > F2FS_XATTR_NAME_ENCRYPTION_CONTEXT, > > > - ctx, len, NULL); > > > + ctx, len, NULL, false); > > > } > > > static int f2fs_set_context(struct inode *inode, const void *ctx, size_t len, > > > @@ -3183,7 +3182,7 @@ static int f2fs_set_context(struct inode *inode, const void *ctx, size_t len, > > > return f2fs_setxattr(inode, F2FS_XATTR_INDEX_ENCRYPTION, > > > F2FS_XATTR_NAME_ENCRYPTION_CONTEXT, > > > - ctx, len, fs_data, XATTR_CREATE); > > > + ctx, len, fs_data, XATTR_CREATE, true); > > > } > > > static const union fscrypt_policy *f2fs_get_dummy_policy(struct super_block *sb) > > > diff --git a/fs/f2fs/verity.c b/fs/f2fs/verity.c > > > index 4fc95f353a7a..e181528b1f26 100644 > > > --- a/fs/f2fs/verity.c > > > +++ b/fs/f2fs/verity.c > > > @@ -181,7 +181,7 @@ static int f2fs_end_enable_verity(struct file *filp, const void *desc, > > > /* Set the verity xattr. */ > > > err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_VERITY, > > > F2FS_XATTR_NAME_VERITY, &dloc, sizeof(dloc), > > > - NULL, XATTR_CREATE); > > > + NULL, XATTR_CREATE, false); > > > if (err) > > > goto cleanup; > > > @@ -226,7 +226,8 @@ static int f2fs_get_verity_descriptor(struct inode *inode, void *buf, > > > /* Get the descriptor location */ > > > res = f2fs_getxattr(inode, F2FS_XATTR_INDEX_VERITY, > > > - F2FS_XATTR_NAME_VERITY, &dloc, sizeof(dloc), NULL); > > > + F2FS_XATTR_NAME_VERITY, &dloc, sizeof(dloc), > > > + NULL, false); > > > if (res < 0 && res != -ERANGE) > > > return res; > > > if (res != sizeof(dloc) || dloc.version != cpu_to_le32(F2FS_VERIFY_VER)) { > > > diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c > > > index 476b186b90a6..a6a611f8a771 100644 > > > --- a/fs/f2fs/xattr.c > > > +++ b/fs/f2fs/xattr.c > > > @@ -61,7 +61,7 @@ static int f2fs_xattr_generic_get(const struct xattr_handler *handler, > > > return -EINVAL; > > > } > > > return f2fs_getxattr(inode, handler->flags, name, > > > - buffer, size, NULL); > > > + buffer, size, NULL, false); > > > } > > > static int f2fs_xattr_generic_set(const struct xattr_handler *handler, > > > @@ -84,7 +84,7 @@ static int f2fs_xattr_generic_set(const struct xattr_handler *handler, > > > return -EINVAL; > > > } > > > return f2fs_setxattr(inode, handler->flags, name, > > > - value, size, NULL, flags); > > > + value, size, NULL, flags, false); > > > } > > > static bool f2fs_xattr_user_list(struct dentry *dentry) > > > @@ -136,15 +136,16 @@ static int f2fs_xattr_advise_set(const struct xattr_handler *handler, > > > #ifdef CONFIG_F2FS_FS_SECURITY > > > static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array, > > > - void *page) > > > + void *fs_data) > > > { > > > + struct f2fs_xattr_arg *xarg = (struct f2fs_xattr_arg *)fs_data; > > > const struct xattr *xattr; > > > int err = 0; > > > for (xattr = xattr_array; xattr->name != NULL; xattr++) { > > > err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY, > > > xattr->name, xattr->value, > > > - xattr->value_len, (struct page *)page, 0); > > > + xattr->value_len, xarg->page, 0, xarg->locked); > > > if (err < 0) > > > break; > > > } > > > @@ -152,10 +153,11 @@ static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array, > > > } > > > int f2fs_init_security(struct inode *inode, struct inode *dir, > > > - const struct qstr *qstr, struct page *ipage) > > > + const struct qstr *qstr, > > > + struct f2fs_xattr_arg *xarg) > > > { > > > return security_inode_init_security(inode, dir, qstr, > > > - &f2fs_initxattrs, ipage); > > > + &f2fs_initxattrs, xarg); > > > } > > > #endif > > > @@ -512,7 +514,8 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize, > > > } > > > int f2fs_getxattr(struct inode *inode, int index, const char *name, > > > - void *buffer, size_t buffer_size, struct page *ipage) > > > + void *buffer, size_t buffer_size, > > > + struct page *ipage, bool locked) > > > { > > > struct f2fs_xattr_entry *entry = NULL; > > > int error; > > > @@ -528,12 +531,12 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name, > > > if (len > F2FS_NAME_LEN) > > > return -ERANGE; > > > - if (!ipage) > > > - f2fs_down_read(&F2FS_I(inode)->i_xattr_sem); > > > + if (!locked) > > > + f2fs_down_read(&F2FS_I(inode)->i_sem); > > > error = lookup_all_xattrs(inode, ipage, index, len, name, > > > &entry, &base_addr, &base_size, &is_inline); > > > - if (!ipage) > > > - f2fs_up_read(&F2FS_I(inode)->i_xattr_sem); > > > + if (!locked) > > > + f2fs_up_read(&F2FS_I(inode)->i_sem); > > > if (error) > > > return error; > > > @@ -567,9 +570,9 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) > > > int error; > > > size_t rest = buffer_size; > > > - f2fs_down_read(&F2FS_I(inode)->i_xattr_sem); > > > + f2fs_down_read(&F2FS_I(inode)->i_sem); > > > error = read_all_xattrs(inode, NULL, &base_addr); > > > - f2fs_up_read(&F2FS_I(inode)->i_xattr_sem); > > > + f2fs_up_read(&F2FS_I(inode)->i_sem); > > > if (error) > > > return error; > > > @@ -775,7 +778,7 @@ static int __f2fs_setxattr(struct inode *inode, int index, > > > int f2fs_setxattr(struct inode *inode, int index, const char *name, > > > const void *value, size_t size, > > > - struct page *ipage, int flags) > > > + struct page *ipage, int flags, bool locked) > > > { > > > struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > > > int err; > > > @@ -796,9 +799,11 @@ int f2fs_setxattr(struct inode *inode, int index, const char *name, > > > f2fs_balance_fs(sbi, true); > > > f2fs_lock_op(sbi); > > > - f2fs_down_write(&F2FS_I(inode)->i_xattr_sem); > > > + if (!locked) > > > + f2fs_down_write(&F2FS_I(inode)->i_sem); > > > err = __f2fs_setxattr(inode, index, name, value, size, ipage, flags); > > > - f2fs_up_write(&F2FS_I(inode)->i_xattr_sem); > > > + if (!locked) > > > + f2fs_up_write(&F2FS_I(inode)->i_sem); > > > f2fs_unlock_op(sbi); > > > f2fs_update_time(sbi, REQ_TIME); > > > diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h > > > index b1811c392e6f..170e4a49af31 100644 > > > --- a/fs/f2fs/xattr.h > > > +++ b/fs/f2fs/xattr.h > > > @@ -52,6 +52,11 @@ struct f2fs_xattr_entry { > > > char e_name[]; /* attribute name */ > > > }; > > > +struct f2fs_xattr_arg { > > > + struct page *page; /* inode page */ > > > + bool locked; /* indicate i_sem is locked */ > > > +}; > > > + > > > #define XATTR_HDR(ptr) ((struct f2fs_xattr_header *)(ptr)) > > > #define XATTR_ENTRY(ptr) ((struct f2fs_xattr_entry *)(ptr)) > > > #define XATTR_FIRST_ENTRY(ptr) (XATTR_ENTRY(XATTR_HDR(ptr) + 1)) > > > @@ -128,9 +133,9 @@ extern const struct xattr_handler f2fs_xattr_security_handler; > > > extern const struct xattr_handler *f2fs_xattr_handlers[]; > > > extern int f2fs_setxattr(struct inode *, int, const char *, > > > - const void *, size_t, struct page *, int); > > > + const void *, size_t, struct page *, int, bool); > > > extern int f2fs_getxattr(struct inode *, int, const char *, void *, > > > - size_t, struct page *); > > > + size_t, struct page *, bool locked); > > > extern ssize_t f2fs_listxattr(struct dentry *, char *, size_t); > > > extern int f2fs_init_xattr_caches(struct f2fs_sb_info *); > > > extern void f2fs_destroy_xattr_caches(struct f2fs_sb_info *); > > > @@ -140,13 +145,13 @@ extern void f2fs_destroy_xattr_caches(struct f2fs_sb_info *); > > > #define f2fs_listxattr NULL > > > static inline int f2fs_setxattr(struct inode *inode, int index, > > > const char *name, const void *value, size_t size, > > > - struct page *page, int flags) > > > + struct page *page, int flags, bool locked) > > > { > > > return -EOPNOTSUPP; > > > } > > > static inline int f2fs_getxattr(struct inode *inode, int index, > > > const char *name, void *buffer, > > > - size_t buffer_size, struct page *dpage) > > > + size_t buffer_size, struct page *dpage, bool locked) > > > { > > > return -EOPNOTSUPP; > > > } > > > @@ -156,10 +161,12 @@ static inline void f2fs_destroy_xattr_caches(struct f2fs_sb_info *sbi) { } > > > #ifdef CONFIG_F2FS_FS_SECURITY > > > extern int f2fs_init_security(struct inode *, struct inode *, > > > - const struct qstr *, struct page *); > > > + const struct qstr *, > > > + struct f2fs_xattr_arg *xarg); > > > #else > > > static inline int f2fs_init_security(struct inode *inode, struct inode *dir, > > > - const struct qstr *qstr, struct page *ipage) > > > + const struct qstr *qstr, > > > + struct f2fs_xattr_arg *xarg) > > > { > > > return 0; > > > } > > > -- > > > 2.40.1
diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c index ec2aeccb69a3..af58b38e953c 100644 --- a/fs/f2fs/acl.c +++ b/fs/f2fs/acl.c @@ -166,7 +166,7 @@ static void *f2fs_acl_to_disk(struct f2fs_sb_info *sbi, } static struct posix_acl *__f2fs_get_acl(struct inode *inode, int type, - struct page *dpage) + struct page *dpage, bool locked) { int name_index = F2FS_XATTR_INDEX_POSIX_ACL_DEFAULT; void *value = NULL; @@ -176,13 +176,13 @@ static struct posix_acl *__f2fs_get_acl(struct inode *inode, int type, if (type == ACL_TYPE_ACCESS) name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS; - retval = f2fs_getxattr(inode, name_index, "", NULL, 0, dpage); + retval = f2fs_getxattr(inode, name_index, "", NULL, 0, dpage, locked); if (retval > 0) { value = f2fs_kmalloc(F2FS_I_SB(inode), retval, GFP_F2FS_ZERO); if (!value) return ERR_PTR(-ENOMEM); retval = f2fs_getxattr(inode, name_index, "", value, - retval, dpage); + retval, dpage, locked); } if (retval > 0) @@ -201,7 +201,7 @@ struct posix_acl *f2fs_get_acl(struct inode *inode, int type, bool rcu) if (rcu) return ERR_PTR(-ECHILD); - return __f2fs_get_acl(inode, type, NULL); + return __f2fs_get_acl(inode, type, NULL, false); } static int f2fs_acl_update_mode(struct mnt_idmap *idmap, @@ -226,9 +226,9 @@ static int f2fs_acl_update_mode(struct mnt_idmap *idmap, return 0; } -static int __f2fs_set_acl(struct mnt_idmap *idmap, - struct inode *inode, int type, - struct posix_acl *acl, struct page *ipage) +static int __f2fs_set_acl(struct mnt_idmap *idmap, struct inode *inode, + int type, struct posix_acl *acl, + struct page *ipage, bool locked) { int name_index; void *value = NULL; @@ -266,7 +266,8 @@ static int __f2fs_set_acl(struct mnt_idmap *idmap, } } - error = f2fs_setxattr(inode, name_index, "", value, size, ipage, 0); + error = f2fs_setxattr(inode, name_index, "", value, size, + ipage, 0, locked); kfree(value); if (!error) @@ -284,7 +285,7 @@ int f2fs_set_acl(struct mnt_idmap *idmap, struct dentry *dentry, if (unlikely(f2fs_cp_error(F2FS_I_SB(inode)))) return -EIO; - return __f2fs_set_acl(idmap, inode, type, acl, NULL); + return __f2fs_set_acl(idmap, inode, type, acl, NULL, false); } /* @@ -362,7 +363,7 @@ static int f2fs_acl_create_masq(struct posix_acl *acl, umode_t *mode_p) static int f2fs_acl_create(struct inode *dir, umode_t *mode, struct posix_acl **default_acl, struct posix_acl **acl, - struct page *dpage) + struct page *dpage, bool locked) { struct posix_acl *p; struct posix_acl *clone; @@ -374,7 +375,7 @@ static int f2fs_acl_create(struct inode *dir, umode_t *mode, if (S_ISLNK(*mode) || !IS_POSIXACL(dir)) return 0; - p = __f2fs_get_acl(dir, ACL_TYPE_DEFAULT, dpage); + p = __f2fs_get_acl(dir, ACL_TYPE_DEFAULT, dpage, locked); if (!p || p == ERR_PTR(-EOPNOTSUPP)) { *mode &= ~current_umask(); return 0; @@ -412,28 +413,29 @@ static int f2fs_acl_create(struct inode *dir, umode_t *mode, } int f2fs_init_acl(struct inode *inode, struct inode *dir, struct page *ipage, - struct page *dpage) + struct page *dpage, bool locked) { struct posix_acl *default_acl = NULL, *acl = NULL; int error; - error = f2fs_acl_create(dir, &inode->i_mode, &default_acl, &acl, dpage); + error = f2fs_acl_create(dir, &inode->i_mode, &default_acl, &acl, + dpage, locked); if (error) return error; f2fs_mark_inode_dirty_sync(inode, true); if (default_acl) { - error = __f2fs_set_acl(NULL, inode, ACL_TYPE_DEFAULT, default_acl, - ipage); + error = __f2fs_set_acl(NULL, inode, ACL_TYPE_DEFAULT, + default_acl, ipage, locked); posix_acl_release(default_acl); } else { inode->i_default_acl = NULL; } if (acl) { if (!error) - error = __f2fs_set_acl(NULL, inode, ACL_TYPE_ACCESS, acl, - ipage); + error = __f2fs_set_acl(NULL, inode, ACL_TYPE_ACCESS, + acl, ipage, locked); posix_acl_release(acl); } else { inode->i_acl = NULL; diff --git a/fs/f2fs/acl.h b/fs/f2fs/acl.h index 94ebfbfbdc6f..9c14b6f549c6 100644 --- a/fs/f2fs/acl.h +++ b/fs/f2fs/acl.h @@ -37,13 +37,13 @@ extern struct posix_acl *f2fs_get_acl(struct inode *, int, bool); extern int f2fs_set_acl(struct mnt_idmap *, struct dentry *, struct posix_acl *, int); extern int f2fs_init_acl(struct inode *, struct inode *, struct page *, - struct page *); + struct page *, bool); #else #define f2fs_get_acl NULL #define f2fs_set_acl NULL static inline int f2fs_init_acl(struct inode *inode, struct inode *dir, - struct page *ipage, struct page *dpage) + struct page *ipage, struct page *dpage, bool locked) { return 0; } diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c index d635c58cf5a3..4b5c62e18d67 100644 --- a/fs/f2fs/dir.c +++ b/fs/f2fs/dir.c @@ -540,6 +540,8 @@ struct page *f2fs_init_inode_metadata(struct inode *inode, struct inode *dir, int err; if (is_inode_flag_set(inode, FI_NEW_INODE)) { + struct f2fs_xattr_arg xarg; + page = f2fs_new_inode_page(inode); if (IS_ERR(page)) return page; @@ -555,12 +557,16 @@ struct page *f2fs_init_inode_metadata(struct inode *inode, struct inode *dir, put_page(page); } - err = f2fs_init_acl(inode, dir, page, dpage); + err = f2fs_init_acl(inode, dir, page, dpage, true); if (err) goto put_error; + xarg.page = page; + xarg.locked = true; + err = f2fs_init_security(inode, dir, - fname ? fname->usr_fname : NULL, page); + fname ? fname->usr_fname : NULL, + &xarg); if (err) goto put_error; @@ -775,18 +781,20 @@ int f2fs_add_dentry(struct inode *dir, const struct f2fs_filename *fname, { int err = -EAGAIN; + f2fs_down_read(&F2FS_I(dir)->i_sem); + if (f2fs_has_inline_dentry(dir)) { /* - * Should get i_xattr_sem to keep the lock order: - * i_xattr_sem -> inode_page lock used by f2fs_setxattr. + * Should get i_sem to keep the lock order: + * i_sem -> inode_page lock used by f2fs_setxattr. */ - f2fs_down_read(&F2FS_I(dir)->i_xattr_sem); err = f2fs_add_inline_entry(dir, fname, inode, ino, mode); - f2fs_up_read(&F2FS_I(dir)->i_xattr_sem); } if (err == -EAGAIN) err = f2fs_add_regular_entry(dir, fname, inode, ino, mode); + f2fs_up_read(&F2FS_I(dir)->i_sem); + f2fs_update_time(F2FS_I_SB(dir), REQ_TIME); return err; } @@ -835,6 +843,7 @@ int f2fs_do_tmpfile(struct inode *inode, struct inode *dir) struct page *page; int err = 0; + f2fs_down_write(&F2FS_I(dir)->i_sem); f2fs_down_write(&F2FS_I(inode)->i_sem); page = f2fs_init_inode_metadata(inode, dir, NULL, NULL); if (IS_ERR(page)) { @@ -847,6 +856,7 @@ int f2fs_do_tmpfile(struct inode *inode, struct inode *dir) f2fs_update_time(F2FS_I_SB(inode), REQ_TIME); fail: f2fs_up_write(&F2FS_I(inode)->i_sem); + f2fs_up_write(&F2FS_I(dir)->i_sem); return err; } diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index c7cb2177b252..60ec032be48d 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -838,7 +838,6 @@ struct f2fs_inode_info { /* avoid racing between foreground op and gc */ struct f2fs_rwsem i_gc_rwsem[2]; - struct f2fs_rwsem i_xattr_sem; /* avoid racing between reading and changing EAs */ int i_extra_isize; /* size of extra space located in i_addr */ kprojid_t i_projid; /* id for project quota */ diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index ca31163da00a..c72fda24cffd 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -1418,7 +1418,6 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb) INIT_LIST_HEAD(&fi->gdirty_list); init_f2fs_rwsem(&fi->i_gc_rwsem[READ]); init_f2fs_rwsem(&fi->i_gc_rwsem[WRITE]); - init_f2fs_rwsem(&fi->i_xattr_sem); /* Will be used by directory only */ fi->i_dir_level = F2FS_SB(sb)->dir_level; @@ -3163,7 +3162,7 @@ static int f2fs_get_context(struct inode *inode, void *ctx, size_t len) { return f2fs_getxattr(inode, F2FS_XATTR_INDEX_ENCRYPTION, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT, - ctx, len, NULL); + ctx, len, NULL, false); } static int f2fs_set_context(struct inode *inode, const void *ctx, size_t len, @@ -3183,7 +3182,7 @@ static int f2fs_set_context(struct inode *inode, const void *ctx, size_t len, return f2fs_setxattr(inode, F2FS_XATTR_INDEX_ENCRYPTION, F2FS_XATTR_NAME_ENCRYPTION_CONTEXT, - ctx, len, fs_data, XATTR_CREATE); + ctx, len, fs_data, XATTR_CREATE, true); } static const union fscrypt_policy *f2fs_get_dummy_policy(struct super_block *sb) diff --git a/fs/f2fs/verity.c b/fs/f2fs/verity.c index 4fc95f353a7a..e181528b1f26 100644 --- a/fs/f2fs/verity.c +++ b/fs/f2fs/verity.c @@ -181,7 +181,7 @@ static int f2fs_end_enable_verity(struct file *filp, const void *desc, /* Set the verity xattr. */ err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_VERITY, F2FS_XATTR_NAME_VERITY, &dloc, sizeof(dloc), - NULL, XATTR_CREATE); + NULL, XATTR_CREATE, false); if (err) goto cleanup; @@ -226,7 +226,8 @@ static int f2fs_get_verity_descriptor(struct inode *inode, void *buf, /* Get the descriptor location */ res = f2fs_getxattr(inode, F2FS_XATTR_INDEX_VERITY, - F2FS_XATTR_NAME_VERITY, &dloc, sizeof(dloc), NULL); + F2FS_XATTR_NAME_VERITY, &dloc, sizeof(dloc), + NULL, false); if (res < 0 && res != -ERANGE) return res; if (res != sizeof(dloc) || dloc.version != cpu_to_le32(F2FS_VERIFY_VER)) { diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c index 476b186b90a6..a6a611f8a771 100644 --- a/fs/f2fs/xattr.c +++ b/fs/f2fs/xattr.c @@ -61,7 +61,7 @@ static int f2fs_xattr_generic_get(const struct xattr_handler *handler, return -EINVAL; } return f2fs_getxattr(inode, handler->flags, name, - buffer, size, NULL); + buffer, size, NULL, false); } static int f2fs_xattr_generic_set(const struct xattr_handler *handler, @@ -84,7 +84,7 @@ static int f2fs_xattr_generic_set(const struct xattr_handler *handler, return -EINVAL; } return f2fs_setxattr(inode, handler->flags, name, - value, size, NULL, flags); + value, size, NULL, flags, false); } static bool f2fs_xattr_user_list(struct dentry *dentry) @@ -136,15 +136,16 @@ static int f2fs_xattr_advise_set(const struct xattr_handler *handler, #ifdef CONFIG_F2FS_FS_SECURITY static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array, - void *page) + void *fs_data) { + struct f2fs_xattr_arg *xarg = (struct f2fs_xattr_arg *)fs_data; const struct xattr *xattr; int err = 0; for (xattr = xattr_array; xattr->name != NULL; xattr++) { err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY, xattr->name, xattr->value, - xattr->value_len, (struct page *)page, 0); + xattr->value_len, xarg->page, 0, xarg->locked); if (err < 0) break; } @@ -152,10 +153,11 @@ static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array, } int f2fs_init_security(struct inode *inode, struct inode *dir, - const struct qstr *qstr, struct page *ipage) + const struct qstr *qstr, + struct f2fs_xattr_arg *xarg) { return security_inode_init_security(inode, dir, qstr, - &f2fs_initxattrs, ipage); + &f2fs_initxattrs, xarg); } #endif @@ -512,7 +514,8 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize, } int f2fs_getxattr(struct inode *inode, int index, const char *name, - void *buffer, size_t buffer_size, struct page *ipage) + void *buffer, size_t buffer_size, + struct page *ipage, bool locked) { struct f2fs_xattr_entry *entry = NULL; int error; @@ -528,12 +531,12 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name, if (len > F2FS_NAME_LEN) return -ERANGE; - if (!ipage) - f2fs_down_read(&F2FS_I(inode)->i_xattr_sem); + if (!locked) + f2fs_down_read(&F2FS_I(inode)->i_sem); error = lookup_all_xattrs(inode, ipage, index, len, name, &entry, &base_addr, &base_size, &is_inline); - if (!ipage) - f2fs_up_read(&F2FS_I(inode)->i_xattr_sem); + if (!locked) + f2fs_up_read(&F2FS_I(inode)->i_sem); if (error) return error; @@ -567,9 +570,9 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) int error; size_t rest = buffer_size; - f2fs_down_read(&F2FS_I(inode)->i_xattr_sem); + f2fs_down_read(&F2FS_I(inode)->i_sem); error = read_all_xattrs(inode, NULL, &base_addr); - f2fs_up_read(&F2FS_I(inode)->i_xattr_sem); + f2fs_up_read(&F2FS_I(inode)->i_sem); if (error) return error; @@ -775,7 +778,7 @@ static int __f2fs_setxattr(struct inode *inode, int index, int f2fs_setxattr(struct inode *inode, int index, const char *name, const void *value, size_t size, - struct page *ipage, int flags) + struct page *ipage, int flags, bool locked) { struct f2fs_sb_info *sbi = F2FS_I_SB(inode); int err; @@ -796,9 +799,11 @@ int f2fs_setxattr(struct inode *inode, int index, const char *name, f2fs_balance_fs(sbi, true); f2fs_lock_op(sbi); - f2fs_down_write(&F2FS_I(inode)->i_xattr_sem); + if (!locked) + f2fs_down_write(&F2FS_I(inode)->i_sem); err = __f2fs_setxattr(inode, index, name, value, size, ipage, flags); - f2fs_up_write(&F2FS_I(inode)->i_xattr_sem); + if (!locked) + f2fs_up_write(&F2FS_I(inode)->i_sem); f2fs_unlock_op(sbi); f2fs_update_time(sbi, REQ_TIME); diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h index b1811c392e6f..170e4a49af31 100644 --- a/fs/f2fs/xattr.h +++ b/fs/f2fs/xattr.h @@ -52,6 +52,11 @@ struct f2fs_xattr_entry { char e_name[]; /* attribute name */ }; +struct f2fs_xattr_arg { + struct page *page; /* inode page */ + bool locked; /* indicate i_sem is locked */ +}; + #define XATTR_HDR(ptr) ((struct f2fs_xattr_header *)(ptr)) #define XATTR_ENTRY(ptr) ((struct f2fs_xattr_entry *)(ptr)) #define XATTR_FIRST_ENTRY(ptr) (XATTR_ENTRY(XATTR_HDR(ptr) + 1)) @@ -128,9 +133,9 @@ extern const struct xattr_handler f2fs_xattr_security_handler; extern const struct xattr_handler *f2fs_xattr_handlers[]; extern int f2fs_setxattr(struct inode *, int, const char *, - const void *, size_t, struct page *, int); + const void *, size_t, struct page *, int, bool); extern int f2fs_getxattr(struct inode *, int, const char *, void *, - size_t, struct page *); + size_t, struct page *, bool locked); extern ssize_t f2fs_listxattr(struct dentry *, char *, size_t); extern int f2fs_init_xattr_caches(struct f2fs_sb_info *); extern void f2fs_destroy_xattr_caches(struct f2fs_sb_info *); @@ -140,13 +145,13 @@ extern void f2fs_destroy_xattr_caches(struct f2fs_sb_info *); #define f2fs_listxattr NULL static inline int f2fs_setxattr(struct inode *inode, int index, const char *name, const void *value, size_t size, - struct page *page, int flags) + struct page *page, int flags, bool locked) { return -EOPNOTSUPP; } static inline int f2fs_getxattr(struct inode *inode, int index, const char *name, void *buffer, - size_t buffer_size, struct page *dpage) + size_t buffer_size, struct page *dpage, bool locked) { return -EOPNOTSUPP; } @@ -156,10 +161,12 @@ static inline void f2fs_destroy_xattr_caches(struct f2fs_sb_info *sbi) { } #ifdef CONFIG_F2FS_FS_SECURITY extern int f2fs_init_security(struct inode *, struct inode *, - const struct qstr *, struct page *); + const struct qstr *, + struct f2fs_xattr_arg *xarg); #else static inline int f2fs_init_security(struct inode *inode, struct inode *dir, - const struct qstr *qstr, struct page *ipage) + const struct qstr *qstr, + struct f2fs_xattr_arg *xarg) { return 0; }
syzbot reports deadlock bug as below: -> #1 (&fi->i_sem){+.+.}-{3:3}: down_write+0x3a/0x50 kernel/locking/rwsem.c:1573 f2fs_down_write fs/f2fs/f2fs.h:2133 [inline] f2fs_add_inline_entry+0x3a8/0x760 fs/f2fs/inline.c:644 f2fs_add_dentry+0xba/0x1e0 fs/f2fs/dir.c:784 f2fs_do_add_link+0x21e/0x340 fs/f2fs/dir.c:827 f2fs_add_link fs/f2fs/f2fs.h:3554 [inline] f2fs_create+0x32c/0x530 fs/f2fs/namei.c:377 lookup_open fs/namei.c:3492 [inline] open_last_lookups fs/namei.c:3560 [inline] path_openat+0x13e7/0x3180 fs/namei.c:3790 do_filp_open+0x234/0x490 fs/namei.c:3820 do_sys_openat2+0x13e/0x1d0 fs/open.c:1407 do_sys_open fs/open.c:1422 [inline] __do_sys_open fs/open.c:1430 [inline] __se_sys_open fs/open.c:1426 [inline] __x64_sys_open+0x225/0x270 fs/open.c:1426 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x63/0xcd -> #0 (&fi->i_xattr_sem){.+.+}-{3:3}: check_prev_add kernel/locking/lockdep.c:3142 [inline] check_prevs_add kernel/locking/lockdep.c:3261 [inline] validate_chain kernel/locking/lockdep.c:3876 [inline] __lock_acquire+0x39ff/0x7f70 kernel/locking/lockdep.c:5144 lock_acquire+0x1e3/0x520 kernel/locking/lockdep.c:5761 down_read+0x47/0x2f0 kernel/locking/rwsem.c:1520 f2fs_down_read fs/f2fs/f2fs.h:2108 [inline] f2fs_getxattr+0xb8/0x1460 fs/f2fs/xattr.c:532 __f2fs_get_acl+0x52/0x8e0 fs/f2fs/acl.c:179 f2fs_acl_create fs/f2fs/acl.c:377 [inline] f2fs_init_acl+0xd7/0x9a0 fs/f2fs/acl.c:420 f2fs_init_inode_metadata+0x824/0x1190 fs/f2fs/dir.c:558 f2fs_do_tmpfile+0x34/0x170 fs/f2fs/dir.c:839 __f2fs_tmpfile+0x1f9/0x380 fs/f2fs/namei.c:884 f2fs_ioc_start_atomic_write+0x4a3/0x9e0 fs/f2fs/file.c:2099 __f2fs_ioctl+0x1b5c/0xb770 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:870 [inline] __se_sys_ioctl+0xf8/0x170 fs/ioctl.c:856 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x63/0xcd The root cause is as below reversed lock order: - f2fs_create - f2fs_add_dentry - f2fs_down_write(&F2FS_I(dir)->i_xattr_sem) - f2fs_ioc_start_atomic_write - __f2fs_tmpfile - f2fs_do_tmpfile - f2fs_down_write(&F2FS_I(inode)->i_sem) - f2fs_init_inode_metadata - f2fs_init_acl - __f2fs_get_acl - f2fs_getxattr - f2fs_down_read(&F2FS_I(dir)->i_xattr_sem) - f2fs_add_inline_entry - f2fs_down_write(&F2FS_I(inode)->i_sem) We can break the dependency of deadlock by below change: - use i_sem to keep order of {get,set}xattr instead of i_xattr_sem - keep below lock order in inode operation to avoid deadlock: dir->i_sem -> inode->i_sem dir->i_sem -> dpage_lock Fixes: 5eda1ad1aaff ("f2fs: fix deadlock in i_xattr_sem and inode page lock") Reported-by: syzbot+e5600587fa9cbf8e3826@syzkaller.appspotmail.com Closes: https://lore.kernel.org/linux-f2fs-devel/00000000000096797d06001a359d@google.com Signed-off-by: Chao Yu <chao@kernel.org> --- fs/f2fs/acl.c | 36 +++++++++++++++++++----------------- fs/f2fs/acl.h | 4 ++-- fs/f2fs/dir.c | 22 ++++++++++++++++------ fs/f2fs/f2fs.h | 1 - fs/f2fs/super.c | 5 ++--- fs/f2fs/verity.c | 5 +++-- fs/f2fs/xattr.c | 37 +++++++++++++++++++++---------------- fs/f2fs/xattr.h | 19 +++++++++++++------ 8 files changed, 76 insertions(+), 53 deletions(-)