[PATCH 0/2] iommu/vt-d: Ensure memory ordering in context & root entry updates

[PATCH 0/2] iommu/vt-d: Ensure memory ordering in context & root entry updates

[Dmytro Maluka]

As discussed in [1], we don't currently prevent the compiler from
reordering memory writes when updating context entries, which is
potentially dangerous, as it may cause setting the present bit (i.e.
enabling DMA translation for the given device) before finishing setting
up other bits in the context entry (and thus creating a time window when
a DMA from the device may result in an unpredicted behavior).

Fix this in the same way as how this is already addressed for PASID
entries, i.e. by using READ_ONCE/WRITE_ONCE in the helpers used for
setting individual bits in context entries, so that memory writes done
by those helpers are ordered in relation to each other (plus, prevent
load/store tearing and so on).

While at it, similarly paranoidally fix updating root entries as well:
use WRITE_ONCE to make sure that the present bit is set atomically
together with the context table address bits, not before them.

[1] https://lore.kernel.org/all/aTG7gc7I5wExai3S@google.com/

Dmytro Maluka (2):
  iommu/vt-d: Ensure memory ordering in context entry updates
  iommu/vt-d: Use WRITE_ONCE for setting root table entries

 drivers/iommu/intel/iommu.c |  2 +-
 drivers/iommu/intel/iommu.h | 37 +++++++++++++++++++++----------------
 drivers/iommu/intel/pasid.c |  3 ++-
 3 files changed, 24 insertions(+), 18 deletions(-)

-- 
2.47.3

[PATCH 1/2] iommu/vt-d: Ensure memory ordering in context entry updates

[Dmytro Maluka]

When updating context table entries, we do take care to set the present
bit as the last step, i.e. in the following order:

	context_clear_entry(context);
	<set other bits>
	context_set_present(context);

However, we don't actually ensure this order, i.e. don't prevent the
compiler from reordering it. And since context entries may be updated at
runtime when translation is already enabled, this may potentially allow
a time window when a device can already do DMA while the translation is
not properly set up yet (e.g. the context entry may point to an
arbitrary page table).

To easily fix this, change context_set_*() and context_clear_*() helpers
to use READ_ONCE/WRITE_ONCE, to ensure that the ordering between updates
of individual bits in context entries matches the order of calling those
helpers, just like we already do for PASID table entries.

Link: https://lore.kernel.org/all/aTG7gc7I5wExai3S@google.com/
Signed-off-by: Dmytro Maluka <dmaluka@chromium.org>
---
 drivers/iommu/intel/iommu.h | 37 +++++++++++++++++++++----------------
 drivers/iommu/intel/pasid.c |  3 ++-
 2 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 25c5e22096d4..7f8f004fa756 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -869,7 +869,7 @@ static inline bool dma_pte_superpage(struct dma_pte *pte)
 
 static inline bool context_present(struct context_entry *context)
 {
-	return (context->lo & 1);
+	return READ_ONCE(context->lo) & 1;
 }
 
 #define LEVEL_STRIDE		(9)
@@ -897,46 +897,51 @@ static inline int pfn_level_offset(u64 pfn, int level)
 	return (pfn >> level_to_offset_bits(level)) & LEVEL_MASK;
 }
 
