* [PATCH 0/3] fs/ntfs3: Refactoring of xattr.c
@ 2021-09-24 16:13 Konstantin Komarov
2021-09-24 16:14 ` [PATCH 1/3] fs/ntfs3: Use available posix_acl_release instead of ntfs_posix_acl_release Konstantin Komarov
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Konstantin Komarov @ 2021-09-24 16:13 UTC (permalink / raw)
To: ntfs3; +Cc: linux-kernel, linux-fsdevel
Removed function, that already have been in kernel.
Changed locking policy to fix some potential bugs.
Changed code for readability.
Konstantin Komarov (3):
fs/ntfs3: Use available posix_acl_release instead of
ntfs_posix_acl_release
fs/ntfs3: Remove locked argument in ntfs_set_ea
fs/ntfs3: Refactoring of ntfs_set_ea
fs/ntfs3/xattr.c | 69 ++++++++++++++++++++++--------------------------
1 file changed, 32 insertions(+), 37 deletions(-)
--
2.33.0
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] fs/ntfs3: Use available posix_acl_release instead of ntfs_posix_acl_release 2021-09-24 16:13 [PATCH 0/3] fs/ntfs3: Refactoring of xattr.c Konstantin Komarov @ 2021-09-24 16:14 ` Konstantin Komarov 2021-09-25 8:50 ` Kari Argillander 2021-09-24 16:15 ` [PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea Konstantin Komarov 2021-09-24 16:16 ` [PATCH 3/3] fs/ntfs3: Refactoring of ntfs_set_ea Konstantin Komarov 2 siblings, 1 reply; 12+ messages in thread From: Konstantin Komarov @ 2021-09-24 16:14 UTC (permalink / raw) To: ntfs3; +Cc: linux-kernel, linux-fsdevel We don't need to maintain ntfs_posix_acl_release. Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> --- fs/ntfs3/xattr.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c index 83bbee277e12..253a07d9aa7b 100644 --- a/fs/ntfs3/xattr.c +++ b/fs/ntfs3/xattr.c @@ -475,12 +475,6 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, } #ifdef CONFIG_NTFS3_FS_POSIX_ACL -static inline void ntfs_posix_acl_release(struct posix_acl *acl) -{ - if (acl && refcount_dec_and_test(&acl->a_refcount)) - kfree(acl); -} - static struct posix_acl *ntfs_get_acl_ex(struct user_namespace *mnt_userns, struct inode *inode, int type, int locked) @@ -641,7 +635,7 @@ static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns, return -ENODATA; err = posix_acl_to_xattr(mnt_userns, acl, buffer, size); - ntfs_posix_acl_release(acl); + posix_acl_release(acl); return err; } @@ -678,7 +672,7 @@ static int ntfs_xattr_set_acl(struct user_namespace *mnt_userns, err = ntfs_set_acl(mnt_userns, inode, acl, type); release_and_out: - ntfs_posix_acl_release(acl); + posix_acl_release(acl); return err; } -- 2.33.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] fs/ntfs3: Use available posix_acl_release instead of ntfs_posix_acl_release 2021-09-24 16:14 ` [PATCH 1/3] fs/ntfs3: Use available posix_acl_release instead of ntfs_posix_acl_release Konstantin Komarov @ 2021-09-25 8:50 ` Kari Argillander 0 siblings, 0 replies; 12+ messages in thread From: Kari Argillander @ 2021-09-25 8:50 UTC (permalink / raw) To: Konstantin Komarov; +Cc: ntfs3, linux-kernel, linux-fsdevel On Fri, Sep 24, 2021 at 07:14:53PM +0300, Konstantin Komarov wrote: > We don't need to maintain ntfs_posix_acl_release. > > Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> Reviewed-by: Kari Argillander <kari.argillander@gmail.com> > --- > fs/ntfs3/xattr.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c > index 83bbee277e12..253a07d9aa7b 100644 > --- a/fs/ntfs3/xattr.c > +++ b/fs/ntfs3/xattr.c > @@ -475,12 +475,6 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, > } > > #ifdef CONFIG_NTFS3_FS_POSIX_ACL > -static inline void ntfs_posix_acl_release(struct posix_acl *acl) > -{ > - if (acl && refcount_dec_and_test(&acl->a_refcount)) > - kfree(acl); > -} > - > static struct posix_acl *ntfs_get_acl_ex(struct user_namespace *mnt_userns, > struct inode *inode, int type, > int locked) > @@ -641,7 +635,7 @@ static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns, > return -ENODATA; > > err = posix_acl_to_xattr(mnt_userns, acl, buffer, size); > - ntfs_posix_acl_release(acl); > + posix_acl_release(acl); > > return err; > } > @@ -678,7 +672,7 @@ static int ntfs_xattr_set_acl(struct user_namespace *mnt_userns, > err = ntfs_set_acl(mnt_userns, inode, acl, type); > > release_and_out: > - ntfs_posix_acl_release(acl); > + posix_acl_release(acl); > return err; > } > > -- > 2.33.0 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea 2021-09-24 16:13 [PATCH 0/3] fs/ntfs3: Refactoring of xattr.c Konstantin Komarov 2021-09-24 16:14 ` [PATCH 1/3] fs/ntfs3: Use available posix_acl_release instead of ntfs_posix_acl_release Konstantin Komarov @ 2021-09-24 16:15 ` Konstantin Komarov 2021-09-25 8:49 ` Kari Argillander 2021-09-24 16:16 ` [PATCH 3/3] fs/ntfs3: Refactoring of ntfs_set_ea Konstantin Komarov 2 siblings, 1 reply; 12+ messages in thread From: Konstantin Komarov @ 2021-09-24 16:15 UTC (permalink / raw) To: ntfs3; +Cc: linux-kernel, linux-fsdevel We always need to lock now, because locks became smaller (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode"). Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> --- fs/ntfs3/xattr.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c index 253a07d9aa7b..1ab109723b10 100644 --- a/fs/ntfs3/xattr.c +++ b/fs/ntfs3/xattr.c @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len, static noinline int ntfs_set_ea(struct inode *inode, const char *name, size_t name_len, const void *value, - size_t val_size, int flags, int locked) + size_t val_size, int flags) { struct ntfs_inode *ni = ntfs_i(inode); struct ntfs_sb_info *sbi = ni->mi.sbi; @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, u64 new_sz; void *p; - if (!locked) - ni_lock(ni); + ni_lock(ni); run_init(&ea_run); @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, mark_inode_dirty(&ni->vfs_inode); out: - if (!locked) - ni_unlock(ni); + ni_unlock(ni); run_close(&ea_run); kfree(ea_all); @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type) static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, struct inode *inode, struct posix_acl *acl, - int type, int locked) + int type) { const char *name; size_t size, name_len; @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, flags = 0; } - err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked); + err = ntfs_set_ea(inode, name, name_len, value, size, flags); if (err == -ENODATA && !size) err = 0; /* Removing non existed xattr. */ if (!err) @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode, struct posix_acl *acl, int type) { - return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0); + return ntfs_set_acl_ex(mnt_userns, inode, acl, type); } static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns, @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode, if (default_acl) { err = ntfs_set_acl_ex(mnt_userns, inode, default_acl, - ACL_TYPE_DEFAULT, 1); + ACL_TYPE_DEFAULT); posix_acl_release(default_acl); } else { inode->i_default_acl = NULL; @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode, else { if (!err) err = ntfs_set_acl_ex(mnt_userns, inode, acl, - ACL_TYPE_ACCESS, 1); + ACL_TYPE_ACCESS); posix_acl_release(acl); } @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler, } #endif /* Deal with NTFS extended attribute. */ - err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0); + err = ntfs_set_ea(inode, name, name_len, value, size, flags); out: return err; @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode) value = cpu_to_le32(i_uid_read(inode)); err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value, - sizeof(value), 0, 0); + sizeof(value), 0); if (err) goto out; value = cpu_to_le32(i_gid_read(inode)); err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value, - sizeof(value), 0, 0); + sizeof(value), 0); if (err) goto out; value = cpu_to_le32(inode->i_mode); err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value, - sizeof(value), 0, 0); + sizeof(value), 0); if (err) goto out; if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) { value = cpu_to_le32(inode->i_rdev); err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value, - sizeof(value), 0, 0); + sizeof(value), 0); if (err) goto out; } -- 2.33.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea 2021-09-24 16:15 ` [PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea Konstantin Komarov @ 2021-09-25 8:49 ` Kari Argillander 2021-09-27 15:10 ` Konstantin Komarov 0 siblings, 1 reply; 12+ messages in thread From: Kari Argillander @ 2021-09-25 8:49 UTC (permalink / raw) To: Konstantin Komarov; +Cc: ntfs3, linux-kernel, linux-fsdevel On Fri, Sep 24, 2021 at 07:15:50PM +0300, Konstantin Komarov wrote: > We always need to lock now, because locks became smaller > (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode"). So basically this actually fixes that commit? Fixes: d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode") Or if you do not use fixes atleast use d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode") You can add these to your gitconfig [core] abbrev = 12 [pretty] fixes = Fixes: %h (\"%s\") fixed = Fixes: %h (\"%s\") And get this annotation with git show --pretty=fixes <sha> Have some comments below also. > > Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> > --- > fs/ntfs3/xattr.c | 28 +++++++++++++--------------- > 1 file changed, 13 insertions(+), 15 deletions(-) > > diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c > index 253a07d9aa7b..1ab109723b10 100644 > --- a/fs/ntfs3/xattr.c > +++ b/fs/ntfs3/xattr.c > @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len, > > static noinline int ntfs_set_ea(struct inode *inode, const char *name, > size_t name_len, const void *value, > - size_t val_size, int flags, int locked) > + size_t val_size, int flags) > { > struct ntfs_inode *ni = ntfs_i(inode); > struct ntfs_sb_info *sbi = ni->mi.sbi; > @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, > u64 new_sz; > void *p; > > - if (!locked) > - ni_lock(ni); > + ni_lock(ni); > > run_init(&ea_run); > > @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, > mark_inode_dirty(&ni->vfs_inode); > > out: > - if (!locked) > - ni_unlock(ni); > + ni_unlock(ni); > > run_close(&ea_run); > kfree(ea_all); > @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type) > > static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, > struct inode *inode, struct posix_acl *acl, > - int type, int locked) > + int type) > { > const char *name; > size_t size, name_len; > @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, > flags = 0; > } > > - err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked); > + err = ntfs_set_ea(inode, name, name_len, value, size, flags); > if (err == -ENODATA && !size) > err = 0; /* Removing non existed xattr. */ > if (!err) > @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, > int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode, > struct posix_acl *acl, int type) > { > - return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0); > + return ntfs_set_acl_ex(mnt_userns, inode, acl, type); > } > > static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns, > @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode, > > if (default_acl) { > err = ntfs_set_acl_ex(mnt_userns, inode, default_acl, > - ACL_TYPE_DEFAULT, 1); > + ACL_TYPE_DEFAULT); > posix_acl_release(default_acl); > } else { > inode->i_default_acl = NULL; > @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode, > else { > if (!err) > err = ntfs_set_acl_ex(mnt_userns, inode, acl, > - ACL_TYPE_ACCESS, 1); > + ACL_TYPE_ACCESS); > posix_acl_release(acl); > } > > @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler, > } > #endif > /* Deal with NTFS extended attribute. */ > - err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0); > + err = ntfs_set_ea(inode, name, name_len, value, size, flags); > > out: > return err; > @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode) > > value = cpu_to_le32(i_uid_read(inode)); > err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value, > - sizeof(value), 0, 0); > + sizeof(value), 0); > if (err) > goto out; > > value = cpu_to_le32(i_gid_read(inode)); > err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value, > - sizeof(value), 0, 0); > + sizeof(value), 0); > if (err) > goto out; > > value = cpu_to_le32(inode->i_mode); > err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value, > - sizeof(value), 0, 0); > + sizeof(value), 0); > if (err) > goto out; > > if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) { > value = cpu_to_le32(inode->i_rdev); > err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value, > - sizeof(value), 0, 0); > + sizeof(value), 0); Is this really that we can lock/unlock same lock 4 times in a row in a ntfs_set_ea? This does not feel correct. Argillander > if (err) > goto out; > } > -- > 2.33.0 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea 2021-09-25 8:49 ` Kari Argillander @ 2021-09-27 15:10 ` Konstantin Komarov 2021-09-27 19:10 ` Kari Argillander 0 siblings, 1 reply; 12+ messages in thread From: Konstantin Komarov @ 2021-09-27 15:10 UTC (permalink / raw) To: Kari Argillander; +Cc: ntfs3, linux-kernel, linux-fsdevel On 25.09.2021 11:49, Kari Argillander wrote: > On Fri, Sep 24, 2021 at 07:15:50PM +0300, Konstantin Komarov wrote: >> We always need to lock now, because locks became smaller >> (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode"). > > So basically this actually fixes that commit? > > Fixes: d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode") > > Or if you do not use fixes atleast use > > d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode") > > You can add these to your gitconfig > > [core] > abbrev = 12 > [pretty] > fixes = Fixes: %h (\"%s\") > fixed = Fixes: %h (\"%s\") > > And get this annotation with > git show --pretty=fixes <sha> > > Have some comments below also. > >> >> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> >> --- >> fs/ntfs3/xattr.c | 28 +++++++++++++--------------- >> 1 file changed, 13 insertions(+), 15 deletions(-) >> >> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c >> index 253a07d9aa7b..1ab109723b10 100644 >> --- a/fs/ntfs3/xattr.c >> +++ b/fs/ntfs3/xattr.c >> @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len, >> >> static noinline int ntfs_set_ea(struct inode *inode, const char *name, >> size_t name_len, const void *value, >> - size_t val_size, int flags, int locked) >> + size_t val_size, int flags) >> { >> struct ntfs_inode *ni = ntfs_i(inode); >> struct ntfs_sb_info *sbi = ni->mi.sbi; >> @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, >> u64 new_sz; >> void *p; >> >> - if (!locked) >> - ni_lock(ni); >> + ni_lock(ni); >> >> run_init(&ea_run); >> >> @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, >> mark_inode_dirty(&ni->vfs_inode); >> >> out: >> - if (!locked) >> - ni_unlock(ni); >> + ni_unlock(ni); >> >> run_close(&ea_run); >> kfree(ea_all); >> @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type) >> >> static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, >> struct inode *inode, struct posix_acl *acl, >> - int type, int locked) >> + int type) >> { >> const char *name; >> size_t size, name_len; >> @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, >> flags = 0; >> } >> >> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked); >> + err = ntfs_set_ea(inode, name, name_len, value, size, flags); >> if (err == -ENODATA && !size) >> err = 0; /* Removing non existed xattr. */ >> if (!err) >> @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, >> int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode, >> struct posix_acl *acl, int type) >> { >> - return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0); >> + return ntfs_set_acl_ex(mnt_userns, inode, acl, type); >> } >> >> static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns, >> @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode, >> >> if (default_acl) { >> err = ntfs_set_acl_ex(mnt_userns, inode, default_acl, >> - ACL_TYPE_DEFAULT, 1); >> + ACL_TYPE_DEFAULT); >> posix_acl_release(default_acl); >> } else { >> inode->i_default_acl = NULL; >> @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode, >> else { >> if (!err) >> err = ntfs_set_acl_ex(mnt_userns, inode, acl, >> - ACL_TYPE_ACCESS, 1); >> + ACL_TYPE_ACCESS); >> posix_acl_release(acl); >> } >> >> @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler, >> } >> #endif >> /* Deal with NTFS extended attribute. */ >> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0); >> + err = ntfs_set_ea(inode, name, name_len, value, size, flags); >> >> out: >> return err; >> @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode) >> >> value = cpu_to_le32(i_uid_read(inode)); >> err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value, >> - sizeof(value), 0, 0); >> + sizeof(value), 0); >> if (err) >> goto out; >> >> value = cpu_to_le32(i_gid_read(inode)); >> err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value, >> - sizeof(value), 0, 0); >> + sizeof(value), 0); >> if (err) >> goto out; >> >> value = cpu_to_le32(inode->i_mode); >> err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value, >> - sizeof(value), 0, 0); >> + sizeof(value), 0); >> if (err) >> goto out; >> >> if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) { >> value = cpu_to_le32(inode->i_rdev); >> err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value, >> - sizeof(value), 0, 0); >> + sizeof(value), 0); > > Is this really that we can lock/unlock same lock 4 times in a row in a > ntfs_set_ea? This does not feel correct. > > Argillander > How it was working before d562e901f25d "fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode": ntfs_create (lock mutex) => ntfs_create_inode => ntfs_save_wsl_perm (we are under lock here) => return to ntfs_create and unlock How it works with d562e901f25d: ntfs_create => ntfs_create_inode (lock in line 1201 file fs/ntfs3/inode.c and unlock in line 1557) => ntfs_save_wsl_perm (we aren't under lock here in line 1605) So we need to lock 4 times because there are 4 ntfs_set_ea calls. But now there can be done more work between those calls in other threads, locks became more granular. >> if (err) >> goto out; >> } >> -- >> 2.33.0 >> >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea 2021-09-27 15:10 ` Konstantin Komarov @ 2021-09-27 19:10 ` Kari Argillander 2021-09-28 18:01 ` Konstantin Komarov 0 siblings, 1 reply; 12+ messages in thread From: Kari Argillander @ 2021-09-27 19:10 UTC (permalink / raw) To: Konstantin Komarov; +Cc: ntfs3, linux-kernel, linux-fsdevel On Mon, Sep 27, 2021 at 06:10:00PM +0300, Konstantin Komarov wrote: > > > On 25.09.2021 11:49, Kari Argillander wrote: > > On Fri, Sep 24, 2021 at 07:15:50PM +0300, Konstantin Komarov wrote: > >> We always need to lock now, because locks became smaller > >> (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode"). > > > > So basically this actually fixes that commit? > > > > Fixes: d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode") > > > > Or if you do not use fixes atleast use > > > > d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode") > > > > You can add these to your gitconfig > > > > [core] > > abbrev = 12 > > [pretty] > > fixes = Fixes: %h (\"%s\") > > fixed = Fixes: %h (\"%s\") > > > > And get this annotation with > > git show --pretty=fixes <sha> > > > > Have some comments below also. > > > >> > >> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> > >> --- > >> fs/ntfs3/xattr.c | 28 +++++++++++++--------------- > >> 1 file changed, 13 insertions(+), 15 deletions(-) > >> > >> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c > >> index 253a07d9aa7b..1ab109723b10 100644 > >> --- a/fs/ntfs3/xattr.c > >> +++ b/fs/ntfs3/xattr.c > >> @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len, > >> > >> static noinline int ntfs_set_ea(struct inode *inode, const char *name, > >> size_t name_len, const void *value, > >> - size_t val_size, int flags, int locked) Maybe we should leave int locked and ... > >> + size_t val_size, int flags) > >> { > >> struct ntfs_inode *ni = ntfs_i(inode); > >> struct ntfs_sb_info *sbi = ni->mi.sbi; > >> @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, > >> u64 new_sz; > >> void *p; > >> > >> - if (!locked) > >> - ni_lock(ni); > >> + ni_lock(ni); > >> > >> run_init(&ea_run); > >> > >> @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, > >> mark_inode_dirty(&ni->vfs_inode); > >> > >> out: > >> - if (!locked) > >> - ni_unlock(ni); > >> + ni_unlock(ni); > >> > >> run_close(&ea_run); > >> kfree(ea_all); > >> @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type) > >> > >> static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, > >> struct inode *inode, struct posix_acl *acl, > >> - int type, int locked) > >> + int type) > >> { > >> const char *name; > >> size_t size, name_len; > >> @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, > >> flags = 0; > >> } > >> > >> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked); > >> + err = ntfs_set_ea(inode, name, name_len, value, size, flags); > >> if (err == -ENODATA && !size) > >> err = 0; /* Removing non existed xattr. */ > >> if (!err) > >> @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, > >> int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode, > >> struct posix_acl *acl, int type) > >> { > >> - return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0); > >> + return ntfs_set_acl_ex(mnt_userns, inode, acl, type); > >> } > >> > >> static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns, > >> @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode, > >> > >> if (default_acl) { > >> err = ntfs_set_acl_ex(mnt_userns, inode, default_acl, > >> - ACL_TYPE_DEFAULT, 1); > >> + ACL_TYPE_DEFAULT); > >> posix_acl_release(default_acl); > >> } else { > >> inode->i_default_acl = NULL; > >> @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode, > >> else { > >> if (!err) > >> err = ntfs_set_acl_ex(mnt_userns, inode, acl, > >> - ACL_TYPE_ACCESS, 1); > >> + ACL_TYPE_ACCESS); > >> posix_acl_release(acl); > >> } > >> > >> @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler, > >> } > >> #endif > >> /* Deal with NTFS extended attribute. */ > >> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0); > >> + err = ntfs_set_ea(inode, name, name_len, value, size, flags); > >> > >> out: > >> return err; > >> @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode) > >> do lock here and ... > >> value = cpu_to_le32(i_uid_read(inode)); > >> err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value, > >> - sizeof(value), 0, 0); > >> + sizeof(value), 0); > >> if (err) > >> goto out; > >> > >> value = cpu_to_le32(i_gid_read(inode)); > >> err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value, > >> - sizeof(value), 0, 0); > >> + sizeof(value), 0); > >> if (err) > >> goto out; > >> > >> value = cpu_to_le32(inode->i_mode); > >> err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value, > >> - sizeof(value), 0, 0); > >> + sizeof(value), 0); > >> if (err) > >> goto out; > >> > >> if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) { > >> value = cpu_to_le32(inode->i_rdev); > >> err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value, > >> - sizeof(value), 0, 0); > >> + sizeof(value), 0); unlock here. Of course unlock also in error path. > > > > Is this really that we can lock/unlock same lock 4 times in a row in a > > ntfs_set_ea? This does not feel correct. > > > > Argillander > > > > How it was working before d562e901f25d > "fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode": > > ntfs_create (lock mutex) => > ntfs_create_inode => > ntfs_save_wsl_perm (we are under lock here) => > return to ntfs_create and unlock > > How it works with d562e901f25d: > > ntfs_create => > ntfs_create_inode (lock in line 1201 file fs/ntfs3/inode.c > and unlock in line 1557) => > ntfs_save_wsl_perm (we aren't under lock here in line 1605) > > So we need to lock 4 times because there are 4 ntfs_set_ea calls. > But now there can be done more work between those calls > in other threads, locks became more granular. Yeah but locking and locking 4 times when we can do it just ones is quite waste. Please consider my suggestion above or tell what is wrong with it. Argillander > > >> if (err) > >> goto out; > >> } > >> -- > >> 2.33.0 > >> > >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea 2021-09-27 19:10 ` Kari Argillander @ 2021-09-28 18:01 ` Konstantin Komarov 2021-09-28 18:13 ` Kari Argillander 0 siblings, 1 reply; 12+ messages in thread From: Konstantin Komarov @ 2021-09-28 18:01 UTC (permalink / raw) To: Kari Argillander; +Cc: ntfs3, linux-kernel, linux-fsdevel On 27.09.2021 22:10, Kari Argillander wrote: > On Mon, Sep 27, 2021 at 06:10:00PM +0300, Konstantin Komarov wrote: >> >> >> On 25.09.2021 11:49, Kari Argillander wrote: >>> On Fri, Sep 24, 2021 at 07:15:50PM +0300, Konstantin Komarov wrote: >>>> We always need to lock now, because locks became smaller >>>> (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode"). >>> >>> So basically this actually fixes that commit? >>> >>> Fixes: d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode") >>> >>> Or if you do not use fixes atleast use >>> >>> d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode") >>> >>> You can add these to your gitconfig >>> >>> [core] >>> abbrev = 12 >>> [pretty] >>> fixes = Fixes: %h (\"%s\") >>> fixed = Fixes: %h (\"%s\") >>> >>> And get this annotation with >>> git show --pretty=fixes <sha> >>> >>> Have some comments below also. >>> >>>> >>>> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> >>>> --- >>>> fs/ntfs3/xattr.c | 28 +++++++++++++--------------- >>>> 1 file changed, 13 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c >>>> index 253a07d9aa7b..1ab109723b10 100644 >>>> --- a/fs/ntfs3/xattr.c >>>> +++ b/fs/ntfs3/xattr.c >>>> @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len, >>>> >>>> static noinline int ntfs_set_ea(struct inode *inode, const char *name, >>>> size_t name_len, const void *value, >>>> - size_t val_size, int flags, int locked) > > Maybe we should leave int locked and ... > >>>> + size_t val_size, int flags) >>>> { >>>> struct ntfs_inode *ni = ntfs_i(inode); >>>> struct ntfs_sb_info *sbi = ni->mi.sbi; >>>> @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, >>>> u64 new_sz; >>>> void *p; >>>> >>>> - if (!locked) >>>> - ni_lock(ni); >>>> + ni_lock(ni); >>>> >>>> run_init(&ea_run); >>>> >>>> @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, >>>> mark_inode_dirty(&ni->vfs_inode); >>>> >>>> out: >>>> - if (!locked) >>>> - ni_unlock(ni); >>>> + ni_unlock(ni); >>>> >>>> run_close(&ea_run); >>>> kfree(ea_all); >>>> @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type) >>>> >>>> static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, >>>> struct inode *inode, struct posix_acl *acl, >>>> - int type, int locked) >>>> + int type) >>>> { >>>> const char *name; >>>> size_t size, name_len; >>>> @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, >>>> flags = 0; >>>> } >>>> >>>> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked); >>>> + err = ntfs_set_ea(inode, name, name_len, value, size, flags); >>>> if (err == -ENODATA && !size) >>>> err = 0; /* Removing non existed xattr. */ >>>> if (!err) >>>> @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, >>>> int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode, >>>> struct posix_acl *acl, int type) >>>> { >>>> - return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0); >>>> + return ntfs_set_acl_ex(mnt_userns, inode, acl, type); >>>> } >>>> >>>> static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns, >>>> @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode, >>>> >>>> if (default_acl) { >>>> err = ntfs_set_acl_ex(mnt_userns, inode, default_acl, >>>> - ACL_TYPE_DEFAULT, 1); >>>> + ACL_TYPE_DEFAULT); >>>> posix_acl_release(default_acl); >>>> } else { >>>> inode->i_default_acl = NULL; >>>> @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode, >>>> else { >>>> if (!err) >>>> err = ntfs_set_acl_ex(mnt_userns, inode, acl, >>>> - ACL_TYPE_ACCESS, 1); >>>> + ACL_TYPE_ACCESS); >>>> posix_acl_release(acl); >>>> } >>>> >>>> @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler, >>>> } >>>> #endif >>>> /* Deal with NTFS extended attribute. */ >>>> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0); >>>> + err = ntfs_set_ea(inode, name, name_len, value, size, flags); >>>> >>>> out: >>>> return err; >>>> @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode) >>>> > > do lock here and ... > >>>> value = cpu_to_le32(i_uid_read(inode)); >>>> err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value, >>>> - sizeof(value), 0, 0); >>>> + sizeof(value), 0); >>>> if (err) >>>> goto out; >>>> >>>> value = cpu_to_le32(i_gid_read(inode)); >>>> err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value, >>>> - sizeof(value), 0, 0); >>>> + sizeof(value), 0); >>>> if (err) >>>> goto out; >>>> >>>> value = cpu_to_le32(inode->i_mode); >>>> err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value, >>>> - sizeof(value), 0, 0); >>>> + sizeof(value), 0); >>>> if (err) >>>> goto out; >>>> >>>> if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) { >>>> value = cpu_to_le32(inode->i_rdev); >>>> err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value, >>>> - sizeof(value), 0, 0); >>>> + sizeof(value), 0); > > unlock here. Of course unlock also in error path. > >>> >>> Is this really that we can lock/unlock same lock 4 times in a row in a >>> ntfs_set_ea? This does not feel correct. >>> >>> Argillander >>> >> >> How it was working before d562e901f25d >> "fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode": >> >> ntfs_create (lock mutex) => >> ntfs_create_inode => >> ntfs_save_wsl_perm (we are under lock here) => >> return to ntfs_create and unlock >> >> How it works with d562e901f25d: >> >> ntfs_create => >> ntfs_create_inode (lock in line 1201 file fs/ntfs3/inode.c >> and unlock in line 1557) => >> ntfs_save_wsl_perm (we aren't under lock here in line 1605) >> >> So we need to lock 4 times because there are 4 ntfs_set_ea calls. >> But now there can be done more work between those calls >> in other threads, locks became more granular. > > Yeah but locking and locking 4 times when we can do it just ones is > quite waste. Please consider my suggestion above or tell what is wrong > with it. > > Argillander > If I've understood correctly, you want to lock once in start of function ntfs_save_wsl_perm and unlock at the end of it. It takes care of locks for those 4 calls to ntfs_set_ea. But there are still other calls to ntfs_set_ea, that aren't protected in this case: function ntfs_set_acl_ex (line 603 file fs/ntfs3/xattr.c) err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked); This function called there: - function ntfs_set_acl (line 621 file fs/ntfs3/xattr.c) return ntfs_set_acl_ex(mnt_userns, inode, acl, type); - function ntfs_init_acl (line 701 file fs/ntfs3/xattr.c) err = ntfs_set_acl_ex(mnt_userns, inode, default_acl, - function ntfs_init_acl (line 712 file fs/ntfs3/xattr.c) err = ntfs_set_acl_ex(mnt_userns, inode, acl, So there are many entry points to ntfs_set_ea (and can be added more). Of course we can let int locked remain for these situations, but in my opinion it makes code a lot less readable (that was the reason to start looking into locking). I'm not sure, that winning some lock/unlocking in one specific scenario is worth it. >> >>>> if (err) >>>> goto out; >>>> } >>>> -- >>>> 2.33.0 >>>> >>>> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea 2021-09-28 18:01 ` Konstantin Komarov @ 2021-09-28 18:13 ` Kari Argillander 0 siblings, 0 replies; 12+ messages in thread From: Kari Argillander @ 2021-09-28 18:13 UTC (permalink / raw) To: Konstantin Komarov; +Cc: ntfs3, linux-kernel, linux-fsdevel On Tue, Sep 28, 2021 at 09:01:38PM +0300, Konstantin Komarov wrote: > > > On 27.09.2021 22:10, Kari Argillander wrote: > > On Mon, Sep 27, 2021 at 06:10:00PM +0300, Konstantin Komarov wrote: > >> > >> > >> On 25.09.2021 11:49, Kari Argillander wrote: > >>> On Fri, Sep 24, 2021 at 07:15:50PM +0300, Konstantin Komarov wrote: > >>>> We always need to lock now, because locks became smaller > >>>> (see "Move ni_lock_dir and ni_unlock into ntfs_create_inode"). > >>> > >>> So basically this actually fixes that commit? > >>> > >>> Fixes: d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode") > >>> > >>> Or if you do not use fixes atleast use > >>> > >>> d562e901f25d ("fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode") > >>> > >>> You can add these to your gitconfig > >>> > >>> [core] > >>> abbrev = 12 > >>> [pretty] > >>> fixes = Fixes: %h (\"%s\") > >>> fixed = Fixes: %h (\"%s\") > >>> > >>> And get this annotation with > >>> git show --pretty=fixes <sha> > >>> > >>> Have some comments below also. > >>> > >>>> > >>>> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> > >>>> --- > >>>> fs/ntfs3/xattr.c | 28 +++++++++++++--------------- > >>>> 1 file changed, 13 insertions(+), 15 deletions(-) > >>>> > >>>> diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c > >>>> index 253a07d9aa7b..1ab109723b10 100644 > >>>> --- a/fs/ntfs3/xattr.c > >>>> +++ b/fs/ntfs3/xattr.c > >>>> @@ -257,7 +257,7 @@ static int ntfs_get_ea(struct inode *inode, const char *name, size_t name_len, > >>>> > >>>> static noinline int ntfs_set_ea(struct inode *inode, const char *name, > >>>> size_t name_len, const void *value, > >>>> - size_t val_size, int flags, int locked) > > > > Maybe we should leave int locked and ... > > > >>>> + size_t val_size, int flags) > >>>> { > >>>> struct ntfs_inode *ni = ntfs_i(inode); > >>>> struct ntfs_sb_info *sbi = ni->mi.sbi; > >>>> @@ -276,8 +276,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, > >>>> u64 new_sz; > >>>> void *p; > >>>> > >>>> - if (!locked) > >>>> - ni_lock(ni); > >>>> + ni_lock(ni); > >>>> > >>>> run_init(&ea_run); > >>>> > >>>> @@ -465,8 +464,7 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, > >>>> mark_inode_dirty(&ni->vfs_inode); > >>>> > >>>> out: > >>>> - if (!locked) > >>>> - ni_unlock(ni); > >>>> + ni_unlock(ni); > >>>> > >>>> run_close(&ea_run); > >>>> kfree(ea_all); > >>>> @@ -537,7 +535,7 @@ struct posix_acl *ntfs_get_acl(struct inode *inode, int type) > >>>> > >>>> static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, > >>>> struct inode *inode, struct posix_acl *acl, > >>>> - int type, int locked) > >>>> + int type) > >>>> { > >>>> const char *name; > >>>> size_t size, name_len; > >>>> @@ -594,7 +592,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, > >>>> flags = 0; > >>>> } > >>>> > >>>> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked); > >>>> + err = ntfs_set_ea(inode, name, name_len, value, size, flags); > >>>> if (err == -ENODATA && !size) > >>>> err = 0; /* Removing non existed xattr. */ > >>>> if (!err) > >>>> @@ -612,7 +610,7 @@ static noinline int ntfs_set_acl_ex(struct user_namespace *mnt_userns, > >>>> int ntfs_set_acl(struct user_namespace *mnt_userns, struct inode *inode, > >>>> struct posix_acl *acl, int type) > >>>> { > >>>> - return ntfs_set_acl_ex(mnt_userns, inode, acl, type, 0); > >>>> + return ntfs_set_acl_ex(mnt_userns, inode, acl, type); > >>>> } > >>>> > >>>> static int ntfs_xattr_get_acl(struct user_namespace *mnt_userns, > >>>> @@ -693,7 +691,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode, > >>>> > >>>> if (default_acl) { > >>>> err = ntfs_set_acl_ex(mnt_userns, inode, default_acl, > >>>> - ACL_TYPE_DEFAULT, 1); > >>>> + ACL_TYPE_DEFAULT); > >>>> posix_acl_release(default_acl); > >>>> } else { > >>>> inode->i_default_acl = NULL; > >>>> @@ -704,7 +702,7 @@ int ntfs_init_acl(struct user_namespace *mnt_userns, struct inode *inode, > >>>> else { > >>>> if (!err) > >>>> err = ntfs_set_acl_ex(mnt_userns, inode, acl, > >>>> - ACL_TYPE_ACCESS, 1); > >>>> + ACL_TYPE_ACCESS); > >>>> posix_acl_release(acl); > >>>> } > >>>> > >>>> @@ -988,7 +986,7 @@ static noinline int ntfs_setxattr(const struct xattr_handler *handler, > >>>> } > >>>> #endif > >>>> /* Deal with NTFS extended attribute. */ > >>>> - err = ntfs_set_ea(inode, name, name_len, value, size, flags, 0); > >>>> + err = ntfs_set_ea(inode, name, name_len, value, size, flags); > >>>> > >>>> out: > >>>> return err; > >>>> @@ -1006,26 +1004,26 @@ int ntfs_save_wsl_perm(struct inode *inode) > >>>> > > > > do lock here and ... > > > >>>> value = cpu_to_le32(i_uid_read(inode)); > >>>> err = ntfs_set_ea(inode, "$LXUID", sizeof("$LXUID") - 1, &value, > >>>> - sizeof(value), 0, 0); > >>>> + sizeof(value), 0); > >>>> if (err) > >>>> goto out; > >>>> > >>>> value = cpu_to_le32(i_gid_read(inode)); > >>>> err = ntfs_set_ea(inode, "$LXGID", sizeof("$LXGID") - 1, &value, > >>>> - sizeof(value), 0, 0); > >>>> + sizeof(value), 0); > >>>> if (err) > >>>> goto out; > >>>> > >>>> value = cpu_to_le32(inode->i_mode); > >>>> err = ntfs_set_ea(inode, "$LXMOD", sizeof("$LXMOD") - 1, &value, > >>>> - sizeof(value), 0, 0); > >>>> + sizeof(value), 0); > >>>> if (err) > >>>> goto out; > >>>> > >>>> if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) { > >>>> value = cpu_to_le32(inode->i_rdev); > >>>> err = ntfs_set_ea(inode, "$LXDEV", sizeof("$LXDEV") - 1, &value, > >>>> - sizeof(value), 0, 0); > >>>> + sizeof(value), 0); > > > > unlock here. Of course unlock also in error path. > > > >>> > >>> Is this really that we can lock/unlock same lock 4 times in a row in a > >>> ntfs_set_ea? This does not feel correct. > >>> > >>> Argillander > >>> > >> > >> How it was working before d562e901f25d > >> "fs/ntfs3: Move ni_lock_dir and ni_unlock into ntfs_create_inode": > >> > >> ntfs_create (lock mutex) => > >> ntfs_create_inode => > >> ntfs_save_wsl_perm (we are under lock here) => > >> return to ntfs_create and unlock > >> > >> How it works with d562e901f25d: > >> > >> ntfs_create => > >> ntfs_create_inode (lock in line 1201 file fs/ntfs3/inode.c > >> and unlock in line 1557) => > >> ntfs_save_wsl_perm (we aren't under lock here in line 1605) > >> > >> So we need to lock 4 times because there are 4 ntfs_set_ea calls. > >> But now there can be done more work between those calls > >> in other threads, locks became more granular. > > > > Yeah but locking and locking 4 times when we can do it just ones is > > quite waste. Please consider my suggestion above or tell what is wrong > > with it. > > > > Argillander > > > > If I've understood correctly, you want to lock once in start of function > ntfs_save_wsl_perm and unlock at the end of it. > It takes care of locks for those 4 calls to ntfs_set_ea. > > But there are still other calls to ntfs_set_ea, that aren't protected > in this case: > function ntfs_set_acl_ex (line 603 file fs/ntfs3/xattr.c) > err = ntfs_set_ea(inode, name, name_len, value, size, flags, locked); > > This function called there: > - function ntfs_set_acl (line 621 file fs/ntfs3/xattr.c) > return ntfs_set_acl_ex(mnt_userns, inode, acl, type); > - function ntfs_init_acl (line 701 file fs/ntfs3/xattr.c) > err = ntfs_set_acl_ex(mnt_userns, inode, default_acl, > - function ntfs_init_acl (line 712 file fs/ntfs3/xattr.c) > err = ntfs_set_acl_ex(mnt_userns, inode, acl, > > So there are many entry points to ntfs_set_ea (and can be added more). > Of course we can let int locked remain for these situations, > but in my opinion it makes code a lot less readable > (that was the reason to start looking into locking). Yeah this what I was suggesting. And I totally agree that it is less readable. > I'm not sure, that winning some lock/unlocking > in one specific scenario is worth it. Still 4 locking / 4 unlocking in row is painfull. One option is to lock outside of ntfs_set_acl if that is ok to you. If not maybe atleast add todo comment to ntfs_set_wsl_perm that locking needs to be rethinked. I know that there is lot of work to do with locks in here so maybe it is not right time to nit pick about them. > > >> > >>>> if (err) > >>>> goto out; > >>>> } > >>>> -- > >>>> 2.33.0 > >>>> > >>>> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] fs/ntfs3: Refactoring of ntfs_set_ea 2021-09-24 16:13 [PATCH 0/3] fs/ntfs3: Refactoring of xattr.c Konstantin Komarov 2021-09-24 16:14 ` [PATCH 1/3] fs/ntfs3: Use available posix_acl_release instead of ntfs_posix_acl_release Konstantin Komarov 2021-09-24 16:15 ` [PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea Konstantin Komarov @ 2021-09-24 16:16 ` Konstantin Komarov 2021-09-24 16:31 ` Joe Perches 2 siblings, 1 reply; 12+ messages in thread From: Konstantin Komarov @ 2021-09-24 16:16 UTC (permalink / raw) To: ntfs3; +Cc: linux-kernel, linux-fsdevel Make code more readable. Don't try to read zero bytes. Add warning when size of exteneded attribute exceeds limit. Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com> --- fs/ntfs3/xattr.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c index 1ab109723b10..395e71291e28 100644 --- a/fs/ntfs3/xattr.c +++ b/fs/ntfs3/xattr.c @@ -75,6 +75,7 @@ static int ntfs_read_ea(struct ntfs_inode *ni, struct EA_FULL **ea, size_t add_bytes, const struct EA_INFO **info) { int err; + struct ntfs_sb_info *sbi = ni->mi.sbi; struct ATTR_LIST_ENTRY *le = NULL; struct ATTRIB *attr_info, *attr_ea; void *ea_p; @@ -99,10 +100,10 @@ static int ntfs_read_ea(struct ntfs_inode *ni, struct EA_FULL **ea, /* Check Ea limit. */ size = le32_to_cpu((*info)->size); - if (size > ni->mi.sbi->ea_max_size) + if (size > sbi->ea_max_size) return -EFBIG; - if (attr_size(attr_ea) > ni->mi.sbi->ea_max_size) + if (attr_size(attr_ea) > sbi->ea_max_size) return -EFBIG; /* Allocate memory for packed Ea. */ @@ -110,15 +111,16 @@ static int ntfs_read_ea(struct ntfs_inode *ni, struct EA_FULL **ea, if (!ea_p) return -ENOMEM; - if (attr_ea->non_res) { + if (!size) { + ; + } else if (attr_ea->non_res) { struct runs_tree run; run_init(&run); err = attr_load_runs(attr_ea, ni, &run, NULL); if (!err) - err = ntfs_read_run_nb(ni->mi.sbi, &run, 0, ea_p, size, - NULL); + err = ntfs_read_run_nb(sbi, &run, 0, ea_p, size, NULL); run_close(&run); if (err) @@ -366,21 +368,22 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, new_ea->name[name_len] = 0; memcpy(new_ea->name + name_len + 1, value, val_size); new_pack = le16_to_cpu(ea_info.size_pack) + packed_ea_size(new_ea); - - /* Should fit into 16 bits. */ - if (new_pack > 0xffff) { - err = -EFBIG; // -EINVAL? - goto out; - } ea_info.size_pack = cpu_to_le16(new_pack); - /* New size of ATTR_EA. */ size += add; - if (size > sbi->ea_max_size) { + ea_info.size = cpu_to_le32(size); + + /* + * 1. Check ea_info.size_pack for overflow. + * 2. New attibute size must fit value from $AttrDef + */ + if (new_pack > 0xffff || size > sbi->ea_max_size) { + ntfs_inode_warn( + inode, + "The size of exteneded attributes must not exceed 64K"); err = -EFBIG; // -EINVAL? goto out; } - ea_info.size = cpu_to_le32(size); update_ea: -- 2.33.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] fs/ntfs3: Refactoring of ntfs_set_ea 2021-09-24 16:16 ` [PATCH 3/3] fs/ntfs3: Refactoring of ntfs_set_ea Konstantin Komarov @ 2021-09-24 16:31 ` Joe Perches 0 siblings, 0 replies; 12+ messages in thread From: Joe Perches @ 2021-09-24 16:31 UTC (permalink / raw) To: Konstantin Komarov, ntfs3; +Cc: linux-kernel, linux-fsdevel On Fri, 2021-09-24 at 19:16 +0300, Konstantin Komarov wrote: > Make code more readable. > Don't try to read zero bytes. > Add warning when size of exteneded attribute exceeds limit. [] > diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c [] > @@ -366,21 +368,22 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, [] > + ea_info.size = cpu_to_le32(size); > + > + /* > + * 1. Check ea_info.size_pack for overflow. > + * 2. New attibute size must fit value from $AttrDef > + */ > + if (new_pack > 0xffff || size > sbi->ea_max_size) { > + ntfs_inode_warn( > + inode, > + "The size of exteneded attributes must not exceed 64K"); trivial typo of extended. Pedants might suggest KiB. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] fs/ntfs3: Refactoring of ntfs_set_ea @ 2021-09-24 16:31 ` Joe Perches 0 siblings, 0 replies; 12+ messages in thread From: Joe Perches @ 2021-09-24 16:31 UTC (permalink / raw) To: Konstantin Komarov, ntfs3; +Cc: linux-kernel, linux-fsdevel On Fri, 2021-09-24 at 19:16 +0300, Konstantin Komarov wrote: > Make code more readable. > Don't try to read zero bytes. > Add warning when size of exteneded attribute exceeds limit. [] > diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c [] > @@ -366,21 +368,22 @@ static noinline int ntfs_set_ea(struct inode *inode, const char *name, [] > + ea_info.size = cpu_to_le32(size); > + > + /* > + * 1. Check ea_info.size_pack for overflow. > + * 2. New attibute size must fit value from $AttrDef > + */ > + if (new_pack > 0xffff || size > sbi->ea_max_size) { > + ntfs_inode_warn( > + inode, > + "The size of exteneded attributes must not exceed 64K"); trivial typo of extended. Pedants might suggest KiB. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-09-28 18:14 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-24 16:13 [PATCH 0/3] fs/ntfs3: Refactoring of xattr.c Konstantin Komarov 2021-09-24 16:14 ` [PATCH 1/3] fs/ntfs3: Use available posix_acl_release instead of ntfs_posix_acl_release Konstantin Komarov 2021-09-25 8:50 ` Kari Argillander 2021-09-24 16:15 ` [PATCH 2/3] fs/ntfs3: Remove locked argument in ntfs_set_ea Konstantin Komarov 2021-09-25 8:49 ` Kari Argillander 2021-09-27 15:10 ` Konstantin Komarov 2021-09-27 19:10 ` Kari Argillander 2021-09-28 18:01 ` Konstantin Komarov 2021-09-28 18:13 ` Kari Argillander 2021-09-24 16:16 ` [PATCH 3/3] fs/ntfs3: Refactoring of ntfs_set_ea Konstantin Komarov 2021-09-24 16:31 ` Joe Perches 2021-09-24 16:31 ` Joe Perches
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.