| 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
| ||
|
Date: Tue, 11 Jun 2013 14:48:36 +0900
From: Changman Lee <cm224.lee@...sung.com>
To: Namjae Jeon <linkinjeon@...il.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.
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?
> >
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index deefd25..29cd449 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -352,10 +352,8 @@ int f2fs_setattr(struct dentry *dentry, struct
> > iattr *attr)
> >
> > 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;
> > + if (err || is_inode_flag_set(fi, FI_ACL_MODE))
> > clear_inode_flag(fi, FI_ACL_MODE);
> > - }
> > }
> >
> > Thanks.
> >
> >
> > On 토, 2013-06-08 at 21:25 +0900, Namjae Jeon wrote:
> >> From: Namjae Jeon <namjae.jeon@...sung.com>
> >>
> >> Remove the redundant code from this function and make it aligned with
> >> usages of latest 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().
> >>
> >> Signed-off-by: Namjae Jeon <namjae.jeon@...sung.com>
> >> Signed-off-by: Pankaj Kumar <pankaj.km@...sung.com>
> >> ---
> >> fs/f2fs/acl.c | 5 +----
> >> fs/f2fs/file.c | 47 +++++------------------------------------------
> >> 2 files changed, 6 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> >> index 44abc2f..2d13f44 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) {
> >> @@ -299,7 +296,7 @@ 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);
> >> + umode_t mode = inode->i_mode;
> >>
> >> if (!test_opt(sbi, POSIX_ACL))
> >> return 0;
> >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >> index deefd25..8dfc1da 100644
> >> --- a/fs/f2fs/file.c
> >> +++ b/fs/f2fs/file.c
> >> @@ -300,63 +300,26 @@ 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;
> >> - struct f2fs_inode_info *fi = F2FS_I(inode);
> >> int err;
> >>
> >> err = inode_change_ok(inode, 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);
> >>
> >> - 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;
> >
> >
> >
--
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