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

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] [day] [month] [year] [list]

Message-ID: <e63edcfe-0ee9-4582-b29e-5beb549904ce@linux.intel.com>
Date: Tue, 23 Dec 2025 14:10:45 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: Dmytro Maluka <dmaluka@...omium.org>
Cc: David Woodhouse <dwmw2@...radead.org>, iommu@...ts.linux.dev,
 Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
 Robin Murphy <robin.murphy@....com>, linux-kernel@...r.kernel.org,
 "Vineeth Pillai (Google)" <vineeth@...byteword.org>,
 Aashish Sharma <aashish@...hishsharma.net>,
 Grzegorz Jaszczyk <jaszczyk@...omium.org>,
 Chuanxiao Dong <chuanxiao.dong@...el.com>, Kevin Tian <kevin.tian@...el.com>
Subject: Re: [PATCH 1/2] iommu/vt-d: Ensure memory ordering in context entry
 updates

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