From patchwork Tue Feb 21 19:09:05 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jason Gunthorpe X-Patchwork-Id: 730719 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from mail-qt0-x23c.google.com (mail-qt0-x23c.google.com [IPv6:2607:f8b0:400d:c0d::23c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3vSVSx1JPbz9ryQ for ; Wed, 22 Feb 2017 06:10:40 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=googlegroups.com header.i=@googlegroups.com header.b="kaG1KHKE"; dkim-atps=neutral Received: by mail-qt0-x23c.google.com with SMTP id h53sf41028355qth.1 for ; Tue, 21 Feb 2017 11:10:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlegroups.com; s=20161025; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent:x-original-sender :x-original-authentication-results:reply-to:precedence:mailing-list :list-id:x-spam-checked-in-group:list-post:list-help:list-archive :list-subscribe:list-unsubscribe; bh=iBJG/aSZ8XSG0ZaQFtxvmlzjAL0+g3Eb94iXFbYtIK8=; b=kaG1KHKEA35B57NH1xiuxgCVwMJ/DUCfODwQpmisaR33q4vS+VTKILRrUL1fR0qeJl KZjgM4AZnn/bRsHyjyDMTX5qVxN4qpBQuiiLfo2/+VLFDGNZt9/dCZzNojdRk7jdWAXU SKyXFv9hFmLm2tkCOstL7v1nPKXFdbcaqtlLnHbJUjCb4q2dPQtUKIZ6iLUtv70fTu2b SCtLVrksKMeBkp5cdgumt7hD+0AwDXQz56rtuUxgNvLo8TzEdWxcZKEFvFkdBLCYy9Yu 1zhGA8uUH06AfwIDeZgst7xiB7jWRSIan5JtDlz/WG/J2KV7vdOYsP4jAi4Tr1ObU6l3 gF4Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=sender:x-gm-message-state:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to:user-agent :x-original-sender:x-original-authentication-results:reply-to :precedence:mailing-list:list-id:x-spam-checked-in-group:list-post :list-help:list-archive:list-subscribe:list-unsubscribe; bh=iBJG/aSZ8XSG0ZaQFtxvmlzjAL0+g3Eb94iXFbYtIK8=; b=iJ35zu6CWN2JcqDG67E2RhuMoxlZlX838O/4wE1GPYpjPwOdkwRhEh2Vxv2VxqMv9x ZGiSrnKXfFiNMHyd6Rst/H2fYD6Cet4VMV0eoilWZU8pqFBW8anPMESUmDut7xL8+k8L VVoJXvou4r4zTbrCQ0YIyKYzezaNGJN++FiUM1+Tziv4AAtX9JZ/sgAUEitvT3QoI2Me /mN5p/L2ApmE2oh1xUCAesSRlw4sZ2Hh1pLPPZtPKJ4wH6+9+6w8dKvuOZPgEplE+hCW 88p34LWefINB3Qnukaih//xxxjVECm8W16kOEmKCfjtrbRPtYDr6zo8glPAdEcIu1uaw U6Eg== Sender: rtc-linux@googlegroups.com X-Gm-Message-State: AMke39nZ6UAPj19bkA1KGyuJ2Biu8zdIrK1yz9x4LupJAvzH4MZScc2nqovvqU6Y+wM2/w== X-Received: by 10.157.12.1 with SMTP id 1mr1591404otr.2.1487704238632; Tue, 21 Feb 2017 11:10:38 -0800 (PST) X-BeenThere: rtc-linux@googlegroups.com Received: by 10.157.48.159 with SMTP id s31ls8062848otc.10.gmail; Tue, 21 Feb 2017 11:10:38 -0800 (PST) X-Received: by 10.176.3.11 with SMTP id 11mr3320173uat.29.1487704238301; Tue, 21 Feb 2017 11:10:38 -0800 (PST) Received: from quartz.orcorp.ca (quartz.orcorp.ca. [184.70.90.242]) by gmr-mx.google.com with ESMTPS id e196si2086313itc.2.2017.02.21.11.10.38 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 21 Feb 2017 11:10:38 -0800 (PST) Received-SPF: pass (google.com: domain of jgunthorpe@obsidianresearch.com designates 184.70.90.242 as permitted sender) client-ip=184.70.90.242; Received: from [10.0.0.156] (helo=jggl.edm.orcorp.ca) by quartz.orcorp.ca with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from ) id 1cgFoA-0007th-1A; Tue, 21 Feb 2017 12:09:06 -0700 Received: from jgg by jggl.edm.orcorp.ca with local (Exim 4.86_2) (envelope-from ) id 1cgFo9-0000yh-Tb; Tue, 21 Feb 2017 12:09:05 -0700 Date: Tue, 21 Feb 2017 12:09:05 -0700 From: Jason Gunthorpe To: Logan Gunthorpe Cc: Greg Kroah-Hartman , Dan Williams , Alexander Viro , Johannes Thumshirn , Jan Kara , Arnd Bergmann , Sajjan Vikas C , Dmitry Torokhov , Linus Walleij , Alexandre Courbot , Peter Huewe , Marcel Selhorst , Jarkko Sakkinen , Olof Johansson , Doug Ledford , Sean Hefty , Hal Rosenstock , Dmitry Vyukov , Haggai Eran , Parav Pandit , Leon Romanovsky , Jonathan Cameron , Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler , Hans Verkuil , Mauro Carvalho Chehab , Artem Bityutskiy , Richard Weinberger , David Woodhouse , Brian Norris , Boris Brezillon , Marek Vasut , Cyrille Pitchen , Matt Porter , Alexandre Bounine , Andrew Morton , Joe Perches , Lorenzo Stoakes , Vladimir Zapolskiy , Alessandro Zummo , Alexandre Belloni , Boaz Harrosh , Benny Halevy , "James E.J. Bottomley" , "Martin K. Petersen" , Stephen Bates , Bjorn Helgaas , linux-pci@vger.kernel.org, osd-dev@open-osd.org, linux-scsi@vger.kernel.org, rtc-linux@googlegroups.com, linux-mtd@lists.infradead.org, linux-media@vger.kernel.org, linux-iio@vger.kernel.org, linux-rdma@vger.kernel.org, tpmdd-devel@lists.sourceforge.net, linux-gpio@vger.kernel.org, linux-input@vger.kernel.org, linux-nvdimm@ml01.01.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [rtc-linux] Re: [PATCH 07/14] infiniband: utilize new device_add_cdev helper function Message-ID: <20170221190905.GD13138@obsidianresearch.com> References: <1487653253-11497-1-git-send-email-logang@deltatee.com> <1487653253-11497-8-git-send-email-logang@deltatee.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1487653253-11497-8-git-send-email-logang@deltatee.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Broken-Reverse-DNS: no host name found for IP address 10.0.0.156 X-Original-Sender: jgunthorpe@obsidianresearch.com X-Original-Authentication-Results: gmr-mx.google.com; dkim=pass (test mode) header.i=@obsidianresearch.com; spf=pass (google.com: domain of jgunthorpe@obsidianresearch.com designates 184.70.90.242 as permitted sender) smtp.mailfrom=jgunthorpe@obsidianresearch.com Reply-To: rtc-linux@googlegroups.com Precedence: list Mailing-list: list rtc-linux@googlegroups.com; contact rtc-linux+owners@googlegroups.com List-ID: X-Spam-Checked-In-Group: rtc-linux@googlegroups.com X-Google-Group-Id: 712029733259 List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , On Mon, Feb 20, 2017 at 10:00:46PM -0700, Logan Gunthorpe wrote: > This patch updates core/ucm.c which didn't originally use the > cdev.kobj.parent with it's parent device. I did not look heavily into > whether this was a bug or not, but it seems likely to me there would > be a use before free. Hum, is probably safe - ib_ucm_remove_one can only happen on module unload and the cdev holds the module lock while open. Even so device_add_cdev should be used anyhow. > I also took a look at core/uverbs_main.c, core/user_mad.c and > hw/hfi1/device.c which utilize cdev.kobj.parent but because the > infiniband core seems to use kobjs internally instead of struct devices > they could not be converted to use the new helper API and still > directly manipulate the internals of the kobj. Yes, and hfi1 had the same use-afte-free bug when it was first submitted as well. IHMO cdev should have a helper entry for this style of use case as well. > diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c > index e0a995b..38ea316 100644 > +++ b/drivers/infiniband/core/ucm.c > @@ -1283,18 +1283,20 @@ static void ib_ucm_add_one(struct ib_device *device) > set_bit(devnum, dev_map); > } > > + device_initialize(&ucm_dev->dev); Ah, this needs more help. Once device_initialize is called put_device should be the error-unwind, can you use something more like this? From ac7a35ea98066c9a9e3f3e54557c0ccda44b0ffc Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Tue, 21 Feb 2017 12:07:55 -0700 Subject: [PATCH] infiniband: utilize new device_add_cdev helper The use after free is not triggerable here because the cdev holds the module lock and the only device_unregister is only triggered by module ouload, however make the change for consistency. To make this work the cdev_del needs to move out of the struct device release function. Signed-off-by: Jason Gunthorpe --- drivers/infiniband/core/ucm.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c index 7713ef089c3ccc..acda8d941381f3 100644 --- a/drivers/infiniband/core/ucm.c +++ b/drivers/infiniband/core/ucm.c @@ -1202,12 +1202,16 @@ static void ib_ucm_release_dev(struct device *dev) struct ib_ucm_device *ucm_dev; ucm_dev = container_of(dev, struct ib_ucm_device, dev); + kfree(ucm_dev); +} + +static void ib_ucm_cdev_del(struct ib_ucm_device *ucm_dev) +{ cdev_del(&ucm_dev->cdev); if (ucm_dev->devnum < IB_UCM_MAX_DEVICES) clear_bit(ucm_dev->devnum, dev_map); else clear_bit(ucm_dev->devnum - IB_UCM_MAX_DEVICES, overflow_map); - kfree(ucm_dev); } static const struct file_operations ucm_fops = { @@ -1263,6 +1267,7 @@ static void ib_ucm_add_one(struct ib_device *device) if (!ucm_dev) return; + device_initialize(&ucm_dev->dev); ucm_dev->ib_dev = device; devnum = find_first_zero_bit(dev_map, IB_UCM_MAX_DEVICES); @@ -1280,18 +1285,19 @@ static void ib_ucm_add_one(struct ib_device *device) set_bit(devnum, dev_map); } + ucm_dev->dev.devt = base; + ucm_dev->dev.release = ib_ucm_release_dev; + cdev_init(&ucm_dev->cdev, &ucm_fops); ucm_dev->cdev.owner = THIS_MODULE; kobject_set_name(&ucm_dev->cdev.kobj, "ucm%d", ucm_dev->devnum); - if (cdev_add(&ucm_dev->cdev, base, 1)) + if (device_add_cdev(&ucm_dev->dev, &ucm_dev->cdev)) goto err; ucm_dev->dev.class = &cm_class; ucm_dev->dev.parent = device->dma_device; - ucm_dev->dev.devt = ucm_dev->cdev.dev; - ucm_dev->dev.release = ib_ucm_release_dev; dev_set_name(&ucm_dev->dev, "ucm%d", ucm_dev->devnum); - if (device_register(&ucm_dev->dev)) + if (device_add(&ucm_dev->dev)) goto err_cdev; if (device_create_file(&ucm_dev->dev, &dev_attr_ibdev)) @@ -1303,13 +1309,9 @@ static void ib_ucm_add_one(struct ib_device *device) err_dev: device_unregister(&ucm_dev->dev); err_cdev: - cdev_del(&ucm_dev->cdev); - if (ucm_dev->devnum < IB_UCM_MAX_DEVICES) - clear_bit(devnum, dev_map); - else - clear_bit(devnum, overflow_map); + ib_ucm_cdev_del(ucm_dev); err: - kfree(ucm_dev); + put_device(&ucm_dev->dev); return; } @@ -1320,6 +1322,7 @@ static void ib_ucm_remove_one(struct ib_device *device, void *client_data) if (!ucm_dev) return; + ib_ucm_cdev_del(ucm_dev); device_unregister(&ucm_dev->dev); }