From patchwork Tue Apr 5 13:36:57 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lars Ellenberg X-Patchwork-Id: 8751831 Return-Path: X-Original-To: patchwork-linux-btrfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 38823C0553 for ; Tue, 5 Apr 2016 13:46:10 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 366892028D for ; Tue, 5 Apr 2016 13:46:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2739A20263 for ; Tue, 5 Apr 2016 13:46:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933208AbcDENqC (ORCPT ); Tue, 5 Apr 2016 09:46:02 -0400 Received: from zimbra13.linbit.com ([212.69.166.240]:58643 "EHLO zimbra13.linbit.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932844AbcDENqB (ORCPT ); Tue, 5 Apr 2016 09:46:01 -0400 X-Greylist: delayed 538 seconds by postgrey-1.27 at vger.kernel.org; Tue, 05 Apr 2016 09:46:00 EDT Received: from localhost (localhost [127.0.0.1]) by zimbra13.linbit.com (Postfix) with ESMTP id 93CBA3F774E; Tue, 5 Apr 2016 15:36:58 +0200 (CEST) Received: from zimbra13.linbit.com ([127.0.0.1]) by localhost (zimbra13.linbit.com [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id d6efzVGPEtyt; Tue, 5 Apr 2016 15:36:58 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by zimbra13.linbit.com (Postfix) with ESMTP id 6DA9C3F7751; Tue, 5 Apr 2016 15:36:58 +0200 (CEST) X-Virus-Scanned: amavisd-new at linbit.com Received: from zimbra13.linbit.com ([127.0.0.1]) by localhost (zimbra13.linbit.com [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id KKYaAynMI5ON; Tue, 5 Apr 2016 15:36:58 +0200 (CEST) Received: from soda.linbit (tuerlsteher.linbit.com [86.59.100.100]) by zimbra13.linbit.com (Postfix) with ESMTPS id 357953F774E; Tue, 5 Apr 2016 15:36:58 +0200 (CEST) Date: Tue, 5 Apr 2016 15:36:57 +0200 From: Lars Ellenberg To: Neil Brown , Jens Axboe Cc: Chris Mason , Josef Bacik , David Sterba , linux-raid@vger.kernel.org, linux-kernel@vger.kernel.org, linux-btrfs@vger.kernel.org Subject: [PATCH] [RFC] fix potential access after free: return value of blk_check_plugged() must be used schedule() safe Message-ID: <20160405133657.GA3078@soda.linbit> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org X-Spam-Status: No, score=-7.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP blk_check_plugged() will return a pointer to an object linked on current->plug->cb_list. That list may "at any time" be implicitly cleared by blk_flush_plug_list() flush_plug_callbacks() either as a result of blk_finish_plug(), or implicitly by schedule() [and maybe other implicit mechanisms?] If there is no protection against an implicit unplug between the call to blk_check_plug() and using its return value, that implicit unplug may have already happened, even before the plug is actually initialized or populated, and we may be using a pointer to already free()d data. I suggest that both raid1 and raid10 can easily be fixed by moving the call to blk_check_plugged() inside the spinlock. For md/raid5 and btrfs/raid56, I'm unsure how (if) this needs to be fixed. The other current in-tree users of blk_check_plugged() are mm_check_plugged(), and mddev_check_plugged(). mm_check_plugged() is already used safely inside a spinlock. with mddev_check_plugged() I'm unsure, at least on a preempt kernel. Did I overlook any magic that protects against such implicit unplug? Also, why pretend that a custom plug struct (such as raid1_plug_cb) may have its member "struct blk_plug_cb cb" at an arbitrary offset? As it is, raid1_check_plugged() below is actually just a cast. Signed-off-by: Lars Ellenberg --- drivers/md/raid1.c | 19 +++++++++++++------ drivers/md/raid10.c | 21 +++++++++++++-------- drivers/md/raid5.c | 5 +++++ fs/btrfs/raid56.c | 5 +++++ 4 files changed, 36 insertions(+), 14 deletions(-) diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c index 39fb21e..55dc960 100644 --- a/drivers/md/raid1.c +++ b/drivers/md/raid1.c @@ -1044,6 +1044,18 @@ static void raid1_unplug(struct blk_plug_cb *cb, bool from_schedule) kfree(plug); } +static struct raid1_plug_cb *raid1_check_plugged(struct mddev *mddev) +{ + /* return (struct raid1_plug_cb*)blk_check_plugged(...); */ + struct blk_plug_cb *cb; + struct raid1_plug_cb *plug = NULL; + + cb = blk_check_plugged(raid1_unplug, mddev, sizeof(*plug)); + if (cb) + plug = container_of(cb, struct raid1_plug_cb, cb); + return plug; +} + static void raid1_make_request(struct mddev *mddev, struct bio * bio) { struct r1conf *conf = mddev->private; @@ -1060,7 +1072,6 @@ static void raid1_make_request(struct mddev *mddev, struct bio * bio) & (REQ_DISCARD | REQ_SECURE)); const unsigned long do_same = (bio->bi_rw & REQ_WRITE_SAME); struct md_rdev *blocked_rdev; - struct blk_plug_cb *cb; struct raid1_plug_cb *plug = NULL; int first_clone; int sectors_handled; @@ -1382,12 +1393,8 @@ read_again: atomic_inc(&r1_bio->remaining); - cb = blk_check_plugged(raid1_unplug, mddev, sizeof(*plug)); - if (cb) - plug = container_of(cb, struct raid1_plug_cb, cb); - else - plug = NULL; spin_lock_irqsave(&conf->device_lock, flags); + plug = raid1_check_plugged(mddev); if (plug) { bio_list_add(&plug->pending, mbio); plug->pending_cnt++; diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c index e3fd725..d7d4397 100644 --- a/drivers/md/raid10.c +++ b/drivers/md/raid10.c @@ -1052,6 +1052,18 @@ static void raid10_unplug(struct blk_plug_cb *cb, bool from_schedule) kfree(plug); } +static struct raid10_plug_cb *raid10_check_plugged(struct mddev *mddev) +{ + /* return (struct raid1_plug_cb*)blk_check_plugged(...); */ + struct blk_plug_cb *cb; + struct raid10_plug_cb *plug = NULL; + + cb = blk_check_plugged(raid10_unplug, mddev, sizeof(*plug)); + if (cb) + plug = container_of(cb, struct raid10_plug_cb, cb); + return plug; +} + static void __make_request(struct mddev *mddev, struct bio *bio) { struct r10conf *conf = mddev->private; @@ -1066,7 +1078,6 @@ static void __make_request(struct mddev *mddev, struct bio *bio) const unsigned long do_same = (bio->bi_rw & REQ_WRITE_SAME); unsigned long flags; struct md_rdev *blocked_rdev; - struct blk_plug_cb *cb; struct raid10_plug_cb *plug = NULL; int sectors_handled; int max_sectors; @@ -1369,14 +1380,8 @@ retry_write: atomic_inc(&r10_bio->remaining); - cb = blk_check_plugged(raid10_unplug, mddev, - sizeof(*plug)); - if (cb) - plug = container_of(cb, struct raid10_plug_cb, - cb); - else - plug = NULL; spin_lock_irqsave(&conf->device_lock, flags); + plug = raid10_check_plugged(mddev); if (plug) { bio_list_add(&plug->pending, mbio); plug->pending_cnt++; diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c index 8ab8b65..4e3b02b 100644 --- a/drivers/md/raid5.c +++ b/drivers/md/raid5.c @@ -5034,6 +5034,11 @@ static void release_stripe_plug(struct mddev *mddev, } cb = container_of(blk_cb, struct raid5_plug_cb, cb); +/* FIXME + * Nothing protects current from being scheduled, which means cb, aka plug, + * may implicitly be "unplugged" any time now, before it even is initialized, + * and will then be a pointer to free()d space. + */ if (cb->list.next == NULL) { int i; diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c index 0b7792e..17757d4 100644 --- a/fs/btrfs/raid56.c +++ b/fs/btrfs/raid56.c @@ -1774,6 +1774,11 @@ int raid56_parity_write(struct btrfs_root *root, struct bio *bio, cb = blk_check_plugged(btrfs_raid_unplug, root->fs_info, sizeof(*plug)); if (cb) { +/* FIXME + * Nothing protects current from being scheduled, which means cb, aka plug, + * may implicitly be "unplugged" any time now, before it even is initialized, + * and will then be a pointer to free()d space. + */ plug = container_of(cb, struct btrfs_plug_cb, cb); if (!plug->info) { plug->info = root->fs_info;