| 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: <1380099177.4529.38.camel@kjgkr>
Date: Wed, 25 Sep 2013 17:52:57 +0900
From: Jaegeuk Kim <jaegeuk.kim@...sung.com>
To: Russ Knize <Russ.Knize@...orola.com>
Cc: Russ Knize <rknize@...il.com>,
"linux-f2fs-devel@...ts.sourceforge.net"
<linux-f2fs-devel@...ts.sourceforge.net>,
"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC 1/1] f2fs: don't GC or take an fs_lock from f2fs_initxattrs()
Hi Russ,
That's what I wanted before.
Merged.
Thanks,
2013-09-24 (화), 15:53 -0500, Russ Knize:
> This is an alternate solution to the deadlock problem I mentioned in
> my previous patch.
>
> On Tue, Sep 24, 2013 at 3:49 PM, Russ Knize <rknize@...il.com> wrote:
> > From: Russ Knize <Russ.Knize@...orola.com>
> >
> > f2fs_initxattrs() is called internally from within F2FS and should
> > not call functions that are used by VFS handlers. This avoids
> > certain deadlocks:
> >
> > - vfs_create()
> > - f2fs_create() <-- takes an fs_lock
> > - f2fs_add_link()
> > - __f2fs_add_link()
> > - init_inode_metadata()
> > - f2fs_init_security()
> > - security_inode_init_security()
> > - f2fs_initxattrs()
> > - f2fs_setxattr() <-- also takes an fs_lock
> >
> > If the caller happens to grab the same fs_lock from the pool in both
> > places, they will deadlock. There are also deadlocks involving
> > multiple threads and mutexes:
> >
> > - f2fs_write_begin()
> > - f2fs_balance_fs() <-- takes gc_mutex
> > - f2fs_gc()
> > - write_checkpoint()
> > - block_operations()
> > - mutex_lock_all() <-- blocks trying to grab all fs_locks
> >
> > - f2fs_mkdir() <-- takes an fs_lock
> > - __f2fs_add_link()
> > - f2fs_init_security()
> > - security_inode_init_security()
> > - f2fs_initxattrs()
> > - f2fs_setxattr()
> > - f2fs_balance_fs() <-- blocks trying to take gc_mutex
> >
> > Signed-off-by: Russ Knize <Russ.Knize@...orola.com>
> > ---
> > fs/f2fs/xattr.c | 35 +++++++++++++++++++++++++----------
> > 1 file changed, 25 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> > index 1ac8a5f..3d900ea 100644
> > --- a/fs/f2fs/xattr.c
> > +++ b/fs/f2fs/xattr.c
> > @@ -154,6 +154,9 @@ static int f2fs_xattr_advise_set(struct dentry *dentry, const char *name,
> > }
> >
> > #ifdef CONFIG_F2FS_FS_SECURITY
> > +static int __f2fs_setxattr(struct inode *inode, int name_index,
> > + const char *name, const void *value, size_t value_len,
> > + struct page *ipage);
> > static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
> > void *page)
> > {
> > @@ -161,7 +164,7 @@ static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
> > int err = 0;
> >
> > for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> > - err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
> > + err = __f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
> > xattr->name, xattr->value,
> > xattr->value_len, (struct page *)page);
> > if (err < 0)
> > @@ -469,16 +472,15 @@ cleanup:
> > return error;
> > }
> >
> > -int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> > - const void *value, size_t value_len, struct page *ipage)
> > +static int __f2fs_setxattr(struct inode *inode, int name_index,
> > + const char *name, const void *value, size_t value_len,
> > + struct page *ipage)
> > {
> > - struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> > struct f2fs_inode_info *fi = F2FS_I(inode);
> > struct f2fs_xattr_entry *here, *last;
> > void *base_addr;
> > int found, newsize;
> > size_t name_len;
> > - int ilock;
> > __u32 new_hsize;
> > int error = -ENOMEM;
> >
> > @@ -493,10 +495,6 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> > if (name_len > F2FS_NAME_LEN || value_len > MAX_VALUE_LEN(inode))
> > return -ERANGE;
> >
> > - f2fs_balance_fs(sbi);
> > -
> > - ilock = mutex_lock_op(sbi);
> > -
> > base_addr = read_all_xattrs(inode, ipage);
> > if (!base_addr)
> > goto exit;
> > @@ -578,7 +576,24 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> > else
> > update_inode_page(inode);
> > exit:
> > - mutex_unlock_op(sbi, ilock);
> > kzfree(base_addr);
> > return error;
> > }
> > +
> > +int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> > + const void *value, size_t value_len, struct page *ipage)
> > +{
> > + struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> > + int ilock;
> > + int err;
> > +
> > + f2fs_balance_fs(sbi);
> > +
> > + ilock = mutex_lock_op(sbi);
> > +
> > + err = __f2fs_setxattr(inode, name_index, name, value, value_len, ipage);
> > +
> > + mutex_unlock_op(sbi, ilock);
> > +
> > + return err;
> > +}
> > --
> > 1.7.10.4
> >
> --
> 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/
--
Jaegeuk Kim
Samsung
--
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