| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3931328b-2470-9cb7-ef87-5d0fdeb58142@intel.com>
Date: Mon, 25 Sep 2023 20:03:13 +0800
From: "Liu, Jingqi" <jingqi.liu@...el.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>, <iommu@...ts.linux.dev>,
Tian Kevin <kevin.tian@...el.com>,
Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
Robin Murphy <robin.murphy@....com>
CC: <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] iommu/vt-d: debugfs: Create/remove debugfs file
per {device, pasid}
On 9/25/2023 9:47 AM, Baolu Lu wrote:
> On 9/22/23 11:16 PM, Jingqi Liu wrote:
>> 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/0/domain_translation_struct.
>>
>> When attaching device without pasid, create a debugfs file with
>> PASID#0, i.e. RID_PASID. When attaching a domain to a pasid of device,
>> create a debugfs file with the specified pasid.
>>
>> When detaching without pasid, remove the directory and file for
>> PASID#0. When detaching with pasid, remove the debugfs directory and
>> file of the specified pasid. Remove the entire debugfs directory of
>> the specified device for releasing device.
>> e.g. /sys/kernel/debug/iommu/intel/0000:00:01.0
>>
>> Signed-off-by: Jingqi Liu <Jingqi.liu@...el.com>
>> ---
>> drivers/iommu/intel/debugfs.c | 117 ++++++++++++++++++++++++++++++++--
>> drivers/iommu/intel/iommu.c | 16 +++++
>> drivers/iommu/intel/iommu.h | 4 ++
>> 3 files changed, 132 insertions(+), 5 deletions(-)
>>
>>
......
>> +
>> +/*
>> + * Create a debugfs directory per pair of {device, pasid},
>> + * then create the corresponding debugfs file in this directory
>> + * for user to dump its page table. e.g.
>> + *
>> /sys/kernel/debug/iommu/intel/0000:00:01.0/0/domain_translation_struct
>> + */
>> +void intel_iommu_debugfs_create_dev_pasid(struct device *dev, u32
>> pasid)
>> +{
>> + struct dentry *pasid_dir = NULL, *dev_dir = NULL;
>> + struct iommu_domain *domain = NULL;
>> + struct show_domain_info *sinfo;
>> + char pname[10];
>> +
>> + if (pasid == IOMMU_NO_PASID) {
>> + struct device_domain_info *info = dev_iommu_priv_get(dev);
>> + if (!info)
>> + return;
>> + domain = &info->domain->domain;
>> + } else
>> + domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
>
> Can you just add domain as a parameter to this helper? This helper is
> probably called in the process of attaching. so the domain pointers may
> not be initialized yet.
>
Thanks.
Yes. This helper may be called before domain is initialized.
I'll add 'domain' as a parameter, and remove the check code above.
Check if the 'domain' is valid as the following code.
>> +
>> + /*
>> + * 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.
>> + */
>> + if (!domain || !(domain->type & __IOMMU_DOMAIN_PAGING))
>> + return;
>> +
>> + dev_dir = debugfs_lookup(dev_name(dev), intel_iommu_debug);
>
> The comments of debugfs_lookup says:
>
> /**
> * debugfs_lookup() - look up an existing debugfs file
> * @name: a pointer to a string containing the name of the file to
> look up.
> * @parent: a pointer to the parent dentry of the file.
> *
> * This function will return a pointer to a dentry if it succeeds. If
> the file
> * doesn't exist or an error occurs, %NULL will be returned. The
> returned
> * dentry must be passed to dput() when it is no longer needed.
> *
> * If debugfs is not enabled in the kernel, the value -%ENODEV will be
> * returned.
> */
>
> where is the paired dput()?
>
> Ditto to all debugfs_lookup() calls in this file.
>
Good catch.
Need to 'dput()' the entry before returning from the helper.
I'll revise it.
......
>> +
>> +/*
>> + * Remove the debugfs directory and file corresponding to each pair of
>> + * {device, pasid}. There're two scenarios that will call this helper:
>> + * 1) Detach the specified device with pasid.
>> + * 2) IOMMU release a device.
>> + */
>> +void intel_iommu_debugfs_remove_dev_pasid(struct device *dev, u32
>> pasid)
>> +{
>> + struct dentry *pasid_dir = NULL, *dev_dir = NULL;
>> + struct dentry *dentry = NULL;
>> + char pname[10];
>> +
>> + dev_dir = debugfs_lookup(dev_name(dev), intel_iommu_debug);
>> + if (!dev_dir)
>> + return;
>> +
>> + /* Check if the entire debugfs directory needs to be removed. */
>> + if (pasid == IOMMU_PASID_INVALID) {
>> + struct list_head *plist;
>> + struct dentry *sub_dentry;
>> +
>> + list_for_each(plist, &(dev_dir->d_subdirs)) {
>> + sub_dentry = list_entry(plist, struct dentry, d_child);
>> + if(sub_dentry) {
>> + dentry = debugfs_lookup("domain_translation_struct",
>> sub_dentry);
>> + if (!dentry)
>> + continue;
>> +
>> + if (dentry->d_inode->i_private)
>> + kfree(dentry->d_inode->i_private);
>> + }
>> + }
>> +
>> + debugfs_remove_recursive(dev_dir);
>> + } else { /* Remove the directory with specified pasid. */
>> + sprintf(pname, "%x", pasid);
>> + pasid_dir = debugfs_lookup(pname, dev_dir);
>> + if (!pasid_dir)
>> + return;
>> +
>> + dentry = debugfs_lookup("domain_translation_struct",
>> pasid_dir);
>> + if (!dentry)
>> + return;
>> +
>> + if (dentry->d_inode->i_private)
>> + kfree(dentry->d_inode->i_private);
>> +
>> + debugfs_remove_recursive(pasid_dir);
>> + }
>> +}
>
> The above is too complex to review and maintain.
>
> If I were to make the change, I would move the device directory
> management to the IOMMU probe/release devices path, and keep the PASID
> directory management in the domain attaching/detaching paths.
>
> Or I overlooked anything?
Good point.
In this way, the debugfs device directory and pasid directory can be
managed separately.
intel_iommu_probe_device()/intel_iommu_release_device()
is responsible for adding/removing the device directory.
In the domain attaching/detaching paths,
add/remove the PASID directory and the corresponding debugfs file.
I'll implement like this.
Thanks,
Jingqi
Powered by blists - more mailing lists