+static inline void context_set_bits(u64 *ptr, u64 mask, u64 bits)
+{
+	u64 old;
+
+	old = READ_ONCE(*ptr);
+	WRITE_ONCE(*ptr, (old & ~mask) | bits);
+}
 
 static inline void context_set_present(struct context_entry *context)
 {
-	context->lo |= 1;
+	context_set_bits(&context->lo, 1 << 0, 1);
 }
 
 static inline void context_set_fault_enable(struct context_entry *context)
 {
-	context->lo &= (((u64)-1) << 2) | 1;
+	context_set_bits(&context->lo, 1 << 1, 0);
 }
 
 static inline void context_set_translation_type(struct context_entry *context,
 						unsigned long value)
 {
-	context->lo &= (((u64)-1) << 4) | 3;
-	context->lo |= (value & 3) << 2;
+	context_set_bits(&context->lo, GENMASK_ULL(3, 2), value << 2);
 }
 
 static inline void context_set_address_root(struct context_entry *context,
 					    unsigned long value)
 {
-	context->lo &= ~VTD_PAGE_MASK;
-	context->lo |= value & VTD_PAGE_MASK;
+	context_set_bits(&context->lo, VTD_PAGE_MASK, value);
 }
 
 static inline void context_set_address_width(struct context_entry *context,
 					     unsigned long value)
 {
-	context->hi |= value & 7;
+	context_set_bits(&context->hi, GENMASK_ULL(2, 0), value);
 }
 
 static inline void context_set_domain_id(struct context_entry *context,
 					 unsigned long value)
 {
-	context->hi |= (value & ((1 << 16) - 1)) << 8;
+	context_set_bits(&context->hi, GENMASK_ULL(23, 8), value << 8);
 }
 
 static inline void context_set_pasid(struct context_entry *context)
 {
-	context->lo |= CONTEXT_PASIDE;
+	context_set_bits(&context->lo, CONTEXT_PASIDE, CONTEXT_PASIDE);
 }
 
 static inline int context_domain_id(struct context_entry *c)
@@ -946,8 +951,8 @@ static inline int context_domain_id(struct context_entry *c)
 
 static inline void context_clear_entry(struct context_entry *context)
 {
-	context->lo = 0;
-	context->hi = 0;
+	WRITE_ONCE(context->lo, 0);
+	WRITE_ONCE(context->hi, 0);
 }
 
 #ifdef CONFIG_INTEL_IOMMU
@@ -980,7 +985,7 @@ clear_context_copied(struct intel_iommu *iommu, u8 bus, u8 devfn)
 static inline void
 context_set_sm_rid2pasid(struct context_entry *context, unsigned long pasid)
 {
-	context->hi |= pasid & ((1 << 20) - 1);
+	context_set_bits(&context->hi, GENMASK_ULL(19, 0), pasid);
 }
 
 /*
@@ -989,7 +994,7 @@ context_set_sm_rid2pasid(struct context_entry *context, unsigned long pasid)
  */
 static inline void context_set_sm_dte(struct context_entry *context)
 {
-	context->lo |= BIT_ULL(2);
+	context_set_bits(&context->lo, BIT_ULL(2), BIT_ULL(2));
 }
 
 /*
@@ -998,7 +1003,7 @@ static inline void context_set_sm_dte(struct context_entry *context)
  */
 static inline void context_set_sm_pre(struct context_entry *context)
 {
-	context->lo |= BIT_ULL(4);
+	context_set_bits(&context->lo, BIT_ULL(4), BIT_ULL(4));
 }
 
 /*
@@ -1007,7 +1012,7 @@ static inline void context_set_sm_pre(struct context_entry *context)
  */
 static inline void context_clear_sm_pre(struct context_entry *context)
 {
-	context->lo &= ~BIT_ULL(4);
+	context_set_bits(&context->lo, BIT_ULL(4), 0);
 }
 
 /* Returns a number of VTD pages, but aligned to MM page size */
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 77b9b147ab50..7e2b75bcecd4 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -984,7 +984,8 @@ static int context_entry_set_pasid_table(struct context_entry *context,
 	context_clear_entry(context);
 
 	pds = context_get_sm_pds(table);
-	context->lo = (u64)virt_to_phys(table->table) | context_pdts(pds);
+	WRITE_ONCE(context->lo,
+		   (u64)virt_to_phys(table->table) | context_pdts(pds));
 	context_set_sm_rid2pasid(context, IOMMU_NO_PASID);
 
 	if (info->ats_supported)
-- 
2.47.3

Re: [PATCH 1/2] iommu/vt-d: Ensure memory ordering in context entry updates

[Baolu Lu]

On 12/21/25 09:43, Dmytro Maluka wrote:
> When updating context table entries, we do take care to set the present
> bit as the last step, i.e. in the following order:
> 
> 	context_clear_entry(context);
> 	<set other bits>
> 	context_set_present(context);
> 
> However, we don't actually ensure this order, i.e. don't prevent the
> compiler from reordering it. And since context entries may be updated at
> runtime when translation is already enabled, this may potentially allow
> a time window when a device can already do DMA while the translation is
> not properly set up yet (e.g. the context entry may point to an
> arbitrary page table).
> 
> To easily fix this, change context_set_*() and context_clear_*() helpers
> to use READ_ONCE/WRITE_ONCE, to ensure that the ordering between updates
> of individual bits in context entries matches the order of calling those
> helpers, just like we already do for PASID table entries.
> 
> Link: https://lore.kernel.org/all/aTG7gc7I5wExai3S@google.com/
> Signed-off-by: Dmytro Maluka <dmaluka@chromium.org>
> ---
>   drivers/iommu/intel/iommu.h | 37 +++++++++++++++++++++----------------
>   drivers/iommu/intel/pasid.c |  3 ++-
>   2 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index 25c5e22096d4..7f8f004fa756 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -869,7 +869,7 @@ static inline bool dma_pte_superpage(struct dma_pte *pte)
>   
>   static inline bool context_present(struct context_entry *context)
>   {
> -	return (context->lo & 1);
> +	return READ_ONCE(context->lo) & 1;
>   }
>   
>   #define LEVEL_STRIDE		(9)
> @@ -897,46 +897,51 @@ static inline int pfn_level_offset(u64 pfn, int level)
>   	return (pfn >> level_to_offset_bits(level)) & LEVEL_MASK;
>   }
>   
> +static inline void context_set_bits(u64 *ptr, u64 mask, u64 bits)
> +{
> +	u64 old;
> +
> +	old = READ_ONCE(*ptr);
> +	WRITE_ONCE(*ptr, (old & ~mask) | bits);
> +}

