Question about memory ordering in context/PASID entry updates

When updating a context or PASID entry, we'd better make sure we set the present bit after setting up other bits, not before, right? And indeed, for example, in domain_context_mapping_one() we are doing it in the following order:

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

But it doesn't look like we have any compiler barriers to actually ensure this order? (And in case of context entries, there is even no WRITE_ONCE or anything like that.)

So, are we just hoping that the compiler will not reorder it?

What am I missing?

Yes, you are right.

Yes, we should do that to ensure the order.

We didn't implement that primarily because we hadn't encountered any real issues, operating under the assumption that the root/context entries population is completed before DMA translation is really enabled.

However, that assumption is incorrect, as users can change a device's default domain in VT-d legacy mode. This action triggers a context entry change while DMA translation is already running. So, yes, this is a bug.

Thanks,
baolu

I see, thanks. I might try to prepare a patch, though it seems not exactly trivial to fix this all over the code. Roughly seems we could just modify the context_set_*() helpers to use READ_ONCE/WRITE_ONCE similarly to pasid_set_*(), plus add barrier() before context_set_present() and pasid_set_present() calls, plus maybe we should also correspondingly fix updating root table entries in iommu_context_addr() etc.

BTW why exactly I'm concerned about this: even if no one has encountered any functional issues caused by this, DMA isolation is also a security thing, and an attacker might be able to exploit this (albeit only in case those operations actually happen to be reordered by the compiler in a dangerous way in the given kernel build).

Not in scalable mode? I'd assume users can change it in both, and I don't immediately see anything in the code that would limit it to legacy mode only.

In the scalable mode case, the context entry is not altered when changing the default domain. But, yes, users can trigger context entry changes in other cases. So this applies to both modes.

Thanks,
baolu