| 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: <c0c198e5-cc91-e2d1-4f54-9d9ae65c4aa3@huawei.com>
Date: Wed, 31 Jul 2019 20:52:01 +0800
From: Gao Xiang <gaoxiang25@...wei.com>
To: Chao Yu <yuchao0@...wei.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
<devel@...verdev.osuosl.org>, <linux-erofs@...ts.ozlabs.org>,
LKML <linux-kernel@...r.kernel.org>, <weidu.du@...wei.com>,
Miao Xie <miaoxie@...wei.com>
Subject: Re: [PATCH 11/22] staging: erofs: kill all failure handling in
fill_super()
Hi Chao,
On 2019/7/31 16:15, Chao Yu wrote:
> On 2019/7/29 14:51, Gao Xiang wrote:
>> .kill_sb() will do that instead in order to remove duplicated code.
>>
>> Note that the initialzation of managed_cache is now moved
>> after s_root is assigned since it's more preferred to iput()
>> in .put_super() and all inodes should be evicted before
>> the end of generic_shutdown_super(sb).
>>
>> Suggested-by: Al Viro <viro@...iv.linux.org.uk>
>> Signed-off-by: Gao Xiang <gaoxiang25@...wei.com>
>> ---
>> drivers/staging/erofs/super.c | 121 +++++++++++++++-------------------
>> 1 file changed, 53 insertions(+), 68 deletions(-)
>>
>> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
>> index bfb6e1e09781..af5d87793e4d 100644
>> --- a/drivers/staging/erofs/super.c
>> +++ b/drivers/staging/erofs/super.c
>> @@ -343,51 +343,52 @@ static const struct address_space_operations managed_cache_aops = {
>> .invalidatepage = managed_cache_invalidatepage,
>> };
>>
>> -static struct inode *erofs_init_managed_cache(struct super_block *sb)
>> +static int erofs_init_managed_cache(struct super_block *sb)
>> {
>> - struct inode *inode = new_inode(sb);
>> + struct erofs_sb_info *const sbi = EROFS_SB(sb);
>> + struct inode *const inode = new_inode(sb);
>>
>> if (unlikely(!inode))
>> - return ERR_PTR(-ENOMEM);
>> + return -ENOMEM;
>>
>> set_nlink(inode, 1);
>> inode->i_size = OFFSET_MAX;
>>
>> inode->i_mapping->a_ops = &managed_cache_aops;
>> mapping_set_gfp_mask(inode->i_mapping,
>> - GFP_NOFS | __GFP_HIGHMEM |
>> - __GFP_MOVABLE | __GFP_NOFAIL);
>> - return inode;
>> + GFP_NOFS | __GFP_HIGHMEM | __GFP_MOVABLE);
Seems so, I will add a new patch addressing on it if needed.
Thanks,
Gao Xiang
>
> It looks above change is not belong to this patch?
>
> Otherwise, it looks good to me.
>
> Reviewed-by: Chao Yu <yuchao0@...wei.com>
>
> Thanks,
>
>> + sbi->managed_cache = inode;
>> + return 0;
>> }
>> -
>> +#else
>> +static int erofs_init_managed_cache(struct super_block *sb) { return 0; }
>> #endif
>>
>> static int erofs_fill_super(struct super_block *sb, void *data, int silent)
>> {
>> struct inode *inode;
>> struct erofs_sb_info *sbi;
>> - int err = -EINVAL;
>> + int err;
>>
>> infoln("fill_super, device -> %s", sb->s_id);
>> infoln("options -> %s", (char *)data);
>>
>> + sb->s_magic = EROFS_SUPER_MAGIC;
>> +
>> if (unlikely(!sb_set_blocksize(sb, EROFS_BLKSIZ))) {
>> errln("failed to set erofs blksize");
>> - goto err;
>> + return -EINVAL;
>> }
>>
>> sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
>> - if (unlikely(!sbi)) {
>> - err = -ENOMEM;
>> - goto err;
>> - }
>> - sb->s_fs_info = sbi;
>> + if (unlikely(!sbi))
>> + return -ENOMEM;
>>
>> + sb->s_fs_info = sbi;
>> err = superblock_read(sb);
>> if (err)
>> - goto err_sbread;
>> + return err;
>>
>> - sb->s_magic = EROFS_SUPER_MAGIC;
>> sb->s_flags |= SB_RDONLY | SB_NOATIME;
>> sb->s_maxbytes = MAX_LFS_FILESIZE;
>> sb->s_time_gran = 1;
>> @@ -397,13 +398,12 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
>> #ifdef CONFIG_EROFS_FS_XATTR
>> sb->s_xattr = erofs_xattr_handlers;
>> #endif
>> -
>> /* set erofs default mount options */
>> default_options(sbi);
>>
>> err = parse_options(sb, data);
>> - if (err)
>> - goto err_parseopt;
>> + if (unlikely(err))
>> + return err;
>>
>> if (!silent)
>> infoln("root inode @ nid %llu", ROOT_NID(sbi));
>> @@ -417,93 +417,78 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
>> INIT_RADIX_TREE(&sbi->workstn_tree, GFP_ATOMIC);
>> #endif
>>
>> -#ifdef EROFS_FS_HAS_MANAGED_CACHE
>> - sbi->managed_cache = erofs_init_managed_cache(sb);
>> - if (IS_ERR(sbi->managed_cache)) {
>> - err = PTR_ERR(sbi->managed_cache);
>> - goto err_init_managed_cache;
>> - }
>> -#endif
>> -
>> /* get the root inode */
>> inode = erofs_iget(sb, ROOT_NID(sbi), true);
>> - if (IS_ERR(inode)) {
>> - err = PTR_ERR(inode);
>> - goto err_iget;
>> - }
>> + if (IS_ERR(inode))
>> + return PTR_ERR(inode);
>>
>> - if (!S_ISDIR(inode->i_mode)) {
>> + if (unlikely(!S_ISDIR(inode->i_mode))) {
>> errln("rootino(nid %llu) is not a directory(i_mode %o)",
>> ROOT_NID(sbi), inode->i_mode);
>> - err = -EINVAL;
>> iput(inode);
>> - goto err_iget;
>> + return -EINVAL;
>> }
>>
>> sb->s_root = d_make_root(inode);
>> - if (!sb->s_root) {
>> - err = -ENOMEM;
>> - goto err_iget;
>> - }
>> + if (unlikely(!sb->s_root))
>> + return -ENOMEM;
>>
>> erofs_shrinker_register(sb);
>> + /* sb->s_umount is already locked, SB_ACTIVE and SB_BORN are not set */
>> + err = erofs_init_managed_cache(sb);
>> + if (unlikely(err))
>> + return err;
>>
>> if (!silent)
>> infoln("mounted on %s with opts: %s.", sb->s_id, (char *)data);
>> return 0;
>> - /*
>> - * please add a label for each exit point and use
>> - * the following name convention, thus new features
>> - * can be integrated easily without renaming labels.
>> - */
>> -err_iget:
>> -#ifdef EROFS_FS_HAS_MANAGED_CACHE
>> - iput(sbi->managed_cache);
>> -err_init_managed_cache:
>> -#endif
>> -err_parseopt:
>> -err_sbread:
>> - sb->s_fs_info = NULL;
>> - kfree(sbi);
>> -err:
>> - return err;
>> +}
>> +
>> +static struct dentry *erofs_mount(struct file_system_type *fs_type, int flags,
>> + const char *dev_name, void *data)
>> +{
>> + return mount_bdev(fs_type, flags, dev_name, data, erofs_fill_super);
>> }
>>
>> /*
>> * could be triggered after deactivate_locked_super()
>> * is called, thus including umount and failed to initialize.
>> */
>> -static void erofs_put_super(struct super_block *sb)
>> +static void erofs_kill_sb(struct super_block *sb)
>> {
>> - struct erofs_sb_info *sbi = EROFS_SB(sb);
>> + struct erofs_sb_info *sbi;
>> +
>> + WARN_ON(sb->s_magic != EROFS_SUPER_MAGIC);
>> + infoln("unmounting for %s", sb->s_id);
>>
>> - /* for cases which are failed in "read_super" */
>> + kill_block_super(sb);
>> +
>> + sbi = EROFS_SB(sb);
>> if (!sbi)
>> return;
>> + kfree(sbi);
>> + sb->s_fs_info = NULL;
>> +}
>>
>> - WARN_ON(sb->s_magic != EROFS_SUPER_MAGIC);
>> +/* called when ->s_root is non-NULL */
>> +static void erofs_put_super(struct super_block *sb)
>> +{
>> + struct erofs_sb_info *const sbi = EROFS_SB(sb);
>>
>> - infoln("unmounted for %s", sb->s_id);
>> + DBG_BUGON(!sbi);
>>
>> erofs_shrinker_unregister(sb);
>> #ifdef EROFS_FS_HAS_MANAGED_CACHE
>> iput(sbi->managed_cache);
>> + sbi->managed_cache = NULL;
>> #endif
>> - kfree(sbi);
>> - sb->s_fs_info = NULL;
>> -}
>> -
>> -static struct dentry *erofs_mount(struct file_system_type *fs_type, int flags,
>> - const char *dev_name, void *data)
>> -{
>> - return mount_bdev(fs_type, flags, dev_name, data, erofs_fill_super);
>> }
>>
>> static struct file_system_type erofs_fs_type = {
>> .owner = THIS_MODULE,
>> .name = "erofs",
>> .mount = erofs_mount,
>> - .kill_sb = kill_block_super,
>> + .kill_sb = erofs_kill_sb,
>> .fs_flags = FS_REQUIRES_DEV,
>> };
>> MODULE_ALIAS_FS("erofs");
>>
Powered by blists - more mailing lists