linux-kernel - RE: [PATCH v1] iommu/vt-d: Remove dead code in intel_iommu_domain_alloc_paging

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DS0PR11MB637391ECB166209035638E8EDC6DA@DS0PR11MB6373.namprd11.prod.outlook.com>
Date: Tue, 3 Jun 2025 03:31:04 +0000
From: "Wang, Wei W" <wei.w.wang@...el.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>, "Tian, Kevin" <kevin.tian@...el.com>,
	"Liu, Yi L" <yi.l.liu@...el.com>, "dwmw2@...radead.org"
	<dwmw2@...radead.org>, "jroedel@...e.de" <jroedel@...e.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"iommu@...ts.linux.dev" <iommu@...ts.linux.dev>
Subject: RE: [PATCH v1] iommu/vt-d: Remove dead code in
 intel_iommu_domain_alloc_paging_flags()

On Tuesday, June 3, 2025 10:40 AM, Baolu Lu wrote:
> On 5/30/25 17:13, Wei Wang wrote:
> > When dirty_tracking is enabled, first_stage is set to false to use the
> > second stage translation table. dmar_domain->use_first_level, which is
> > assigned from first_page, is guaranteed to be false when the execution
> > reaches the location of the code to be removed by this patch. So the
> > handling for dmar_domain->use_first_level being true there will never
> > be executed.
> >
> > Signed-off-by: Wei Wang<wei.w.wang@...el.com>
> > ---
> >   drivers/iommu/intel/iommu.c | 7 +------
> >   1 file changed, 1 insertion(+), 6 deletions(-)
> >
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index cb0b993bebb4..1145567c60f9 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -3418,13 +3418,8 @@ intel_iommu_domain_alloc_paging_flags(struct
> device *dev, u32 flags,
> >   		spin_lock_init(&dmar_domain->s1_lock);
> >   	}
> >
> > -	if (dirty_tracking) {
> > -		if (dmar_domain->use_first_level) {
> 
> This *explicit* check enforces that dirty tracking cannot be supported for a
> domain that relies on first-stage translation due to the lack of
> enabling/disabling dirty tracking support.
> 
> While this might appear redundant, this prevents potential issues if related
> code is modified without awareness of this dependency.

Would a warning (i.e., WARN_ON(dmar_domain->use_first_level)) be better here?
(this condition seems unlikely to occur in practice, especially in official kernel releases,
and the current handling logic appears somewhat confusing. If developers add some
new logic that could change dmar_domain->use_first_level to ture (maybe by mistake)
in the future, they could bring the handling back, which might look clearer?)

> 
> > -			iommu_domain_free(domain);
> > -			return ERR_PTR(-EOPNOTSUPP);
> > -		}
> 
> Thanks,
> baolu