Received: by 2002:a25:1506:0:0:0:0:0 with SMTP id 6csp3693931ybv; Mon, 10 Feb 2020 04:50:49 -0800 (PST) X-Google-Smtp-Source: APXvYqz4EZxDRXVrKeB1XhQdVyTTC5s82FIfYyF2n7DEIcWF01WECmTS5IefW9VIe1M9fDmZNYhP X-Received: by 2002:aca:fc0c:: with SMTP id a12mr694778oii.118.1581339049333; Mon, 10 Feb 2020 04:50:49 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1581339049; cv=none; d=google.com; s=arc-20160816; b=y5pHhDWHg2CC+Vq2xeFFIlPLBd5hmQKIgpysLfzkUVMf82IJklAzuDBJz8D9je3h50 XuJkyXD7SIQs7+tgYrVfUtUZTrrhYZv+jkDp0GdH5l3pRdxL+UA17UvZcGpce1R926DQ QPb+IxBaYjX6h+HWyw3QAvm0K3jO3Q/E0kb0JHJQQ061PBVFNI3IcFvrAIH1bBxIEbCP a4QBBS2uhuJOisWt/m3/hxK3zhqu5zq4QCKk6oIBR51UFXZ53Kx6bXNMlsD91TIvaJGr tCswo5TOuN5FcLhnI4v4C7OQNEed8pG8joutGfWeBCBVf2YWA4mKTQRfNLsjCpAt8+pw aHsg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :user-agent:references:in-reply-to:message-id:date:subject:cc:to :from:dkim-signature; bh=qvoPskyDREl4xacJ9+m6e2vdMiUtxLhHxLFME0lfVlw=; b=vnYedbOzAeWTpFpU0ocONIxLPAFWkketU+VB6VvBLrlvuj45TANdgGowl797mcKfMk eKZpufhh3a3Wu/mXuyUThP9Tv6pIWDBfo//krPDma62sOnznNq4Y5hxx1lYh1ZTm8x4A 8YQOb9Ws9eBV0WZn19vL8MFgtqPselSDKeHKYaSn6lkNJ9HJZaSBJ55hFgiC9z81vGdd dNOl8a9JHZ8eRJY2zflPan3ZyHNGvuBK17KdqIgJSoqr1Soobgw6H7vP6/x7N/oC1EsB sBYcqvVMedoM2qGoph9kPr31B9MaHdSy0IpNLEZpOD4mPK3acG9w7HhMrzBOZLbwUA7C fDwg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b="SommgJL/"; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id n11si155636otf.36.2020.02.10.04.50.37; Mon, 10 Feb 2020 04:50:49 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b="SommgJL/"; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730131AbgBJMpH (ORCPT + 99 others); Mon, 10 Feb 2020 07:45:07 -0500 Received: from mail.kernel.org ([198.145.29.99]:41428 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729822AbgBJMkx (ORCPT ); Mon, 10 Feb 2020 07:40:53 -0500 Received: from localhost (unknown [209.37.97.194]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 4535520733; Mon, 10 Feb 2020 12:40:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1581338452; bh=a2IUfa9DBBpc9a6T9LMpFETdGD1oEW1hyYjY7eDioO4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=SommgJL/aBrfZ/3kiox2Mrvfheb0RpBWlUmSXOp7LTZdunTJrVZ8SwQPT1hxoTnex Z6H9om61rrE2s8TGyCi5YfvDqtIkD6lGiUha5vYWeFvpPSiPkATGuNIZvF4mtxZa63 ufqXqNMDGC2b3hTR6HGWZgCNrAxRMWhhP/nFtgR4= From: Greg Kroah-Hartman To: linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , stable@vger.kernel.org, Josef Bacik , David Sterba Subject: [PATCH 5.5 198/367] btrfs: set trans->drity in btrfs_commit_transaction Date: Mon, 10 Feb 2020 04:31:51 -0800 Message-Id: <20200210122442.854292803@linuxfoundation.org> X-Mailer: git-send-email 2.25.0 In-Reply-To: <20200210122423.695146547@linuxfoundation.org> References: <20200210122423.695146547@linuxfoundation.org> User-Agent: quilt/0.66 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Josef Bacik commit d62b23c94952e78211a383b7d90ef0afbd9a3717 upstream. If we abort a transaction we have the following sequence if (!trans->dirty && list_empty(&trans->new_bgs)) return; WRITE_ONCE(trans->transaction->aborted, err); The idea being if we didn't modify anything with our trans handle then we don't really need to abort the whole transaction, maybe the other trans handles are fine and we can carry on. However in the case of create_snapshot we add a pending_snapshot object to our transaction and then commit the transaction. We don't actually modify anything. sync() behaves the same way, attach to an existing transaction and commit it. This means that if we have an IO error in the right places we could abort the committing transaction with our trans->dirty being not set and thus not set transaction->aborted. This is a problem because in the create_snapshot() case we depend on pending->error being set to something, or btrfs_commit_transaction returning an error. If we are not the trans handle that gets to commit the transaction, and we're waiting on the commit to happen we get our return value from cur_trans->aborted. If this was not set to anything because sync() hit an error in the transaction commit before it could modify anything then cur_trans->aborted would be 0. Thus we'd return 0 from btrfs_commit_transaction() in create_snapshot. This is a problem because we then try to do things with pending_snapshot->snap, which will be NULL because we didn't create the snapshot, and then we'll get a NULL pointer dereference like the following "BUG: kernel NULL pointer dereference, address: 00000000000001f0" RIP: 0010:btrfs_orphan_cleanup+0x2d/0x330 Call Trace: ? btrfs_mksubvol.isra.31+0x3f2/0x510 btrfs_mksubvol.isra.31+0x4bc/0x510 ? __sb_start_write+0xfa/0x200 ? mnt_want_write_file+0x24/0x50 btrfs_ioctl_snap_create_transid+0x16c/0x1a0 btrfs_ioctl_snap_create_v2+0x11e/0x1a0 btrfs_ioctl+0x1534/0x2c10 ? free_debug_processing+0x262/0x2a3 do_vfs_ioctl+0xa6/0x6b0 ? do_sys_open+0x188/0x220 ? syscall_trace_enter+0x1f8/0x330 ksys_ioctl+0x60/0x90 __x64_sys_ioctl+0x16/0x20 do_syscall_64+0x4a/0x1b0 In order to fix this we need to make sure anybody who calls commit_transaction has trans->dirty set so that they properly set the trans->transaction->aborted value properly so any waiters know bad things happened. This was found while I was running generic/475 with my modified fsstress, it reproduced within a few runs. I ran with this patch all night and didn't see the problem again. CC: stable@vger.kernel.org # 4.4+ Signed-off-by: Josef Bacik Reviewed-by: David Sterba Signed-off-by: David Sterba Signed-off-by: Greg Kroah-Hartman --- fs/btrfs/transaction.c | 8 ++++++++ 1 file changed, 8 insertions(+) --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -2013,6 +2013,14 @@ int btrfs_commit_transaction(struct btrf ASSERT(refcount_read(&trans->use_count) == 1); + /* + * Some places just start a transaction to commit it. We need to make + * sure that if this commit fails that the abort code actually marks the + * transaction as failed, so set trans->dirty to make the abort code do + * the right thing. + */ + trans->dirty = true; + /* Stop the commit early if ->aborted is set */ if (unlikely(READ_ONCE(cur_trans->aborted))) { ret = cur_trans->aborted;