linux-kernel - Re: [RFC 1/1] f2fs: don't GC or take an fs_lock from f2fs

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGfvh62J=rkvAw_nxEt1CSE9EhDQqnow+waQJ_gNt1YmkrD9wg@mail.gmail.com>
Date:	Tue, 24 Sep 2013 15:53:56 -0500
From:	Russ Knize <Russ.Knize@...orola.com>
To:	Russ Knize <rknize@...il.com>
Cc:	"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()

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/