Add a debugfs directory per pair of {device, pasid} if the mappings of
its page table are created and destroyed by the iommu_map/unmap()
interfaces. i.e. /sys/kernel/debug/iommu/intel/<device source id>/<pasid>.
Create a debugfs file in the directory for users to dump the page
table corresponding to {device, pasid}. e.g.
/sys/kernel/debug/iommu/intel/0000:00:02.0/1/domain_translation_struct.
For the default domain without pasid, it creates a debugfs file in the
debugfs device directory for users to dump its page table. e.g.
/sys/kernel/debug/iommu/intel/0000:00:02.0/domain_translation_struct.
When setting a domain to a PASID of device, create a debugfs file in
the pasid debugfs directory for users to dump the page table of the
specified pasid. Remove the debugfs device directory of the device
when releasing a device. e.g.
/sys/kernel/debug/iommu/intel/0000:00:01.0
Signed-off-by: Jingqi Liu <Jingqi.liu@intel.com>
---
drivers/iommu/intel/debugfs.c | 53 +++++++++++++++++++++++++++++++----
drivers/iommu/intel/iommu.c | 7 +++++
drivers/iommu/intel/iommu.h | 14 +++++++++
3 files changed, 69 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/debugfs.c
index 497c1561d3d2..8a18a7be5215 100644
--- a/drivers/iommu/intel/debugfs.c
+++ b/drivers/iommu/intel/debugfs.c
@@ -111,6 +111,8 @@ static const struct iommu_regset iommu_regs_64[] = {
IOMMU_REGSET_ENTRY(VCRSP),
};
+static struct dentry *intel_iommu_debug;
+
static int iommu_regset_show(struct seq_file *m, void *unused)
{
struct dmar_drhd_unit *drhd;
@@ -671,16 +673,12 @@ static const struct file_operations dmar_perf_latency_fops = {
void __init intel_iommu_debugfs_init(void)
{
- struct dentry *intel_iommu_debug = debugfs_create_dir("intel",
- iommu_debugfs_dir);
+ intel_iommu_debug = debugfs_create_dir("intel", iommu_debugfs_dir);
debugfs_create_file("iommu_regset", 0444, intel_iommu_debug, NULL,
&iommu_regset_fops);
debugfs_create_file("dmar_translation_struct", 0444, intel_iommu_debug,
NULL, &dmar_translation_struct_fops);
- debugfs_create_file("domain_translation_struct", 0444,
- intel_iommu_debug, NULL,
- &domain_translation_struct_fops);
debugfs_create_file("invalidation_queue", 0444, intel_iommu_debug,
NULL, &invalidation_queue_fops);
#ifdef CONFIG_IRQ_REMAP
@@ -690,3 +688,48 @@ void __init intel_iommu_debugfs_init(void)
debugfs_create_file("dmar_perf_latency", 0644, intel_iommu_debug,
NULL, &dmar_perf_latency_fops);
}
+
+/*
+ * Create a debugfs directory for each device, and then create a
+ * debugfs file in this directory for users to dump the page table
+ * of the default domain. e.g.
+ * /sys/kernel/debug/iommu/intel/0000:00:01.0/domain_translation_struct
+ */
+void intel_iommu_debugfs_create_dev(struct device_domain_info *info)
+{
+ info->debugfs_dentry = debugfs_create_dir(dev_name(info->dev), intel_iommu_debug);
+
+ debugfs_create_file("domain_translation_struct", 0444, info->debugfs_dentry,
+ NULL, &domain_translation_struct_fops);
+}
+
+/* Remove the device debugfs directory. */
+void intel_iommu_debugfs_remove_dev(struct device_domain_info *info)
+{
+ debugfs_remove_recursive(info->debugfs_dentry);
+}
+
+/*
+ * Create a debugfs directory per pair of {device, pasid}, then create the
+ * corresponding debugfs file in this directory for users to dump its page
+ * table. e.g.
+ * /sys/kernel/debug/iommu/intel/0000:00:01.0/1/domain_translation_struct
+ *
+ * The debugfs only dumps the page tables whose mappings are created and
+ * destroyed by the iommu_map/unmap() interfaces. Check the mapping type
+ * of the domain before creating debugfs directory.
+ */
+void intel_iommu_debugfs_create_dev_pasid(struct dev_pasid_info *dev_pasid)
+{
+ struct device_domain_info *info = dev_iommu_priv_get(dev_pasid->dev);
+ char dir_name[10];
+
+ sprintf(dir_name, "%x", dev_pasid->pasid);
+ dev_pasid->debugfs_dentry = debugfs_create_dir(dir_name, info->debugfs_dentry);
+}
+
+/* Remove the device pasid debugfs directory. */
+void intel_iommu_debugfs_remove_dev_pasid(struct dev_pasid_info *dev_pasid)
+{
+ debugfs_remove_recursive(dev_pasid->debugfs_dentry);
+}
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 3685ba90ec88..d77d60bc418f 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4409,6 +4409,8 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)
}
}
+ intel_iommu_debugfs_create_dev(info);
+
return &iommu->iommu;
}
@@ -4418,6 +4420,7 @@ static void intel_iommu_release_device(struct device *dev)
dmar_remove_one_dev_info(dev);
intel_pasid_free_table(dev);
+ intel_iommu_debugfs_remove_dev(info);
dev_iommu_priv_set(dev, NULL);
kfree(info);
set_dma_ops(dev, NULL);
@@ -4710,6 +4713,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
spin_unlock_irqrestore(&dmar_domain->lock, flags);
domain_detach_iommu(dmar_domain, iommu);
+ intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
kfree(dev_pasid);
out_tear_down:
intel_pasid_tear_down_entry(iommu, dev, pasid, false);
@@ -4762,6 +4766,9 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
spin_unlock_irqrestore(&dmar_domain->lock, flags);
+ if (domain->type & __IOMMU_DOMAIN_PAGING)
+ intel_iommu_debugfs_create_dev_pasid(dev_pasid);
+
return 0;
out_detach_iommu:
domain_detach_iommu(dmar_domain, iommu);
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 7dac94f62b4e..f9c1dd1ccff8 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -716,12 +716,18 @@ struct device_domain_info {
struct intel_iommu *iommu; /* IOMMU used by this device */
struct dmar_domain *domain; /* pointer to domain */
struct pasid_table *pasid_table; /* pasid table */
+#ifdef CONFIG_INTEL_IOMMU_DEBUGFS
+ struct dentry *debugfs_dentry; /* pointer to device directory dentry */
+#endif
};
struct dev_pasid_info {
struct list_head link_domain; /* link to domain siblings */
struct device *dev;
ioasid_t pasid;
+#ifdef CONFIG_INTEL_IOMMU_DEBUGFS
+ struct dentry *debugfs_dentry; /* pointer to pasid directory dentry */
+#endif
};
static inline void __iommu_flush_cache(
@@ -883,8 +889,16 @@ static inline void intel_svm_remove_dev_pasid(struct device *dev, ioasid_t pasid
#ifdef CONFIG_INTEL_IOMMU_DEBUGFS
void intel_iommu_debugfs_init(void);
+void intel_iommu_debugfs_create_dev(struct device_domain_info *info);
+void intel_iommu_debugfs_remove_dev(struct device_domain_info *info);
+void intel_iommu_debugfs_create_dev_pasid(struct dev_pasid_info *dev_pasid);
+void intel_iommu_debugfs_remove_dev_pasid(struct dev_pasid_info *dev_pasid);
#else
static inline void intel_iommu_debugfs_init(void) {}
+static inline void intel_iommu_debugfs_create_dev(struct device_domain_info *info) {}
+static inline void intel_iommu_debugfs_remove_dev(struct device_domain_info *info) {}
+static inline void intel_iommu_debugfs_create_dev_pasid(struct dev_pasid_info *dev_pasid) {}
+static inline void intel_iommu_debugfs_remove_dev_pasid(struct dev_pasid_info *dev_pasid) {}
#endif /* CONFIG_INTEL_IOMMU_DEBUGFS */
extern const struct attribute_group *intel_iommu_groups[];
--
2.21.3Op 13-10-2023 om 15:58 schreef Jingqi Liu:
> Add a debugfs directory per pair of {device, pasid} if the mappings of
> its page table are created and destroyed by the iommu_map/unmap()
> interfaces. i.e. /sys/kernel/debug/iommu/intel/<device source id>/<pasid>.
> Create a debugfs file in the directory for users to dump the page
> table corresponding to {device, pasid}. e.g.
> /sys/kernel/debug/iommu/intel/0000:00:02.0/1/domain_translation_struct.
> For the default domain without pasid, it creates a debugfs file in the
> debugfs device directory for users to dump its page table. e.g.
> /sys/kernel/debug/iommu/intel/0000:00:02.0/domain_translation_struct.
>
> When setting a domain to a PASID of device, create a debugfs file in
> the pasid debugfs directory for users to dump the page table of the
> specified pasid. Remove the debugfs device directory of the device
> when releasing a device. e.g.
> /sys/kernel/debug/iommu/intel/0000:00:01.0
>
> Signed-off-by: Jingqi Liu <Jingqi.liu@intel.com>
> ---
> drivers/iommu/intel/debugfs.c | 53 +++++++++++++++++++++++++++++++----
> drivers/iommu/intel/iommu.c | 7 +++++
> drivers/iommu/intel/iommu.h | 14 +++++++++
> 3 files changed, 69 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/debugfs.c
> [...]
> +/* Remove the device pasid debugfs directory. */
> +void intel_iommu_debugfs_remove_dev_pasid(struct dev_pasid_info *dev_pasid)
> +{
> + debugfs_remove_recursive(dev_pasid->debugfs_dentry);
> +}
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> [...]
> @@ -4710,6 +4713,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
> spin_unlock_irqrestore(&dmar_domain->lock, flags);
>
> domain_detach_iommu(dmar_domain, iommu);
> + intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
> kfree(dev_pasid);
> out_tear_down:
> intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>
So, a call to intel_iommu_debugfs_remove_dev_pasid() was added.
There is a potential problem that dev_pasid can be NULL.
The diff doesn't show the whole context so let me give that here.
Today that piece of the code looks like this
list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
if (curr->dev == dev && curr->pasid == pasid) {
list_del(&curr->link_domain);
dev_pasid = curr;
break;
}
}
WARN_ON_ONCE(!dev_pasid);
spin_unlock_irqrestore(&dmar_domain->lock, flags);
cache_tag_unassign_domain(dmar_domain, dev, pasid);
domain_detach_iommu(dmar_domain, iommu);
intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
kfree(dev_pasid);
The for_each loop can exit without finding an entry.
The WARN_ON_ONCE also suggests that there can be a NULL.
After that the new function is called (see above) and it will
dereference the NULL pointer.
Can you have a closer look?
--
Kees Bakker
On 11/13/24 05:22, Kees Bakker wrote:
> Op 13-10-2023 om 15:58 schreef Jingqi Liu:
>> Add a debugfs directory per pair of {device, pasid} if the mappings of
>> its page table are created and destroyed by the iommu_map/unmap()
>> interfaces. i.e. /sys/kernel/debug/iommu/intel/<device source id>/
>> <pasid>.
>> Create a debugfs file in the directory for users to dump the page
>> table corresponding to {device, pasid}. e.g.
>> /sys/kernel/debug/iommu/intel/0000:00:02.0/1/domain_translation_struct.
>> For the default domain without pasid, it creates a debugfs file in the
>> debugfs device directory for users to dump its page table. e.g.
>> /sys/kernel/debug/iommu/intel/0000:00:02.0/domain_translation_struct.
>>
>> When setting a domain to a PASID of device, create a debugfs file in
>> the pasid debugfs directory for users to dump the page table of the
>> specified pasid. Remove the debugfs device directory of the device
>> when releasing a device. e.g.
>> /sys/kernel/debug/iommu/intel/0000:00:01.0
>>
>> Signed-off-by: Jingqi Liu <Jingqi.liu@intel.com>
>> ---
>> drivers/iommu/intel/debugfs.c | 53 +++++++++++++++++++++++++++++++----
>> drivers/iommu/intel/iommu.c | 7 +++++
>> drivers/iommu/intel/iommu.h | 14 +++++++++
>> 3 files changed, 69 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/
>> debugfs.c
>> [...]
>> +/* Remove the device pasid debugfs directory. */
>> +void intel_iommu_debugfs_remove_dev_pasid(struct dev_pasid_info
>> *dev_pasid)
>> +{
>> + debugfs_remove_recursive(dev_pasid->debugfs_dentry);
>> +}
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> [...]
>> @@ -4710,6 +4713,7 @@ static void intel_iommu_remove_dev_pasid(struct
>> device *dev, ioasid_t pasid)
>> spin_unlock_irqrestore(&dmar_domain->lock, flags);
>> domain_detach_iommu(dmar_domain, iommu);
>> + intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
>> kfree(dev_pasid);
>> out_tear_down:
>> intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>>
>
> So, a call to intel_iommu_debugfs_remove_dev_pasid() was added.
> There is a potential problem that dev_pasid can be NULL.
> The diff doesn't show the whole context so let me give that here.
> Today that piece of the code looks like this
>
> list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
> if (curr->dev == dev && curr->pasid == pasid) {
> list_del(&curr->link_domain);
> dev_pasid = curr;
> break;
> }
> }
> WARN_ON_ONCE(!dev_pasid);
> spin_unlock_irqrestore(&dmar_domain->lock, flags);
>
> cache_tag_unassign_domain(dmar_domain, dev, pasid);
> domain_detach_iommu(dmar_domain, iommu);
> intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
> kfree(dev_pasid);
>
> The for_each loop can exit without finding an entry.
> The WARN_ON_ONCE also suggests that there can be a NULL.
> After that the new function is called (see above) and it will
> dereference the NULL pointer.
>
> Can you have a closer look?
It's already a kernel bug if dev_pasid is NULL when it reaches here. If
that happens, we should fix the bug, not avoid using the NULL pointer.
--
baolu
Op 13-11-2024 om 03:13 schreef Baolu Lu:
> On 11/13/24 05:22, Kees Bakker wrote:
>> Op 13-10-2023 om 15:58 schreef Jingqi Liu:
>>> Add a debugfs directory per pair of {device, pasid} if the mappings of
>>> its page table are created and destroyed by the iommu_map/unmap()
>>> interfaces. i.e. /sys/kernel/debug/iommu/intel/<device source id>/
>>> <pasid>.
>>> Create a debugfs file in the directory for users to dump the page
>>> table corresponding to {device, pasid}. e.g.
>>> /sys/kernel/debug/iommu/intel/0000:00:02.0/1/domain_translation_struct.
>>> For the default domain without pasid, it creates a debugfs file in the
>>> debugfs device directory for users to dump its page table. e.g.
>>> /sys/kernel/debug/iommu/intel/0000:00:02.0/domain_translation_struct.
>>>
>>> When setting a domain to a PASID of device, create a debugfs file in
>>> the pasid debugfs directory for users to dump the page table of the
>>> specified pasid. Remove the debugfs device directory of the device
>>> when releasing a device. e.g.
>>> /sys/kernel/debug/iommu/intel/0000:00:01.0
>>>
>>> Signed-off-by: Jingqi Liu <Jingqi.liu@intel.com>
>>> ---
>>> drivers/iommu/intel/debugfs.c | 53
>>> +++++++++++++++++++++++++++++++----
>>> drivers/iommu/intel/iommu.c | 7 +++++
>>> drivers/iommu/intel/iommu.h | 14 +++++++++
>>> 3 files changed, 69 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/
>>> debugfs.c
>>> [...]
>>> +/* Remove the device pasid debugfs directory. */
>>> +void intel_iommu_debugfs_remove_dev_pasid(struct dev_pasid_info
>>> *dev_pasid)
>>> +{
>>> + debugfs_remove_recursive(dev_pasid->debugfs_dentry);
>>> +}
>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>> [...]
>>> @@ -4710,6 +4713,7 @@ static void
>>> intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>>> spin_unlock_irqrestore(&dmar_domain->lock, flags);
>>> domain_detach_iommu(dmar_domain, iommu);
>>> + intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
>>> kfree(dev_pasid);
>>> out_tear_down:
>>> intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>>>
>>
>> So, a call to intel_iommu_debugfs_remove_dev_pasid() was added.
>> There is a potential problem that dev_pasid can be NULL.
>> The diff doesn't show the whole context so let me give that here.
>> Today that piece of the code looks like this
>>
>> list_for_each_entry(curr, &dmar_domain->dev_pasids,
>> link_domain) {
>> if (curr->dev == dev && curr->pasid == pasid) {
>> list_del(&curr->link_domain);
>> dev_pasid = curr;
>> break;
>> }
>> }
>> WARN_ON_ONCE(!dev_pasid);
>> spin_unlock_irqrestore(&dmar_domain->lock, flags);
>>
>> cache_tag_unassign_domain(dmar_domain, dev, pasid);
>> domain_detach_iommu(dmar_domain, iommu);
>> intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
>> kfree(dev_pasid);
>>
>> The for_each loop can exit without finding an entry.
>> The WARN_ON_ONCE also suggests that there can be a NULL.
>> After that the new function is called (see above) and it will
>> dereference the NULL pointer.
>>
>> Can you have a closer look?
>
> It's already a kernel bug if dev_pasid is NULL when it reaches here. If
> that happens, we should fix the bug, not avoid using the NULL pointer.
How about moving the WARN_ON_ONCE down a bit and use its return value?
Like so
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 527f6f89d8a1..204873976ef3 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4096,13 +4096,14 @@ void domain_remove_dev_pasid(struct iommu_domain
*domain,
break;
}
}
- WARN_ON_ONCE(!dev_pasid);
spin_unlock_irqrestore(&dmar_domain->lock, flags);
cache_tag_unassign_domain(dmar_domain, dev, pasid);
domain_detach_iommu(dmar_domain, iommu);
- intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
- kfree(dev_pasid);
+ if (!WARN_ON_ONCE(!dev_pasid)) {
+ intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
+ kfree(dev_pasid);
+ }
}
static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t
pasid,
Would you accept a patch like that? Or do you want to change it yourself?
--
Kees
On 11/14/24 04:35, Kees Bakker wrote:
> Op 13-11-2024 om 03:13 schreef Baolu Lu:
>> On 11/13/24 05:22, Kees Bakker wrote:
>>> Op 13-10-2023 om 15:58 schreef Jingqi Liu:
>>>> Add a debugfs directory per pair of {device, pasid} if the mappings of
>>>> its page table are created and destroyed by the iommu_map/unmap()
>>>> interfaces. i.e. /sys/kernel/debug/iommu/intel/<device source id>/
>>>> <pasid>.
>>>> Create a debugfs file in the directory for users to dump the page
>>>> table corresponding to {device, pasid}. e.g.
>>>> /sys/kernel/debug/iommu/intel/0000:00:02.0/1/domain_translation_struct.
>>>> For the default domain without pasid, it creates a debugfs file in the
>>>> debugfs device directory for users to dump its page table. e.g.
>>>> /sys/kernel/debug/iommu/intel/0000:00:02.0/domain_translation_struct.
>>>>
>>>> When setting a domain to a PASID of device, create a debugfs file in
>>>> the pasid debugfs directory for users to dump the page table of the
>>>> specified pasid. Remove the debugfs device directory of the device
>>>> when releasing a device. e.g.
>>>> /sys/kernel/debug/iommu/intel/0000:00:01.0
>>>>
>>>> Signed-off-by: Jingqi Liu <Jingqi.liu@intel.com>
>>>> ---
>>>> drivers/iommu/intel/debugfs.c | 53 ++++++++++++++++++++++++++++++
>>>> +----
>>>> drivers/iommu/intel/iommu.c | 7 +++++
>>>> drivers/iommu/intel/iommu.h | 14 +++++++++
>>>> 3 files changed, 69 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/
>>>> debugfs.c
>>>> [...]
>>>> +/* Remove the device pasid debugfs directory. */
>>>> +void intel_iommu_debugfs_remove_dev_pasid(struct dev_pasid_info
>>>> *dev_pasid)
>>>> +{
>>>> + debugfs_remove_recursive(dev_pasid->debugfs_dentry);
>>>> +}
>>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>>> [...]
>>>> @@ -4710,6 +4713,7 @@ static void
>>>> intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>>>> spin_unlock_irqrestore(&dmar_domain->lock, flags);
>>>> domain_detach_iommu(dmar_domain, iommu);
>>>> + intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
>>>> kfree(dev_pasid);
>>>> out_tear_down:
>>>> intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>>>>
>>>
>>> So, a call to intel_iommu_debugfs_remove_dev_pasid() was added.
>>> There is a potential problem that dev_pasid can be NULL.
>>> The diff doesn't show the whole context so let me give that here.
>>> Today that piece of the code looks like this
>>>
>>> list_for_each_entry(curr, &dmar_domain->dev_pasids,
>>> link_domain) {
>>> if (curr->dev == dev && curr->pasid == pasid) {
>>> list_del(&curr->link_domain);
>>> dev_pasid = curr;
>>> break;
>>> }
>>> }
>>> WARN_ON_ONCE(!dev_pasid);
>>> spin_unlock_irqrestore(&dmar_domain->lock, flags);
>>>
>>> cache_tag_unassign_domain(dmar_domain, dev, pasid);
>>> domain_detach_iommu(dmar_domain, iommu);
>>> intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
>>> kfree(dev_pasid);
>>>
>>> The for_each loop can exit without finding an entry.
>>> The WARN_ON_ONCE also suggests that there can be a NULL.
>>> After that the new function is called (see above) and it will
>>> dereference the NULL pointer.
>>>
>>> Can you have a closer look?
>>
>> It's already a kernel bug if dev_pasid is NULL when it reaches here. If
>> that happens, we should fix the bug, not avoid using the NULL pointer.
>
> How about moving the WARN_ON_ONCE down a bit and use its return value?
> Like so
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 527f6f89d8a1..204873976ef3 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4096,13 +4096,14 @@ void domain_remove_dev_pasid(struct iommu_domain
> *domain,
> break;
> }
> }
> - WARN_ON_ONCE(!dev_pasid);
> spin_unlock_irqrestore(&dmar_domain->lock, flags);
>
> cache_tag_unassign_domain(dmar_domain, dev, pasid);
> domain_detach_iommu(dmar_domain, iommu);
> - intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
> - kfree(dev_pasid);
> + if (!WARN_ON_ONCE(!dev_pasid)) {
> + intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
> + kfree(dev_pasid);
> + }
> }
>
> static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t
> pasid,
>
>
> Would you accept a patch like that? Or do you want to change it yourself?
Yes. That looks better. Please post a patch. Thanks!
--
baolu
© 2016 - 2025 Red Hat, Inc.