| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAKYAXd_44td8BDbnuGR-ChHXjmhTnoVNq4vgujgAyAtbO6vayw@mail.gmail.com>
Date: Fri, 14 Jun 2013 13:20:18 +0900
From: Namjae Jeon <linkinjeon@...il.com>
To: Changman Lee <cm224.lee@...sung.com>
Cc: jaegeuk.kim@...sung.com, Namjae Jeon <namjae.jeon@...sung.com>,
Pankaj Kumar <pankaj.km@...sung.com>,
linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-f2fs-devel@...ts.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH 1/4] f2fs: reorganize the f2fs_setattr() function.
2013/6/11, Namjae Jeon <linkinjeon@...il.com>:
> 2013/6/11, Changman Lee <cm224.lee@...sung.com>:
>> On 화, 2013-06-11 at 07:57 +0900, Namjae Jeon wrote:
>>> 2013/6/10, Changman Lee <cm224.lee@...sung.com>:
>>> > Hello, Namjae
>>> Hi. Changman.
>>> >
>>> > If using ACL, whenever i_mode is changed we should update acl_mode
>>> > which
>>> > is written to xattr block, too. And vice versa.
>>> > Because update_inode() is called at any reason and anytime, so we
>>> > should
>>> > sync both the moment xattr is written.
>>> > We don't hope that only i_mode is written to disk and xattr is not. So
>>> > f2fs_setattr is dirty.
>>> Yes, agreed this could be issue.
>>> >
>>> > And, below code has a bug. When error is occurred, inode->i_mode
>>> > shouldn't be changed. Please, check one more time, Namjae.
>>> And, below code has a bug. When error is occurred, inode->i_mode
>>> shouldn't be changed. Please, check one more time, Namjae.
>>>
>>> This was part of the default code, when ‘acl’ is not set for file’
>>> Then, inode should be updated by these conditions (i.e., it covers the
>>> ‘chmod’ and ‘setacl’ scenario).
>>> When ACL is not present on the file and ‘chmod’ is done, then mode is
>>> changed from this part, as f2fs_get_acl() will fail and cause the
>>> below code to be executed:
>>>
>>> if (err || is_inode_flag_set(fi, FI_ACL_MODE)) {
>>> inode->i_mode = fi->i_acl_mode;
>>> clear_inode_flag(fi, FI_ACL_MODE);
>>> }
>>>
>>> Now, in order to make it consistent and work on all scenario we need
>>> to make further change like this in addition to the patch changes.
>>> setattr_copy(inode, attr);
>>> if (attr->ia_valid & ATTR_MODE) {
>>> + set_acl_inode(fi, inode->i_mode);
>>> err = f2fs_acl_chmod(inode);
>>> if (err || is_inode_flag_set(fi, FI_ACL_MODE)) {
>>>
>>> Let me know your opinion.
>>> Thanks.
>>>
>>
>> setattr_copy changes inode->i_mode, this is not our expectation.
>> So I made redundant __setatt_copy that copy attr->mode to
>> fi->i_acl_mode.
>> When acl_mode is reflected in xattr, acl_mode is copied to
>> inode->i_mode.
>>
>> Agree?
>>
> Hi Changman.
>
> First, Sorry for interrupt.
> I think that inode i_mode should be updated regardless of f2fs_acl_chmod.
> Actually I am still not understand the reason why we should use
> temporarily acl mode(i_acl_mode).
> I wroted the v2 patch to not use i_acl_mode like this.
> Am I missing something ?
To Changman,
I am still waiting for your reply. Correct us if we are wrong or
missing something.
Hi Jaegeuk,
Could you please share your views on this?
Thanks.
>
> ----------------------------------------------------------------------------------------------------------------
> Subject: [PATCH v2] f2fs: reorganize the f2fs_setattr(), f2fs_set_acl,
> f2fs_setxattr()
> From: Namjae Jeon <namjae.jeon@...sung.com>
>
> Remove the redundant code from f2fs_setattr() function and make it aligned
> with usages of generic vfs layer function e.g using the setattr_copy()
> instead of using the f2fs specific function.
>
> Also correct the condition for updating the size of file via
> truncate_setsize().
>
> Also modify the code of f2fs_set_acl and f2fs_setxattr for removing the
> redundant code & add the required changes to correct the requested
> operations.
>
> Remove the variable "i_acl_mode" from the f2fs_inode_info struct since
> i_mode will
> hold the latest 'mode' value which can be used for any further
> references. And in
> order to make 'chmod' work without ACL support, inode i_mode should be
> first
> updated correctly.
>
> Remove the helper functions to access and set the i_acl_mode.
>
> Signed-off-by: Namjae Jeon <namjae.jeon@...sung.com>
> Signed-off-by: Pankaj Kumar <pankaj.km@...sung.com>
> ---
> fs/f2fs/acl.c | 23 +++++++++--------------
> fs/f2fs/f2fs.h | 17 -----------------
> fs/f2fs/file.c | 48 ++++++------------------------------------------
> fs/f2fs/xattr.c | 9 ++-------
> 4 files changed, 17 insertions(+), 80 deletions(-)
>
> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> index 44abc2f..7ebddf1 100644
> --- a/fs/f2fs/acl.c
> +++ b/fs/f2fs/acl.c
> @@ -17,9 +17,6 @@
> #include "xattr.h"
> #include "acl.h"
>
> -#define get_inode_mode(i) ((is_inode_flag_set(F2FS_I(i), FI_ACL_MODE)) ? \
> - (F2FS_I(i)->i_acl_mode) : ((i)->i_mode))
> -
> static inline size_t f2fs_acl_size(int count)
> {
> if (count <= 4) {
> @@ -208,7 +205,6 @@ struct posix_acl *f2fs_get_acl(struct inode
> *inode, int type)
> static int f2fs_set_acl(struct inode *inode, int type, struct posix_acl
> *acl)
> {
> struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> - struct f2fs_inode_info *fi = F2FS_I(inode);
> int name_index;
> void *value = NULL;
> size_t size = 0;
> @@ -226,9 +222,12 @@ static int f2fs_set_acl(struct inode *inode, int
> type, struct posix_acl *acl)
> error = posix_acl_equiv_mode(acl, &inode->i_mode);
> if (error < 0)
> return error;
> - set_acl_inode(fi, inode->i_mode);
> - if (error == 0)
> - acl = NULL;
> + else {
> + inode->i_ctime = CURRENT_TIME;
> + mark_inode_dirty(inode);
> + if (error == 0)
> + acl = NULL;
> + }
> }
> break;
>
> @@ -244,10 +243,8 @@ static int f2fs_set_acl(struct inode *inode, int
> type, struct posix_acl *acl)
>
> if (acl) {
> value = f2fs_acl_to_disk(acl, &size);
> - if (IS_ERR(value)) {
> - cond_clear_inode_flag(fi, FI_ACL_MODE);
> + if (IS_ERR(value))
> return (int)PTR_ERR(value);
> - }
> }
>
> error = f2fs_setxattr(inode, name_index, "", value, size);
> @@ -256,7 +253,6 @@ static int f2fs_set_acl(struct inode *inode, int
> type, struct posix_acl *acl)
> if (!error)
> set_cached_acl(inode, type, acl);
>
> - cond_clear_inode_flag(fi, FI_ACL_MODE);
> return error;
> }
>
> @@ -299,18 +295,17 @@ int f2fs_acl_chmod(struct inode *inode)
> struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> struct posix_acl *acl;
> int error;
> - umode_t mode = get_inode_mode(inode);
>
> if (!test_opt(sbi, POSIX_ACL))
> return 0;
> - if (S_ISLNK(mode))
> + if (S_ISLNK(inode->i_mode))
> return -EOPNOTSUPP;
>
> acl = f2fs_get_acl(inode, ACL_TYPE_ACCESS);
> if (IS_ERR(acl) || !acl)
> return PTR_ERR(acl);
>
> - error = posix_acl_chmod(&acl, GFP_KERNEL, mode);
> + error = posix_acl_chmod(&acl, GFP_KERNEL, inode->i_mode);
> if (error)
> return error;
> error = f2fs_set_acl(inode, ACL_TYPE_ACCESS, acl);
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 40b137a..241ba31 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -159,7 +159,6 @@ struct f2fs_inode_info {
> unsigned char i_advise; /* use to give file attribute hints */
> unsigned int i_current_depth; /* use only in directory structure */
> unsigned int i_pino; /* parent inode number */
> - umode_t i_acl_mode; /* keep file acl mode temporarily */
>
> /* Use below internally in f2fs*/
> unsigned long flags; /* use to pass per-file flags */
> @@ -857,7 +856,6 @@ static inline int f2fs_clear_bit(unsigned int nr,
> char *addr)
> enum {
> FI_NEW_INODE, /* indicate newly allocated inode */
> FI_INC_LINK, /* need to increment i_nlink */
> - FI_ACL_MODE, /* indicate acl mode */
> FI_NO_ALLOC, /* should not allocate any blocks */
> FI_DELAY_IPUT, /* used for the recovery */
> };
> @@ -877,21 +875,6 @@ static inline void clear_inode_flag(struct
> f2fs_inode_info *fi, int flag)
> clear_bit(flag, &fi->flags);
> }
>
> -static inline void set_acl_inode(struct f2fs_inode_info *fi, umode_t mode)
> -{
> - fi->i_acl_mode = mode;
> - set_inode_flag(fi, FI_ACL_MODE);
> -}
> -
> -static inline int cond_clear_inode_flag(struct f2fs_inode_info *fi, int
> flag)
> -{
> - if (is_inode_flag_set(fi, FI_ACL_MODE)) {
> - clear_inode_flag(fi, FI_ACL_MODE);
> - return 1;
> - }
> - return 0;
> -}
> -
> static inline int f2fs_readonly(struct super_block *sb)
> {
> return sb->s_flags & MS_RDONLY;
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index deefd25..4162fbb 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -300,37 +300,6 @@ static int f2fs_getattr(struct vfsmount *mnt,
> return 0;
> }
>
> -#ifdef CONFIG_F2FS_FS_POSIX_ACL
> -static void __setattr_copy(struct inode *inode, const struct iattr *attr)
> -{
> - struct f2fs_inode_info *fi = F2FS_I(inode);
> - unsigned int ia_valid = attr->ia_valid;
> -
> - if (ia_valid & ATTR_UID)
> - inode->i_uid = attr->ia_uid;
> - if (ia_valid & ATTR_GID)
> - inode->i_gid = attr->ia_gid;
> - if (ia_valid & ATTR_ATIME)
> - inode->i_atime = timespec_trunc(attr->ia_atime,
> - inode->i_sb->s_time_gran);
> - if (ia_valid & ATTR_MTIME)
> - inode->i_mtime = timespec_trunc(attr->ia_mtime,
> - inode->i_sb->s_time_gran);
> - if (ia_valid & ATTR_CTIME)
> - inode->i_ctime = timespec_trunc(attr->ia_ctime,
> - inode->i_sb->s_time_gran);
> - if (ia_valid & ATTR_MODE) {
> - umode_t mode = attr->ia_mode;
> -
> - if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
> - mode &= ~S_ISGID;
> - set_acl_inode(fi, mode);
> - }
> -}
> -#else
> -#define __setattr_copy setattr_copy
> -#endif
> -
> int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
> {
> struct inode *inode = dentry->d_inode;
> @@ -341,24 +310,19 @@ int f2fs_setattr(struct dentry *dentry, struct
> iattr *attr)
> if (err)
> return err;
>
> - if ((attr->ia_valid & ATTR_SIZE) &&
> - attr->ia_size != i_size_read(inode)) {
> - truncate_setsize(inode, attr->ia_size);
> + if ((attr->ia_valid & ATTR_SIZE)) {
> + if (attr->ia_size != i_size_read(inode))
> + truncate_setsize(inode, attr->ia_size);
> f2fs_truncate(inode);
> f2fs_balance_fs(F2FS_SB(inode->i_sb));
> }
>
> - __setattr_copy(inode, attr);
> + setattr_copy(inode, attr);
> + mark_inode_dirty(inode);
>
> - if (attr->ia_valid & ATTR_MODE) {
> + if (attr->ia_valid & ATTR_MODE)
> err = f2fs_acl_chmod(inode);
> - if (err || is_inode_flag_set(fi, FI_ACL_MODE)) {
> - inode->i_mode = fi->i_acl_mode;
> - clear_inode_flag(fi, FI_ACL_MODE);
> - }
> - }
>
> - mark_inode_dirty(inode);
> return err;
> }
>
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index 0b02dce..8f02acf 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -333,7 +333,6 @@ int f2fs_setxattr(struct inode *inode, int
> name_index, const char *name,
> goto exit;
> }
> set_new_dnode(&dn, inode, NULL, NULL, fi->i_xattr_nid);
> - mark_inode_dirty(inode);
>
> page = new_node_page(&dn, XATTR_NODE_OFFSET);
> if (IS_ERR(page)) {
> @@ -430,12 +429,8 @@ int f2fs_setxattr(struct inode *inode, int
> name_index, const char *name,
> set_page_dirty(page);
> f2fs_put_page(page, 1);
>
> - if (is_inode_flag_set(fi, FI_ACL_MODE)) {
> - inode->i_mode = fi->i_acl_mode;
> - inode->i_ctime = CURRENT_TIME;
> - clear_inode_flag(fi, FI_ACL_MODE);
> - }
> - update_inode_page(inode);
> + inode->i_ctime = CURRENT_TIME;
> + mark_inode_dirty(inode);
> mutex_unlock_op(sbi, ilock);
>
> return 0;
> --
> 1.7.2.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists