linux-kernel - Re: [PATCH rc v3 1/4] iommu/arm-smmu-v3: Add ignored bits to fix STE update sequence

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: <CAFgf54pDfBhx8j0M0JJfJR8kDwqOyPem7W1E8kpmBur5q2pjxQ@mail.gmail.com>
Date: Tue, 16 Dec 2025 22:49:58 +0000
From: Mostafa Saleh <smostafa@...gle.com>
To: Nicolin Chen <nicolinc@...dia.com>
Cc: jgg@...dia.com, will@...nel.org, robin.murphy@....com, joro@...tes.org, 
	linux-arm-kernel@...ts.infradead.org, iommu@...ts.linux.dev, 
	linux-kernel@...r.kernel.org, skolothumtho@...dia.com, praan@...gle.com, 
	xueshuai@...ux.alibaba.com
Subject: Re: [PATCH rc v3 1/4] iommu/arm-smmu-v3: Add ignored bits to fix STE
 update sequence

On Mon, Dec 15, 2025 at 8:51 PM Nicolin Chen <nicolinc@...dia.com> wrote:
>
> Hi Mostafa,
>
> On Sun, Dec 14, 2025 at 10:32:35PM +0000, Mostafa Saleh wrote:
> > On Tue, Dec 09, 2025 at 06:45:16PM -0800, Nicolin Chen wrote:
> > > @@ -1207,12 +1223,9 @@ void arm_smmu_write_entry(struct arm_smmu_entry_writer *writer, __le64 *entry,
> > >             entry_set(writer, entry, target, 0, 1);
> > >     } else {
> > >             /*
> > > -            * No inuse bit changed. Sanity check that all unused bits are 0
> > > -            * in the entry. The target was already sanity checked by
> > > -            * compute_qword_diff().
> > > +            * No inuse bit changed, though ignored bits may have changed.
> > >              */
> > > -           WARN_ON_ONCE(
> > > -                   entry_set(writer, entry, target, 0, NUM_ENTRY_QWORDS));
> > > +           entry_set(writer, entry, target, 0, NUM_ENTRY_QWORDS);
> >
> > After this change, no other caller uses the entry_set() return value, so it
> > can be changed to return void.
>
> OK.
>
> > >     }
> > >  }
> > >  EXPORT_SYMBOL_IF_KUNIT(arm_smmu_write_entry);
> > > @@ -1543,6 +1556,7 @@ static void arm_smmu_ste_writer_sync_entry(struct arm_smmu_entry_writer *writer)
> > >  static const struct arm_smmu_entry_writer_ops arm_smmu_ste_writer_ops = {
> > >     .sync = arm_smmu_ste_writer_sync_entry,
> > >     .get_used = arm_smmu_get_ste_used,
> > > +   .get_ignored = arm_smmu_get_ste_ignored,
> > >  };
> > >
> >
> > I have some mixed feelings about this, having get_used(), then get_ignored()
> > with the same bits set seems confusing to me, specially the get_ignored()
> > loops back to update cur_used, which is set from get_used()
> >
> > My initial though was just to remove this bit from get_used() + some changes
> > to checks setting bits that are not used would be enough, and the semantics
> > of get_used() can be something as:
> > “Return bits used by the updated translation regime that MUST be observed
> > atomically” and in that case we can ignore things as MEV as it doesn’t
> > impact the translation.
> >
> > However, this approach makes it a bit explicit which bits are ignored, if we
> > keep this logic, I think changing the name of get_ignored() might help, to
> > something as "get_allowed_break()" or "get_update_safe()"?
>
> I think "ignored" itself is brief and understandable.. Instead,
> perhaps we can add a kdocs to make it clearer:
>
> /**
>  * struct arm_smmu_entry_writer_ops - STE/CD entry writer operations
>  * @get_used: Output to @used the bits used by the hardware corresponding to the
>  *            configurations bits set in a given @entry
>  * @get_ignored: Output to @ignored the bits that are listed in the "used" list
>  *               but allowed to be ignored by arm_smmu_entry_qword_diff(). Each
>  *               field (bits) must provide a reason to justify that the entries
>  *               can be updated safely without breaking STE/CD configurations.
>  * @sync: Operation to synchronize the updated STE/CD entries in the memory
>  */
> struct arm_smmu_entry_writer_ops {
>         void (*get_used)(const __le64 *entry, __le64 *used);
>         void (*get_ignored)(__le64 *ignored);
>         void (*sync)(struct arm_smmu_entry_writer *writer);
> };
>
> ?
>

A comment is indeed helpful, but my point was that "used" and
"ignored" make it seem that they are mutually exclusive.

Thanks,
Mostafa

> Thanks
> Nicolin