Re: [PATCH v2 2/2] iommu/arm-smmu-v3: Adopt arm_smmu_event in handlers

[Pranjal Shrivastava <praan@google.com>]

On Wed, Aug 28, 2024 at 10:20:02PM -0700, Nicolin Chen wrote:
> Hi Pranjal,
> 
> On Tue, Aug 27, 2024 at 12:30:26PM -0700, Pranjal Shrivastava wrote:
> 
> >         /* Stage-2 is always pinned at the moment */
> > -       if (evt[1] & EVTQ_1_S2)
> > +       if (event->stage)
> >                 return -EFAULT;
> 
> Should it be named to "s2" v.s stage?

Sure, will rename it with the next version.

> 
> > -       mutex_lock(&smmu->streams_mutex);
> > -       master = arm_smmu_find_master(smmu, sid);
> > -       if (!master) {
> > -               ret = -EINVAL;
> > -               goto out_unlock;
> > -       }
> > +       if (!event->master)
> > +               return -EINVAL;
> > 
> > -       ret = iommu_report_device_fault(master->dev, &fault_evt);
> > -out_unlock:
> > -       mutex_unlock(&smmu->streams_mutex);
> > +       ret = iommu_report_device_fault(event->master->dev, &fault_evt);
> 
> The iommu_report_device_fault(master->dev) call lost its mutex
> lock. I wonder if it could be unsafe to continue with that dev
> pointer.

Ack, I agree, responded with [Patch 1/2] in detail.

In summary, we need to avoid the following situation:

1. The evtq_thread gets scheduled
2. arm_smmu_release_device removes the `master` & its streams
3. In the `handle_evt` we dereference `master` which has been `kfree`ed
   (also, we don't return -EINVAL in that case as we ideally should)

I'll add back the `arm_smmu_find_master` in the the `arm_smmu_handle_evt`
along with the locks. Nice catch! :)

> 
> Nicolin

Thanks,
Pranjal