From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 154981385 for ; Wed, 17 Jan 2024 01:57:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1705456661; cv=none; b=W7NJMc5XZG/Z48b7tDlN/f4m4qfpUTglwqaki8YzDQ6rzK8bycu7ZKaQ4FFRerDUR/MQgMc+y5rFQYRpsgLMa0IDkvTsOnM/74N8LH2WzMAPwrPHiLaAdaUwLJEtjvzHhKmhCYs/RDKmzkBcm4VYeYinEmImEgsI3nuSLYlnbYs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1705456661; c=relaxed/simple; bh=/SrlWjIMTigyESjz1QsaSHrVg3bHKGkzoomgx993OW4=; h=Received:Received:Message-ID:Date:MIME-Version:User-Agent:Subject: Content-Language:To:Cc:References:From:In-Reply-To:Content-Type: Content-Transfer-Encoding; b=de8z+OQS+XBne0PXdqtQtFHZ5joXQYpXZ9Wn3BEy0BwTeQ0/f7EDQXaKByKbB/haDpfLvB0tNVBzgwQZduJ1gTH3qLQIT5ekaObxeqccJgVvLGUZ/y8xijm3vTjjybmsBYCpQ/434fLFowgZkV8LOAr7mEwwijqwyK3YiYoPg5Y= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8D5182F4; Tue, 16 Jan 2024 17:58:24 -0800 (PST) Received: from [10.57.46.46] (unknown [10.57.46.46]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E4D763F5A1; Tue, 16 Jan 2024 17:57:36 -0800 (PST) Message-ID: <46a61123-dd22-46cb-8b2a-15431fcc03f1@arm.com> Date: Wed, 17 Jan 2024 01:57:34 +0000 Precedence: bulk X-Mailing-List: iommu@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] iommu/vt-d: Check for non-NULL domain on device release Content-Language: en-GB To: Jason Gunthorpe , Eric Badger Cc: David Woodhouse , Lu Baolu , Joerg Roedel , Will Deacon , "open list:INTEL IOMMU (VT-d)" , open list References: <20240113181713.1817855-1-ebadger@purestorage.com> <20240116152215.GE50608@ziepe.ca> From: Robin Murphy In-Reply-To: <20240116152215.GE50608@ziepe.ca> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 2024-01-16 3:22 pm, Jason Gunthorpe wrote: > On Sat, Jan 13, 2024 at 10:17:13AM -0800, Eric Badger wrote: >> In the kdump kernel, the IOMMU will operate in deferred_attach mode. In >> this mode, info->domain may not yet be assigned by the time the >> release_device function is called, which leads to the following crash in >> the crashkernel: > > This never worked right? But why are you getting to release in a crash > kernel in the first place, that seems kinda weird.. > >> BUG: kernel NULL pointer dereference, address: 000000000000003c >> ... >> RIP: 0010:do_raw_spin_lock+0xa/0xa0 >> ... >> _raw_spin_lock_irqsave+0x1b/0x30 >> intel_iommu_release_device+0x96/0x170 >> iommu_deinit_device+0x39/0xf0 >> __iommu_group_remove_device+0xa0/0xd0 >> iommu_bus_notifier+0x55/0xb0 >> notifier_call_chain+0x5a/0xd0 >> blocking_notifier_call_chain+0x41/0x60 >> bus_notify+0x34/0x50 >> device_del+0x269/0x3d0 >> pci_remove_bus_device+0x77/0x100 >> p2sb_bar+0xae/0x1d0 >> ... >> i801_probe+0x423/0x740 >> >> Signed-off-by: Eric Badger > > It should have a fixes line, but I'm not sure what it is.. > >> --- >> drivers/iommu/intel/iommu.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) > > Unfortunately this issue is likely systemic in all the drivers. All two of the drivers which support deferred attach, that is. > Something I've been thinking about for a while now is to have the > option for the core code release to set the driver to a specific > releasing domain (probably a blocking or identity global static) > > A lot of drivers are open coding this internally in their release > functions, like Intel. But it really doesn't deserve to be special. Either way it shouldn't apply in this case, though. Crash kernels *are* special. The whole point of deferred attach is that we don't disturb anything unless we've got as far as definitely using a given default domain (from which we can infer the relevant client device should have been reset and quiesced). Thus regardless of why release might get called, if a deferred attach never happened then the release should still avoid touching any hardware configuration either. I'd suggest using dev->iommu->attach_deferred as the condition rather than a NULL domain, to be clear that it's the *only* valid reason to not have a domain at this point. Thanks, Robin.