Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752364AbdGESxM (ORCPT ); Wed, 5 Jul 2017 14:53:12 -0400 Received: from imap0.codethink.co.uk ([185.43.218.159]:52294 "EHLO imap0.codethink.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752305AbdGESxL (ORCPT ); Wed, 5 Jul 2017 14:53:11 -0400 Message-ID: <1499280779.1935.96.camel@codethink.co.uk> Subject: Re: [PATCH 4.4 095/101] iommu: Handle default domain attach failure From: Ben Hutchings To: Robin Murphy , Joerg Roedel Cc: linux-kernel@vger.kernel.org, stable@vger.kernel.org, Punit Agrawal , Greg Kroah-Hartman Date: Wed, 05 Jul 2017 19:52:59 +0100 In-Reply-To: <20170703133350.300209040@linuxfoundation.org> References: <20170703133334.237346187@linuxfoundation.org> <20170703133350.300209040@linuxfoundation.org> Organization: Codethink Ltd. Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.9-1+b1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2434 Lines: 72 On Mon, 2017-07-03 at 15:35 +0200, Greg Kroah-Hartman wrote: > 4.4-stable review patch. If anyone has any objections, please let me know. > > ------------------ > > From: Robin Murphy > > commit 797a8b4d768c58caac58ee3e8cb36a164d1b7751 upstream. > > We wouldn't normally expect ops->attach_dev() to fail, but on IOMMUs > with limited hardware resources, or generally misconfigured systems, > it is certainly possible. We report failure correctly from the external > iommu_attach_device() interface, but do not do so in iommu_group_add() > when attaching to the default domain. The result of failure there is > that the device, group and domain all get left in a broken, > part-configured state which leads to weird errors and misbehaviour down > the line when IOMMU API calls sort-of-but-don't-quite work. > > Check the return value of __iommu_attach_device() on the default domain, > and refactor the error handling paths to cope with its failure and clean > up correctly in such cases. [...] > @@ -432,8 +426,10 @@ rename: > mutex_lock(&group->mutex); > list_add_tail(&device->list, &group->devices); > if (group->domain) > - __iommu_attach_device(group->domain, dev); > + ret = __iommu_attach_device(group->domain, dev); > mutex_unlock(&group->mutex); > + if (ret) > + goto err_put_group; It's still (briefly) possible for other tasks to observe the device in the broken state. Shouldn't the error check be done before mutex_unlock()? > /* Notify any listeners about change to group. */ > blocking_notifier_call_chain(&group->notifier, > @@ -444,6 +440,21 @@ rename: > pr_info("Adding device %s to group %d\n", dev_name(dev), group->id); > > return 0; > + > +err_put_group: > + mutex_lock(&group->mutex); > + list_del(&device->list); > + mutex_unlock(&group->mutex); > + dev->iommu_group = NULL; > + kobject_put(group->devices_kobj); > +err_free_name: > + kfree(device->name); > +err_remove_link: > + sysfs_remove_link(&dev->kobj, "iommu_group"); > +err_free_device: > + kfree(device); > + pr_err("Failed to add device %s to group %d: %d\n", dev_name(dev), group->id, ret); > + return ret; > } > EXPORT_SYMBOL_GPL(iommu_group_add_device); > It seems like this cleanup statement in iommu_group_remove_device() should also be done here under err_put_group: sysfs_remove_link(group->devices_kobj, device->name); Ben. -- Ben Hutchings Software Developer, Codethink Ltd.