On Fri, Feb 28, 2020 at 03:39:35PM +0100, Jean-Philippe Brucker wrote:
> > > + list_for_each_entry_rcu(bond, &io_mm->devices, mm_head) {
> > > + /*
> > > + * To ensure that we observe the initialization of io_mm fields
> > > + * by io_mm_finalize() before the registration of this bond to
> > > + * the list by io_mm_attach(), introduce an address dependency
> > > + * between bond and io_mm. It pairs with the smp_store_release()
> > > + * from list_add_rcu().
> > > + */
> > > + io_mm = rcu_dereference(bond->io_mm);
> >
> > A rcu_dereference isn't need here, just a normal derference is fine.
>
> bond->io_mm is annotated with __rcu (for iommu_sva_get_pasid_generic(),
> which does bond->io_mm under rcu_read_lock())
I'm surprised the bond->io_mm can change over the lifetime of the
bond memory..
> > > If io_mm->ctx and io_mm->ops are already valid before the
> > > mmu notifier is published, then we don't need that stuff.
> >
> > So, this trickyness with RCU is not a bad reason to introduce the priv
> > scheme, maybe explain it in the commit message?
>
> Ok, I've added this to the commit message:
>
> The IOMMU SVA module, which attaches an mm to multiple devices,
> exemplifies this situation. In essence it does:
>
> mmu_notifier_get()
> alloc_notifier()
> A = kzalloc()
> /* MMU notifier is published */
> A->ctx = ctx; // (1)
> device->A = A;
> list_add_rcu(device, A->devices); // (2)
>
> The invalidate notifier, which may start running before A is fully
> initialized at (1), does the following:
>
> io_mm_invalidate(A)
> list_for_each_entry_rcu(device, A->devices)
> A = device->A; // (3)
I would drop the work around from the decription, it is enough to say
that the line below needs to observe (1) after (2) and this is
trivially achieved by moving (1) to before publishing the notifier so
the core MM locking can be used.
Regards,
Jason
