From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.marcansoft.com (marcansoft.com [212.63.210.85]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B49FC2031F; Thu, 23 Nov 2023 09:08:55 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=marcan.st Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=marcan.st Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=marcan.st header.i=@marcan.st header.b="bEylMheX" Received: from [127.0.0.1] (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: marcan@marcan.st) by mail.marcansoft.com (Postfix) with ESMTPSA id 9BDB241EF0; Thu, 23 Nov 2023 09:08:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=marcan.st; s=default; t=1700730528; bh=tKvmM8VauJ2SKlC76Ts+IMS9Y0f1D93gbN6PS2aS1RQ=; h=Date:Subject:To:Cc:References:From:In-Reply-To; b=bEylMheXkP1AY7uDhyUgpvOvmafAgnH8kYOyGsGvzSEm3AG0TuKGO12XpHG7pIpPk 7DWjDpQ/2UVGkEBlWmpkMsK+kkFSTbFl2mNZf+evyauyJJjLkxYiIMvQnQZccc7GYl eyA+fRFKB/4nH2lfKLFZ7jOWf+ZnhEEAdxplIRYiCSC8oH9YCENIvFOPHr97tQOVpS vFMyIE8sCHGbUWmMQTW6+kNNQZA2kAYPQ/d2k1mzDzQUzP4vU581SpL8b4NQXqoDyL Nq2SuV6KB6PtAtBepsn6kZamgJlbVvwi56sMxTDqNBWGfHkerXitbj24KAHWAlJH6q nnb+VrbjJRB5A== Message-ID: Date: Thu, 23 Nov 2023 18:08:33 +0900 Precedence: bulk X-Mailing-List: patches@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 06/17] iommu: Add iommu_fwspec_alloc/dealloc() Content-Language: en-US To: Jason Gunthorpe Cc: acpica-devel@lists.linux.dev, Alyssa Rosenzweig , Albert Ou , asahi@lists.linux.dev, Catalin Marinas , Dexuan Cui , devicetree@vger.kernel.org, David Woodhouse , Frank Rowand , Hanjun Guo , Haiyang Zhang , iommu@lists.linux.dev, Jean-Philippe Brucker , Jonathan Hunter , Joerg Roedel , "K. Y. Srinivasan" , Len Brown , linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-hyperv@vger.kernel.org, linux-mips@vger.kernel.org, linux-riscv@lists.infradead.org, linux-snps-arc@lists.infradead.org, linux-tegra@vger.kernel.org, Russell King , Lorenzo Pieralisi , Marek Szyprowski , Palmer Dabbelt , patches@lists.linux.dev, Paul Walmsley , "Rafael J. Wysocki" , Robert Moore , Rob Herring , Robin Murphy , Sudeep Holla , Suravee Suthikulpanit , Sven Peter , Thierry Reding , Thomas Bogendoerfer , Krishna Reddy , Vineet Gupta , virtualization@lists.linux.dev, Wei Liu , Will Deacon References: <6-v2-36a0088ecaa7+22c6e-iommu_fwspec_jgg@nvidia.com> <20a7ef6d-a8ca-4bd8-ad7e-11856db617a2@marcan.st> <1eb12c35-e64e-4c32-af99-8743dc2ec266@marcan.st> <20231119141329.GA6083@nvidia.com> <90855bbf-e845-4e4d-a713-df71d1e477d2@marcan.st> <20231121160058.GG6083@nvidia.com> From: Hector Martin In-Reply-To: <20231121160058.GG6083@nvidia.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 2023/11/22 1:00, Jason Gunthorpe wrote: > On Tue, Nov 21, 2023 at 03:47:48PM +0900, Hector Martin wrote: >>> Which is sensitive only to !NULL fwspec, and if EPROBE_DEFER is >>> returned fwspec will be freed and dev->iommu->fwspec will be NULL >>> here. >>> >>> In the NULL case it does a 'bus probe' with a NULL fwspec and all the >>> fwspec drivers return immediately from their probe functions. >>> >>> Did I miss something? >> >> apple_dart is not a fwspec driver and doesn't do that :-) > > It implements of_xlate that makes it a driver using the fwspec probe > path. > > The issue is in apple-dart. Its logic for avoiding bus probe vs > fwspec probe is not correct. > > It does: > > static int apple_dart_of_xlate(struct device *dev, struct of_phandle_args *args) > { > [..] > dev_iommu_priv_set(dev, cfg); > > > Then: > > static struct iommu_device *apple_dart_probe_device(struct device *dev) > { > struct apple_dart_master_cfg *cfg = dev_iommu_priv_get(dev); > struct apple_dart_stream_map *stream_map; > int i; > > if (!cfg) > return ERR_PTR(-ENODEV); > > Which leaks the cfg memory on rare error cases and wrongly allows the > driver to probe without a fwspec, which I think is what you are > hitting. > > It should be > > if (!dev_iommu_fwspec_get(dev) || !cfg) > return ERR_PTR(-ENODEV); > > To ensure the driver never probes on the bus path. > > Clearing the dev->iommu in the core code has the side effect of > clearing (and leaking) the cfg which would hide this issue. Aha! Yes, that makes it work with only the first change. I'll throw the apple-dart fix into our tree (and submit it once I get to clearing out the branch; the affected consumer driver isn't upstream yet so this isn't particularly urgent). - Hector