From patchwork Wed Mar 21 16:13:25 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Brian Foster X-Patchwork-Id: 10299759 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 14797600CC for ; Wed, 21 Mar 2018 16:13:41 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id EBDC0288D1 for ; Wed, 21 Mar 2018 16:13:40 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id E0736288FE; Wed, 21 Mar 2018 16:13:40 +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=-6.9 required=2.0 tests=BAYES_00,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 8D4FC298F0 for ; Wed, 21 Mar 2018 16:13:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751729AbeCUQN2 (ORCPT ); Wed, 21 Mar 2018 12:13:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53916 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751873AbeCUQN1 (ORCPT ); Wed, 21 Mar 2018 12:13:27 -0400 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 50B7F80462 for ; Wed, 21 Mar 2018 16:13:27 +0000 (UTC) Received: from bfoster.bfoster (dhcp-41-20.bos.redhat.com [10.18.41.20]) by smtp.corp.redhat.com (Postfix) with ESMTP id 20701611A0 for ; Wed, 21 Mar 2018 16:13:27 +0000 (UTC) Received: by bfoster.bfoster (Postfix, from userid 1000) id C0C65120281; Wed, 21 Mar 2018 12:13:25 -0400 (EDT) From: Brian Foster To: linux-xfs@vger.kernel.org Subject: [PATCH] xfs: clean up xfs_mount allocation and dynamic initializers Date: Wed, 21 Mar 2018 12:13:25 -0400 Message-Id: <20180321161325.12773-1-bfoster@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Wed, 21 Mar 2018 16:13:27 +0000 (UTC) Sender: linux-xfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-xfs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Most of the generic data structures embedded in xfs_mount are dynamically initialized immediately after mp is allocated. A few fields are left out and initialized during the xfs_mountfs() sequence, after mp has been attached to the superblock. To clean this up and help prevent premature access of associated fields, refactor xfs_mount allocation and all dependent init calls into a new helper. This self-documents that all low level data structures (i.e., locks, trees, etc.) should be initialized before xfs_mount is attached to the superblock. Signed-off-by: Brian Foster Reviewed-by: Darrick J. Wong --- I realized after the fact that this steps on the ability to instrument the use-before-init variant[1] of the problematic radix tree access on failed mount issue that Dave tracked down. This is because we'd immediately initialize the structure and thus a subsequent memset() would ultimately require another proper initialization. As Dave noted in that thread, we could alternatively instrument the use-after-free variant of the problem with a post-free delay of the mp. I actually wonder if a last step instrumented failure in fill_super() might be a more broadly useful test because it provides future coverage of the entire mount teardown sequence rather than just the bad radix tree access. On the flipside, this patch isn't the cleanup of the century so I'd be fine with dropping it in favor of [1], but I think it's worthy enough to consider. Brian [1] https://marc.info/?l=linux-xfs&m=152152202700598&w=2 fs/xfs/libxfs/xfs_sb.c | 1 - fs/xfs/xfs_mount.c | 2 -- fs/xfs/xfs_super.c | 39 +++++++++++++++++++++++++++++---------- 3 files changed, 29 insertions(+), 13 deletions(-) diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c index a55f7a45fa78..53433cc024fd 100644 --- a/fs/xfs/libxfs/xfs_sb.c +++ b/fs/xfs/libxfs/xfs_sb.c @@ -731,7 +731,6 @@ xfs_sb_mount_common( struct xfs_sb *sbp) { mp->m_agfrotor = mp->m_agirotor = 0; - spin_lock_init(&mp->m_agirotor_lock); mp->m_maxagi = mp->m_sb.sb_agcount; mp->m_blkbit_log = sbp->sb_blocklog + XFS_NBBYLOG; mp->m_blkbb_log = sbp->sb_blocklog - BBSHIFT; diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index 6f5a5e6764d8..a901b86772f8 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -817,8 +817,6 @@ xfs_mountfs( /* * Allocate and initialize the per-ag data. */ - spin_lock_init(&mp->m_perag_lock); - INIT_RADIX_TREE(&mp->m_perag_tree, GFP_ATOMIC); error = xfs_initialize_perag(mp, sbp->sb_agcount, &mp->m_maxagi); if (error) { xfs_warn(mp, "Failed per-ag init: %d", error); diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 951271f57d00..612c1d5348b3 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -1579,29 +1579,48 @@ xfs_destroy_percpu_counters( percpu_counter_destroy(&mp->m_fdblocks); } -STATIC int -xfs_fs_fill_super( - struct super_block *sb, - void *data, - int silent) +static struct xfs_mount * +xfs_mount_alloc( + struct super_block *sb) { - struct inode *root; - struct xfs_mount *mp = NULL; - int flags = 0, error = -ENOMEM; + struct xfs_mount *mp; mp = kzalloc(sizeof(struct xfs_mount), GFP_KERNEL); if (!mp) - goto out; + return NULL; + mp->m_super = sb; spin_lock_init(&mp->m_sb_lock); + spin_lock_init(&mp->m_agirotor_lock); + INIT_RADIX_TREE(&mp->m_perag_tree, GFP_ATOMIC); + spin_lock_init(&mp->m_perag_lock); mutex_init(&mp->m_growlock); atomic_set(&mp->m_active_trans, 0); INIT_DELAYED_WORK(&mp->m_reclaim_work, xfs_reclaim_worker); INIT_DELAYED_WORK(&mp->m_eofblocks_work, xfs_eofblocks_worker); INIT_DELAYED_WORK(&mp->m_cowblocks_work, xfs_cowblocks_worker); mp->m_kobj.kobject.kset = xfs_kset; + return mp; +} - mp->m_super = sb; + +STATIC int +xfs_fs_fill_super( + struct super_block *sb, + void *data, + int silent) +{ + struct inode *root; + struct xfs_mount *mp = NULL; + int flags = 0, error = -ENOMEM; + + /* + * allocate mp and do all low-level struct initializations before we + * attach it to the super + */ + mp = xfs_mount_alloc(sb); + if (!mp) + goto out; sb->s_fs_info = mp; error = xfs_parseargs(mp, (char *)data);