From patchwork Tue May 14 01:39:00 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alex Elder X-Patchwork-Id: 2562141 Return-Path: X-Original-To: patchwork-ceph-devel@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id 219913FD4E for ; Tue, 14 May 2013 01:39:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754656Ab3ENBjE (ORCPT ); Mon, 13 May 2013 21:39:04 -0400 Received: from mail-oa0-f44.google.com ([209.85.219.44]:38196 "EHLO mail-oa0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752347Ab3ENBjC (ORCPT ); Mon, 13 May 2013 21:39:02 -0400 Received: by mail-oa0-f44.google.com with SMTP id n12so8437715oag.17 for ; Mon, 13 May 2013 18:39:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:message-id:date:from:user-agent:mime-version:to:subject :content-type:content-transfer-encoding:x-gm-message-state; bh=NXpKPaHZ6HkmRbk6GtloCyHoEGOBaCTMo0sC9/S9i4w=; b=YoBz2XFBUqSQ7+rUwqXAq5DWhmSw5VZR4PtO+SXK27sn+4fgWrTTizC5BK9NNEKxqq Ng/Zs+KmWitJrJmLPjqoGvigBe7y7nC+5rw6FutFtq7QJ4qwjpTeKrImeh2aluHD7jG6 5YayucYvRsbBrop9RWtC5c24ewyXyea5vGUjjDFp95Xc2D/A3+TTt2zuHv5dHLa4yF/5 2b3ophrzP8XwLaneoMo4zjKffX2IvOzY3651nx/mOP3X2t0Zr8s820lMrGYX+sYDKPlv VTKUIwJ0MceDTjOXANsH7USuc8ZwdD4b89zeU18LBuGQeKEMFTt5RUDccUUUDBw37bjH R/ZQ== X-Received: by 10.60.94.171 with SMTP id dd11mr14648926oeb.76.1368495542314; Mon, 13 May 2013 18:39:02 -0700 (PDT) Received: from [172.22.22.4] (c-71-195-31-37.hsd1.mn.comcast.net. [71.195.31.37]) by mx.google.com with ESMTPSA id dt3sm19317884obb.12.2013.05.13.18.39.00 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 13 May 2013 18:39:01 -0700 (PDT) Message-ID: <519195B4.1080802@inktank.com> Date: Mon, 13 May 2013 20:39:00 -0500 From: Alex Elder User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: ceph-devel@vger.kernel.org Subject: [PATCH] rbd: fix cleanup in rbd_add() X-Gm-Message-State: ALoCoQnN9vh9Rua5oNL5RRfpIOJ7vTjUJcX1Oh42TzTd892Uz2gciU5lMern+ffo/54FbVL+Nd6f Sender: ceph-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: ceph-devel@vger.kernel.org Bjorn Helgaas pointed out that a recent commit introduced a use-after-free condition in an error path for rbd_add(). He correctly stated: I think b536f69a3a5 "rbd: set up devices only for mapped images" introduced a use-after-free error in rbd_add(): ... If rbd_dev_device_setup() returns an error, we call rbd_dev_image_release(), which ultimately kfrees rbd_dev. Then we call rbd_dev_destroy(), which references fields in the already-freed rbd_dev struct before kfreeing it again. The simple fix is to return the error code after the call to rbd_dev_image_release(). Closer examination revealed that there's no need to clean up ceph_opts or rbd_opts in that function, so fix that too. Update some other comments that have also become out of date. Reported-by: Bjorn Helgaas Signed-off-by: Alex Elder --- drivers/block/rbd.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) if (ret) @@ -4994,7 +4997,7 @@ static ssize_t rbd_add(struct bus_type *bus, rc = PTR_ERR(rbdc); goto err_out_args; } - ceph_opts = NULL; /* rbd_dev client now owns this */ + ceph_opts = NULL; /* rbd_get_client() consumes this */ /* pick the pool */ osdc = &rbdc->client->osdc; @@ -5033,14 +5036,14 @@ static ssize_t rbd_add(struct bus_type *bus, return count; rbd_dev_image_release(rbd_dev); + + return rc; + err_out_rbd_dev: rbd_dev_destroy(rbd_dev); err_out_client: rbd_put_client(rbdc); err_out_args: - if (ceph_opts) - ceph_destroy_options(ceph_opts); - kfree(rbd_opts); rbd_spec_put(spec); err_out_module: module_put(THIS_MODULE); diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index e8d4693..632dd35 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -4699,8 +4699,10 @@ out: return ret; } -/* Undo whatever state changes are made by v1 or v2 image probe */ - +/* + * Undo whatever state changes are made by v1 or v2 header info + * call. + */ static void rbd_dev_unprobe(struct rbd_device *rbd_dev) { struct rbd_image_header *header; @@ -4904,9 +4906,10 @@ static int rbd_dev_image_probe(struct rbd_device *rbd_dev, bool mapping) int tmp; /* - * Get the id from the image id object. If it's not a - * format 2 image, we'll get ENOENT back, and we'll assume - * it's a format 1 image. + * Get the id from the image id object. Unless there's an + * error, rbd_dev->spec->image_id will be filled in with + * a dynamically-allocated string, and rbd_dev->image_format + * will be set to either 1 or 2. */ ret = rbd_dev_image_id(rbd_dev);