Received-SPF: pass (google.com: domain of linux-kernel+bounces-28446-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <46a61123-dd22-46cb-8b2a-15431fcc03f1@arm.com>
Date: Wed, 17 Jan 2024 01:57:34 +0000
Precedence: bulk
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 <jgg@ziepe.ca>, Eric Badger <ebadger@purestorage.com>
Cc: David Woodhouse <dwmw2@infradead.org>, Lu Baolu
 <baolu.lu@linux.intel.com>, Joerg Roedel <joro@8bytes.org>,
 Will Deacon <will@kernel.org>,
 "open list:INTEL IOMMU (VT-d)" <iommu@lists.linux.dev>,
 open list <linux-kernel@vger.kernel.org>
References: <20240113181713.1817855-1-ebadger@purestorage.com>
 <20240116152215.GE50608@ziepe.ca>
From: Robin Murphy <robin.murphy@arm.com>
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 <ebadger@purestorage.com>
> 
> 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.