From patchwork Tue May 8 01:48:04 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Ernesto_A=2E_Fern=C3=A1ndez?= X-Patchwork-Id: 10385047 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id D733160236 for ; Tue, 8 May 2018 01:48:17 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 691182621E for ; Tue, 8 May 2018 01:48:14 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 5DBB02624A; Tue, 8 May 2018 01:48:14 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9C45B2621E for ; Tue, 8 May 2018 01:48:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753586AbeEHBsM (ORCPT ); Mon, 7 May 2018 21:48:12 -0400 Received: from mail-qt0-f193.google.com ([209.85.216.193]:39214 "EHLO mail-qt0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752835AbeEHBsL (ORCPT ); Mon, 7 May 2018 21:48:11 -0400 Received: by mail-qt0-f193.google.com with SMTP id f1-v6so39055914qtj.6 for ; Mon, 07 May 2018 18:48:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=YoW20G+En3TgVppz7/r4EMy0Na5ocwubVPNR6H+vAhw=; b=XDt30dX5RfMqoGhoovbWehHIheIanU9z/ekW0LTxBxzvn0SfE7WETl6AWasfFQ3maT Hgl+cr0P30wizGeJGhkaJQbj7DlVuHqwDRqlyay18CivCaattSF7WcPFh62Rkp2cEfDw YGOn3iVztRg9r1C47ETjrYc0xEJoUXfZ/Ddnfj0HnImhg5wIBpv9FlEgGFv0NEcPLKRd yQjl3wRyoJtCYAvIpAqj4tkmfkIOMLOJI3DboJXf3SQwit+uN2y84dCwXGUVGYARRNQ6 p42xqRXTjK9GWxLXijMQnMUDFZuW0hiN4wxN3Jlih6OIJaH9UyjPHYjeAgzxlUAX+F9i c2Hg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=YoW20G+En3TgVppz7/r4EMy0Na5ocwubVPNR6H+vAhw=; b=ViPy12ksYIE3xW+hAnceTRwXDx7tV+aKBART6x6u94Eyz+C8CKfnJse0yIC/W5GhVi S52UuFPnfxKFCexvrn41FHvryPKYqSg8/C7Z7W2R2BK2pg7vLOFebyjL8bzeXUxGpAMz x1Jkjmmr8xoDJxLRSJ0WAaz5CjyG7KyS0hRa4Nt6XohQ4HJStbAsWFYWSczv1AD0G+M6 1eEQl039TRTJ2KkQvhLkaOFpi2kos0XB12Vs+d7ygSKxZkxHHv27JZkqrsoLGX+ayaOQ FK1sjn6NDMqq7dJuIdVBmG2RkRo3Il9dAoxW6ZGYHt2pekCqts+5priZTGFL4WkCRxvG 56xQ== X-Gm-Message-State: ALQs6tDz4qu0ClI+wedos8hy/p73yd2QdXcX5C8vztDhdgtEGFF+zpB9 wTZmWz7yGcKa353VQF0K/90kVw== X-Google-Smtp-Source: AB8JxZrr/kR8Ej9HKx0HPH7fiFbHoen6iYclONikV4u/hjeFd2wRKmVokgTxOGF07sYHhRXzgTgRZQ== X-Received: by 2002:a0c:e609:: with SMTP id z9-v6mr36222939qvm.158.1525744090317; Mon, 07 May 2018 18:48:10 -0700 (PDT) Received: from eaf ([181.47.179.0]) by smtp.gmail.com with ESMTPSA id y188sm8034803qka.19.2018.05.07.18.48.07 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 07 May 2018 18:48:09 -0700 (PDT) Date: Mon, 7 May 2018 22:48:04 -0300 From: Ernesto =?utf-8?Q?A=2E_Fern=C3=A1ndez?= To: Al Viro Cc: syzbot , Andrew Morton , Christoph Hellwig , Alexey Khoroshilov , Artem Bityutskiy , linux-fsdevel@vger.kernel.org, Ernesto =?utf-8?Q?A=2E_Fern=C3=A1ndez?= Subject: Re: [PATCH 1/2] hfsplus: clean up delayed work if fill_super fails Message-ID: <20180508014803.w7yqx5k6frk55ohy@eaf> References: <20180503223115.GD30522@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180503223115.GD30522@ZenIV.linux.org.uk> Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, May 03, 2018 at 11:31:15PM +0100, Al Viro wrote: > On Thu, May 03, 2018 at 07:08:22PM -0300, Ernesto A. Fernández wrote: > > If no hidden directory exists, the hfsplus_fill_super() function will > > create it. A delayed work is then queued to sync the superblock, which > > is never canceled in case of failure. Fix this. > > Wouldn't it be simpler to avoid all the crap with clearing ->s_root > on failure, letting ->put_super() take care of everything? Or, better > yet, take cleanups into ->kill_sb(), which is always called on > superblock shutdown, ->s_root or no ->s_root... If you mean that I move all cleanups from fill_super() to ->kill_sb(), that sounds like a lot of trouble. Is this common in other filesystems? As for letting ->put_super() do the cleanup, the problem is that it will flag the volume as consistent, even if the directory count was increased and the hidden dir was not added to the catalog. So we should correct that before returning from fill_super(). I wouldn't call this simpler, but I guess it's a bit better for consistency. -- >8 -- Subject: [PATCH] hfsplus: fix cleanup for hfsplus_fill_super() If no hidden directory exists, the hfsplus_fill_super() function will create it. A delayed work is then queued to sync the superblock, which is never canceled in case of failure. Also, if the filesystem is corrupted in such a way that the hidden directory is not of type HFSPLUS_FOLDER, the mount will fail without throwing an error code. The vfs layer is then forced to dereference a NULL root dentry. To fix these issues, return an error and allow ->put_super() to take care of most of the cleanup if failure occurs after sb->s_root has been set. Before this patch the volume was simply flagged as inconsistent if the mount failed while creating the hidden directory. Since ->put_super() will now toggle those flags, be sure to correct the folder count before returning, with a call to hfsplus_delete_inode(). Reported-by: syzbot+4f2e5f086147d543ab03@syzkaller.appspotmail.com Fixes: 5bd9d99d107c ("hfsplus: add error checking for hfs_find_init()") Fixes: 9e6c5829b07c ("hfsplus: get rid of write_super") Signed-off-by: Ernesto A. Fernández --- fs/hfsplus/super.c | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c index 513c357c734b..cd39f8d34241 100644 --- a/fs/hfsplus/super.c +++ b/fs/hfsplus/super.c @@ -514,22 +514,26 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) goto out_put_alloc_file; } + /* From here on, most of the cleanup is handled by ->put_super */ + str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1; str.name = HFSP_HIDDENDIR_NAME; err = hfs_find_init(sbi->cat_tree, &fd); if (err) - goto out_put_root; + goto out; err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str); if (unlikely(err < 0)) - goto out_put_root; + goto out; if (!hfs_brec_read(&fd, &entry, sizeof(entry))) { hfs_find_exit(&fd); - if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) - goto out_put_root; + if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) { + err = -EINVAL; + goto out; + } inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id)); if (IS_ERR(inode)) { err = PTR_ERR(inode); - goto out_put_root; + goto out; } sbi->hidden_dir = inode; } else @@ -551,16 +555,13 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) mutex_lock(&sbi->vh_mutex); sbi->hidden_dir = hfsplus_new_inode(sb, root, S_IFDIR); if (!sbi->hidden_dir) { - mutex_unlock(&sbi->vh_mutex); err = -ENOMEM; - goto out_put_root; + goto out_unlock_vh_mutex; } err = hfsplus_create_cat(sbi->hidden_dir->i_ino, root, &str, sbi->hidden_dir); - if (err) { - mutex_unlock(&sbi->vh_mutex); - goto out_put_hidden_dir; - } + if (err) + goto out_delete_hidden_dir; err = hfsplus_init_inode_security(sbi->hidden_dir, root, &str); @@ -573,8 +574,7 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) */ hfsplus_delete_cat(sbi->hidden_dir->i_ino, root, &str); - mutex_unlock(&sbi->vh_mutex); - goto out_put_hidden_dir; + goto out_delete_hidden_dir; } mutex_unlock(&sbi->vh_mutex); @@ -587,11 +587,13 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) sbi->nls = nls; return 0; -out_put_hidden_dir: - iput(sbi->hidden_dir); -out_put_root: - dput(sb->s_root); - sb->s_root = NULL; +out_delete_hidden_dir: + clear_nlink(inode); + hfsplus_delete_inode(inode); +out_unlock_vh_mutex: + mutex_unlock(&sbi->vh_mutex); + goto out; + out_put_alloc_file: iput(sbi->alloc_file); out_close_attr_tree: @@ -605,9 +607,9 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent) kfree(sbi->s_backup_vhdr_buf); out_unload_nls: unload_nls(sbi->nls); - unload_nls(nls); kfree(sbi); out: + unload_nls(nls); return err; }