From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753214AbdHJTrb (ORCPT ); Thu, 10 Aug 2017 15:47:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35238 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752840AbdHJTr2 (ORCPT ); Thu, 10 Aug 2017 15:47:28 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com BDD861A9D9B Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=alex.williamson@redhat.com Date: Thu, 10 Aug 2017 13:47:25 -0600 From: Alex Williamson To: Eric Auger Cc: eric.auger.pro@gmail.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] vfio: fix noiommu vfio_iommu_group_get reference count Message-ID: <20170810134725.13fc5648@w520.home> In-Reply-To: <1502225068-9699-1-git-send-email-eric.auger@redhat.com> References: <1502225068-9699-1-git-send-email-eric.auger@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 10 Aug 2017 19:47:27 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 8 Aug 2017 22:44:28 +0200 Eric Auger wrote: > In vfio_iommu_group_get() we want to increase the reference > count of the iommu group. > > In noiommu case, the group does not exist and is allocated. > iommu_group_add_device() increases the group ref count. However we > then call iommu_group_put() which decrements it. > > This leads to a "refcount_t: underflow WARN_ON". Yep, the group is created with an initial reference count of 1, we then add the device, which increments the reference count. Normally the instantiator of the group would then release the reference, so that only the device reference holds the group. However here we want a reference in addition to the device reference, so we should never have released the initial reference. Seems right, except... > Signed-off-by: Eric Auger > --- > drivers/vfio/vfio.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index 330d505..fd8d691 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -138,7 +138,6 @@ struct iommu_group *vfio_iommu_group_get(struct device *dev) > iommu_group_set_name(group, "vfio-noiommu"); > iommu_group_set_iommudata(group, &noiommu, NULL); > ret = iommu_group_add_device(group, dev); > - iommu_group_put(group); > if (ret) > return NULL; We leak the group in the error case here. Perhaps the 'put' is correct, it was just typo'd outside of the error case. Thanks, Alex