Add a line to ensures that the input "bits" cannot overflow the assigned
"mask".

static inline void context_set_bits(u64 *ptr, u64 mask, u64 bits)
{
	u64 val;

	val = READ_ONCE(*ptr);
	val &= ~mask;
	val |= (bits & mask);
	WRITE_ONCE(*ptr, val);
}

>   
>   static inline void context_set_present(struct context_entry *context)
>   {
> -	context->lo |= 1;
> +	context_set_bits(&context->lo, 1 << 0, 1);
>   }

How about adding a smp_wmb() before setting the present bit? Maybe it's
unnecessary for x86 architecture, but at least it's harmless and more
readable. Or not?

static inline void context_set_present(struct context_entry *context)
{
	smp_wmb();
	context_set_bits(&context->lo, 1ULL << 0, 1ULL);
}

>   
>   static inline void context_set_fault_enable(struct context_entry *context)
>   {
> -	context->lo &= (((u64)-1) << 2) | 1;
> +	context_set_bits(&context->lo, 1 << 1, 0);
>   }
>   
>   static inline void context_set_translation_type(struct context_entry *context,
>   						unsigned long value)
>   {
> -	context->lo &= (((u64)-1) << 4) | 3;
> -	context->lo |= (value & 3) << 2;
> +	context_set_bits(&context->lo, GENMASK_ULL(3, 2), value << 2);
>   }
>   
>   static inline void context_set_address_root(struct context_entry *context,
>   					    unsigned long value)
>   {
> -	context->lo &= ~VTD_PAGE_MASK;
> -	context->lo |= value & VTD_PAGE_MASK;
> +	context_set_bits(&context->lo, VTD_PAGE_MASK, value);
>   }
>   
>   static inline void context_set_address_width(struct context_entry *context,
>   					     unsigned long value)
>   {
> -	context->hi |= value & 7;
> +	context_set_bits(&context->hi, GENMASK_ULL(2, 0), value);
>   }
>   
>   static inline void context_set_domain_id(struct context_entry *context,
>   					 unsigned long value)
>   {
> -	context->hi |= (value & ((1 << 16) - 1)) << 8;
> +	context_set_bits(&context->hi, GENMASK_ULL(23, 8), value << 8);
>   }
>   
>   static inline void context_set_pasid(struct context_entry *context)
>   {
> -	context->lo |= CONTEXT_PASIDE;
> +	context_set_bits(&context->lo, CONTEXT_PASIDE, CONTEXT_PASIDE);
>   }
>   
>   static inline int context_domain_id(struct context_entry *c)
> @@ -946,8 +951,8 @@ static inline int context_domain_id(struct context_entry *c)
>   
>   static inline void context_clear_entry(struct context_entry *context)
>   {
> -	context->lo = 0;
> -	context->hi = 0;
> +	WRITE_ONCE(context->lo, 0);
> +	WRITE_ONCE(context->hi, 0);
>   }
>   
>   #ifdef CONFIG_INTEL_IOMMU
> @@ -980,7 +985,7 @@ clear_context_copied(struct intel_iommu *iommu, u8 bus, u8 devfn)
>   static inline void
>   context_set_sm_rid2pasid(struct context_entry *context, unsigned long pasid)
>   {
> -	context->hi |= pasid & ((1 << 20) - 1);
> +	context_set_bits(&context->hi, GENMASK_ULL(19, 0), pasid);
>   }
>   
>   /*
> @@ -989,7 +994,7 @@ context_set_sm_rid2pasid(struct context_entry *context, unsigned long pasid)
>    */
>   static inline void context_set_sm_dte(struct context_entry *context)
>   {
> -	context->lo |= BIT_ULL(2);
> +	context_set_bits(&context->lo, BIT_ULL(2), BIT_ULL(2));
>   }
>   
>   /*
> @@ -998,7 +1003,7 @@ static inline void context_set_sm_dte(struct context_entry *context)
>    */
>   static inline void context_set_sm_pre(struct context_entry *context)
>   {
> -	context->lo |= BIT_ULL(4);
> +	context_set_bits(&context->lo, BIT_ULL(4), BIT_ULL(4));
>   }
>   
>   /*
> @@ -1007,7 +1012,7 @@ static inline void context_set_sm_pre(struct context_entry *context)
>    */
>   static inline void context_clear_sm_pre(struct context_entry *context)
>   {
> -	context->lo &= ~BIT_ULL(4);
> +	context_set_bits(&context->lo, BIT_ULL(4), 0);
>   }
>   
>   /* Returns a number of VTD pages, but aligned to MM page size */
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index 77b9b147ab50..7e2b75bcecd4 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -984,7 +984,8 @@ static int context_entry_set_pasid_table(struct context_entry *context,
>   	context_clear_entry(context);
>   
>   	pds = context_get_sm_pds(table);
> -	context->lo = (u64)virt_to_phys(table->table) | context_pdts(pds);
> +	WRITE_ONCE(context->lo,
> +		   (u64)virt_to_phys(table->table) | context_pdts(pds));
>   	context_set_sm_rid2pasid(context, IOMMU_NO_PASID);
>   
>   	if (info->ats_supported)

Thanks,
baolu

Re: [PATCH 1/2] iommu/vt-d: Ensure memory ordering in context entry updates

[Dmytro Maluka]

On Sun, Dec 21, 2025 at 05:04:27PM +0800, Baolu Lu wrote:
> On 12/21/25 09:43, Dmytro Maluka wrote:
> > +static inline void context_set_bits(u64 *ptr, u64 mask, u64 bits)
> > +{
> > +	u64 old;
> > +
> > +	old = READ_ONCE(*ptr);
> > +	WRITE_ONCE(*ptr, (old & ~mask) | bits);
> > +}
> 
> Add a line to ensures that the input "bits" cannot overflow the assigned
> "mask".
> 
> static inline void context_set_bits(u64 *ptr, u64 mask, u64 bits)
> {
> 	u64 val;
> 
> 	val = READ_ONCE(*ptr);
> 	val &= ~mask;
> 	val |= (bits & mask);
> 	WRITE_ONCE(*ptr, val);
> }

Makes sense. And then worth doing the same in pasid_set_bits() as well?

Actually we can use the same helper for both context and pasid entries
(rename it e.g. to entry_set_bits()).

> >   static inline void context_set_present(struct context_entry *context)
> >   {
> > -	context->lo |= 1;
> > +	context_set_bits(&context->lo, 1 << 0, 1);
> >   }
> 
> How about adding a smp_wmb() before setting the present bit? Maybe it's
> unnecessary for x86 architecture, but at least it's harmless and more
> readable. Or not?
> 
> static inline void context_set_present(struct context_entry *context)
> {
> 	smp_wmb();
> 	context_set_bits(&context->lo, 1ULL << 0, 1ULL);
> }

Maybe... And if so, then in pasid_set_present() as well?

Actually this would make a slight behavioral difference even on x86:
right now this patch only provides ordering of context entry updates
between each other, while smp_wmb() would add a full compiler barrier
here.

So this barrier may be redundant as long as we always use these
context_*() helpers (and thus always use WRITE_ONCE) for any updates of
context entries. On the other hand, it might make it more robust if we
still occasionally do that without WRITE_ONCE, for example in
context_entry_set_pasid_table() which I also changed to use WRITE_ONCE
in this patch.

Re: [PATCH 1/2] iommu/vt-d: Ensure memory ordering in context entry updates

[Baolu Lu]

On 12/21/25 21:11, Dmytro Maluka wrote:
> On Sun, Dec 21, 2025 at 05:04:27PM +0800, Baolu Lu wrote:
>> On 12/21/25 09:43, Dmytro Maluka wrote:
>>> +static inline void context_set_bits(u64 *ptr, u64 mask, u64 bits)
>>> +{
>>> +	u64 old;
>>> +
>>> +	old = READ_ONCE(*ptr);
>>> +	WRITE_ONCE(*ptr, (old & ~mask) | bits);
>>> +}
>>
>> Add a line to ensures that the input "bits" cannot overflow the assigned
>> "mask".
>>
>> static inline void context_set_bits(u64 *ptr, u64 mask, u64 bits)
>> {
>> 	u64 val;
>>
>> 	val = READ_ONCE(*ptr);
>> 	val &= ~mask;
>> 	val |= (bits & mask);
>> 	WRITE_ONCE(*ptr, val);
>> }
> 
> Makes sense. And then worth doing the same in pasid_set_bits() as well?
> 
> Actually we can use the same helper for both context and pasid entries
> (rename it e.g. to entry_set_bits()).

Yeah, that sounds better.

> 
>>>    static inline void context_set_present(struct context_entry *context)
>>>    {
>>> -	context->lo |= 1;
>>> +	context_set_bits(&context->lo, 1 << 0, 1);
>>>    }
>>
>> How about adding a smp_wmb() before setting the present bit? Maybe it's
>> unnecessary for x86 architecture, but at least it's harmless and more
>> readable. Or not?
>>
>> static inline void context_set_present(struct context_entry *context)
>> {
>> 	smp_wmb();
>> 	context_set_bits(&context->lo, 1ULL << 0, 1ULL);
>> }
> 
> Maybe... And if so, then in pasid_set_present() as well?
> 
> Actually this would make a slight behavioral difference even on x86:
> right now this patch only provides ordering of context entry updates
> between each other, while smp_wmb() would add a full compiler barrier
> here.
> 
> So this barrier may be redundant as long as we always use these
> context_*() helpers (and thus always use WRITE_ONCE) for any updates of
> context entries. On the other hand, it might make it more robust if we
> still occasionally do that without WRITE_ONCE, for example in
> context_entry_set_pasid_table() which I also changed to use WRITE_ONCE
> in this patch.

Fair point. It is somewhat redundant. However, I would suggest adding a
comment there. For example:

static inline void context_set_present(struct context_entry *context)
{
  	/*
	 * smp_wmb() is unnecessary here because x86 hardware naturally
	 * keeps writes in order and all context entry modifications
	 * use WRITE_ONCE().
	 */
  	context_set_bits(&context->lo, 1ULL << 0, 1ULL);
}

So that people are still aware of this when porting this driver to
platforms with a weak memory model.

Thanks,
baolu

Re: [PATCH 1/2] iommu/vt-d: Ensure memory ordering in context entry updates

[Dmytro Maluka]

On Tue, Dec 23, 2025 at 02:10:45PM +0800, Baolu Lu wrote:
> On 12/21/25 21:11, Dmytro Maluka wrote:
> > On Sun, Dec 21, 2025 at 05:04:27PM +0800, Baolu Lu wrote:
> > > On 12/21/25 09:43, Dmytro Maluka wrote:
> > > >    static inline void context_set_present(struct context_entry *context)
> > > >    {
> > > > -	context->lo |= 1;
> > > > +	context_set_bits(&context->lo, 1 << 0, 1);
> > > >    }
> > > 
> > > How about adding a smp_wmb() before setting the present bit? Maybe it's
> > > unnecessary for x86 architecture, but at least it's harmless and more
> > > readable. Or not?
> > > 
> > > static inline void context_set_present(struct context_entry *context)
> > > {
> > > 	smp_wmb();
> > > 	context_set_bits(&context->lo, 1ULL << 0, 1ULL);
> > > }
> > 
> > Maybe... And if so, then in pasid_set_present() as well?
> > 
> > Actually this would make a slight behavioral difference even on x86:
> > right now this patch only provides ordering of context entry updates
> > between each other, while smp_wmb() would add a full compiler barrier
> > here.
> > 
> > So this barrier may be redundant as long as we always use these
> > context_*() helpers (and thus always use WRITE_ONCE) for any updates of
> > context entries. On the other hand, it might make it more robust if we
> > still occasionally do that without WRITE_ONCE, for example in
> > context_entry_set_pasid_table() which I also changed to use WRITE_ONCE
> > in this patch.
> 
> Fair point. It is somewhat redundant. However, I would suggest adding a
> comment there. For example:
> 
> static inline void context_set_present(struct context_entry *context)
> {
>  	/*
> 	 * smp_wmb() is unnecessary here because x86 hardware naturally
> 	 * keeps writes in order and all context entry modifications
> 	 * use WRITE_ONCE().
> 	 */
>  	context_set_bits(&context->lo, 1ULL << 0, 1ULL);
> }
> 
> So that people are still aware of this when porting this driver to
> platforms with a weak memory model.

What I actually had in mind was to add smp_wmb() here anyway. It might
be useful as an extra safety measure in case we still forget to use
WRITE_ONCE in some cases, and its performance cost would be virtually
non-existent, I guess. Done in v2.

[PATCH 2/2] iommu/vt-d: Use WRITE_ONCE for setting root table entries

[Dmytro Maluka]

Like context table entries in the previous patch, root table entries may
also be set up at runtime, when DMA translation is already enabled (e.g.
due to a device hotplug). So to stay on the safe side, use WRITE_ONCE
when setting a root table entry, to prevent the compiler from doing
store tearing which could result in setting the present bit earlier than
the context table address bits.

Signed-off-by: Dmytro Maluka <dmaluka@chromium.org>
---
 drivers/iommu/intel/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 134302fbcd92..fe4d0d210a5f 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -374,7 +374,7 @@ struct context_entry *iommu_context_addr(struct intel_iommu *iommu, u8 bus,
 
 		__iommu_flush_cache(iommu, (void *)context, CONTEXT_SIZE);
 		phy_addr = virt_to_phys((void *)context);
-		*entry = phy_addr | 1;
+		WRITE_ONCE(*entry, phy_addr | 1);
 		__iommu_flush_cache(iommu, entry, sizeof(*entry));
 	}
 	return &context[devfn];
-- 
2.47.3