[2/4] iommu: Use group ownership to avoid driver attachment

  The iommu_group_store_type() requires the devices in the iommu group are
not bound to any device driver during the whole operation. The existing
code locks the device with device_lock(dev) and use device_is_bound() to
check whether any driver is bound to device.

In fact, this can be achieved through the DMA ownership helpers. Replace
them with iommu_group_claim/release_dma_owner() helpers.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)
  

On Mon, Feb 13, 2023 at 03:49:39PM +0800, Lu Baolu wrote:
> The iommu_group_store_type() requires the devices in the iommu group are
> not bound to any device driver during the whole operation. The existing
> code locks the device with device_lock(dev) and use device_is_bound() to
> check whether any driver is bound to device.
> 
> In fact, this can be achieved through the DMA ownership helpers. Replace
> them with iommu_group_claim/release_dma_owner() helpers.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 4f71dcd2621b..6547cb38480c 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2807,12 +2807,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
>  
>  	mutex_lock(&group->mutex);
>  
> -	if (group->default_domain != group->domain) {
> -		dev_err_ratelimited(prev_dev, "Group not assigned to default domain\n");
> -		ret = -EBUSY;
> -		goto out;
> -	}
> -
>  	/*
>  	 * iommu group wasn't locked while acquiring device lock in
>  	 * iommu_group_store_type(). So, make sure that the device count hasn't
> @@ -2971,6 +2965,7 @@ static void iommu_group_unfreeze_dev_ops(struct iommu_group *group)
>  static ssize_t iommu_group_store_type(struct iommu_group *group,
>  				      const char *buf, size_t count)
>  {
> +	bool group_owner_claimed = false;
>  	struct group_device *grp_dev;
>  	struct device *dev;
>  	int ret, req_type;
> @@ -2992,6 +2987,14 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
>  	else
>  		return -EINVAL;
>  
> +	if (req_type != IOMMU_DOMAIN_DMA_FQ ||
> +	    group->default_domain->type != IOMMU_DOMAIN_DMA) {
> +		ret = iommu_group_claim_dma_owner(group, (void *)buf);
> +		if (ret)
> +			return ret;
> +		group_owner_claimed = true;
> +	}

I don't get it, this should be done unconditionally. If we couldn't
take ownership then we simply can't progress.

But there is more to it than that, a device that is owned should not
be release and to achieve this the general logic around the owner
scheme assumes that a driver is attached.

So if you call it from this non-driver context you have to hold the
group_mutex as previously discussed, which also means this needs to be
an externally version of iommu_group_claim_dma_owner()

Jason
  

On 2/13/23 10:19 PM, Jason Gunthorpe wrote:
> On Mon, Feb 13, 2023 at 03:49:39PM +0800, Lu Baolu wrote:
>> The iommu_group_store_type() requires the devices in the iommu group are
>> not bound to any device driver during the whole operation. The existing
>> code locks the device with device_lock(dev) and use device_is_bound() to
>> check whether any driver is bound to device.
>>
>> In fact, this can be achieved through the DMA ownership helpers. Replace
>> them with iommu_group_claim/release_dma_owner() helpers.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/iommu.c | 27 +++++++++++++--------------
>>   1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 4f71dcd2621b..6547cb38480c 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2807,12 +2807,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
>>   
>>   	mutex_lock(&group->mutex);
>>   
>> -	if (group->default_domain != group->domain) {
>> -		dev_err_ratelimited(prev_dev, "Group not assigned to default domain\n");
>> -		ret = -EBUSY;
>> -		goto out;
>> -	}
>> -
>>   	/*
>>   	 * iommu group wasn't locked while acquiring device lock in
>>   	 * iommu_group_store_type(). So, make sure that the device count hasn't
>> @@ -2971,6 +2965,7 @@ static void iommu_group_unfreeze_dev_ops(struct iommu_group *group)
>>   static ssize_t iommu_group_store_type(struct iommu_group *group,
>>   				      const char *buf, size_t count)
>>   {
>> +	bool group_owner_claimed = false;
>>   	struct group_device *grp_dev;
>>   	struct device *dev;
>>   	int ret, req_type;
>> @@ -2992,6 +2987,14 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
>>   	else
>>   		return -EINVAL;
>>   
>> +	if (req_type != IOMMU_DOMAIN_DMA_FQ ||
>> +	    group->default_domain->type != IOMMU_DOMAIN_DMA) {
>> +		ret = iommu_group_claim_dma_owner(group, (void *)buf);
>> +		if (ret)
>> +			return ret;
>> +		group_owner_claimed = true;
>> +	}
> 
> I don't get it, this should be done unconditionally. If we couldn't
> take ownership then we simply can't progress.

The existing code allows the user to switch the default domain from
strict to lazy invalidation mode. The default domain is not changed,
hence it should be seamless and transparent to the device driver.

> 
> But there is more to it than that, a device that is owned should not
> be release and to achieve this the general logic around the owner
> scheme assumes that a driver is attached.

Yes. Current ownership scheme was built on this assumption.

> 
> So if you call it from this non-driver context you have to hold the
> group_mutex as previously discussed,

Yes.

> which also means this needs to be
> an externally version of iommu_group_claim_dma_owner()

Sorry! What does "an externally version of
iommu_group_claim_dma_owner()" mean?

My understanding is that we should limit iommu_group_claim_dma_owner()
use in the driver context. For this non-driver context, we should not
use iommu_group_claim_dma_owner() directly, but hold the group->mutex
and check the group->owner_cnt directly:

         mutex_lock(&group->mutex);
         if (group->owner_cnt) {
                 ret = -EPERM;
                 goto unlock_out;
         }

the group->mutex should be held until everything is done.

Best regards,
baolu
  

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, February 15, 2023 1:51 PM
> 
> On 2/13/23 10:19 PM, Jason Gunthorpe wrote:
> > On Mon, Feb 13, 2023 at 03:49:39PM +0800, Lu Baolu wrote:
> >> @@ -2992,6 +2987,14 @@ static ssize_t iommu_group_store_type(struct
> iommu_group *group,
> >>   	else
> >>   		return -EINVAL;
> >>
> >> +	if (req_type != IOMMU_DOMAIN_DMA_FQ ||
> >> +	    group->default_domain->type != IOMMU_DOMAIN_DMA) {
> >> +		ret = iommu_group_claim_dma_owner(group, (void *)buf);
> >> +		if (ret)
> >> +			return ret;
> >> +		group_owner_claimed = true;
> >> +	}
> >
> > I don't get it, this should be done unconditionally. If we couldn't
> > take ownership then we simply can't progress.
> 
> The existing code allows the user to switch the default domain from
> strict to lazy invalidation mode. The default domain is not changed,
> hence it should be seamless and transparent to the device driver.

Is there real usage relying on this transition for a bound device?

In concept strict->lazy transition implies relaxed DMA security. It's hard
to think of a motivation of doing so while the device might be doing
in-fly DMAs.

Presumably such perf/security tradeoff should be planned way before
binding device/driver together.

btw if strict->lazy is allowed why lazy->strict is prohibited?

> 
> > which also means this needs to be
> > an externally version of iommu_group_claim_dma_owner()
> 
> Sorry! What does "an externally version of
> iommu_group_claim_dma_owner()" mean?
> 
> My understanding is that we should limit iommu_group_claim_dma_owner()
> use in the driver context. For this non-driver context, we should not
> use iommu_group_claim_dma_owner() directly, but hold the group->mutex
> and check the group->owner_cnt directly:
> 
>          mutex_lock(&group->mutex);
>          if (group->owner_cnt) {
>                  ret = -EPERM;
>                  goto unlock_out;
>          }
> 
> the group->mutex should be held until everything is done.
> 

I guess you two meant the same thing.

	mutex_lock(&group->mutex);
	iommu_group_claim_dma_owner_unlocked();
	//blah blah
	mutex_unlock(&group->mutex);
  

On 2023/2/15 14:56, Tian, Kevin wrote:
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Wednesday, February 15, 2023 1:51 PM
>>
>> On 2/13/23 10:19 PM, Jason Gunthorpe wrote:
>>> On Mon, Feb 13, 2023 at 03:49:39PM +0800, Lu Baolu wrote:
>>>> @@ -2992,6 +2987,14 @@ static ssize_t iommu_group_store_type(struct
>> iommu_group *group,
>>>>    	else
>>>>    		return -EINVAL;
>>>>
>>>> +	if (req_type != IOMMU_DOMAIN_DMA_FQ ||
>>>> +	    group->default_domain->type != IOMMU_DOMAIN_DMA) {
>>>> +		ret = iommu_group_claim_dma_owner(group, (void *)buf);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +		group_owner_claimed = true;
>>>> +	}
>>> I don't get it, this should be done unconditionally. If we couldn't
>>> take ownership then we simply can't progress.
>> The existing code allows the user to switch the default domain from
>> strict to lazy invalidation mode. The default domain is not changed,
>> hence it should be seamless and transparent to the device driver.
> Is there real usage relying on this transition for a bound device?
> 
> In concept strict->lazy transition implies relaxed DMA security. It's hard
> to think of a motivation of doing so while the device might be doing
> in-fly DMAs.
> 
> Presumably such perf/security tradeoff should be planned way before
> binding device/driver together.
> 
> btw if strict->lazy is allowed why lazy->strict is prohibited?
> 

We all know, strict vs. lazy is a tradeoff between performance and
security.

strict -> lazy: driver works in secure mode. This transition trades off
security for better performance.

lazy->strict: The driver is already working in non-safety mode. This
transition only results in worse performance. It makes no sense. If user
want to put the driver in a secure mode, they need to unbind the driver,
reset the device and do the lazy->strict transition.

Robin might have better insights.

Best regards,
baolu
  

On 2023-02-15 07:28, Baolu Lu wrote:
> On 2023/2/15 14:56, Tian, Kevin wrote:
>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>> Sent: Wednesday, February 15, 2023 1:51 PM
>>>
>>> On 2/13/23 10:19 PM, Jason Gunthorpe wrote:
>>>> On Mon, Feb 13, 2023 at 03:49:39PM +0800, Lu Baolu wrote:
>>>>> @@ -2992,6 +2987,14 @@ static ssize_t iommu_group_store_type(struct
>>> iommu_group *group,
>>>>>        else
>>>>>            return -EINVAL;
>>>>>
>>>>> +    if (req_type != IOMMU_DOMAIN_DMA_FQ ||
>>>>> +        group->default_domain->type != IOMMU_DOMAIN_DMA) {
>>>>> +        ret = iommu_group_claim_dma_owner(group, (void *)buf);
>>>>> +        if (ret)
>>>>> +            return ret;
>>>>> +        group_owner_claimed = true;
>>>>> +    }
>>>> I don't get it, this should be done unconditionally. If we couldn't
>>>> take ownership then we simply can't progress.
>>> The existing code allows the user to switch the default domain from
>>> strict to lazy invalidation mode. The default domain is not changed,
>>> hence it should be seamless and transparent to the device driver.
>> Is there real usage relying on this transition for a bound device?
>>
>> In concept strict->lazy transition implies relaxed DMA security. It's 
>> hard
>> to think of a motivation of doing so while the device might be doing
>> in-fly DMAs.
>>
>> Presumably such perf/security tradeoff should be planned way before
>> binding device/driver together.
>>
>> btw if strict->lazy is allowed why lazy->strict is prohibited?
>>
> 
> We all know, strict vs. lazy is a tradeoff between performance and
> security.
> 
> strict -> lazy: driver works in secure mode. This transition trades off
> security for better performance.
> 
> lazy->strict: The driver is already working in non-safety mode. This
> transition only results in worse performance. It makes no sense. If user
> want to put the driver in a secure mode, they need to unbind the driver,
> reset the device and do the lazy->strict transition.
> 
> Robin might have better insights.

Yes, this was added for a definite use-case in ChromeOS, where 
strict->lazy needs to support being done "live" since the device in 
question is the storage controller for the already-mounted root 
filesystem. Your reasoning seems to match what I summarised in the 
original commit message :)

Thanks,
Robin.
  

On Wed, Feb 15, 2023 at 01:51:14PM +0800, Baolu Lu wrote:
> On 2/13/23 10:19 PM, Jason Gunthorpe wrote:
> > On Mon, Feb 13, 2023 at 03:49:39PM +0800, Lu Baolu wrote:
> > > The iommu_group_store_type() requires the devices in the iommu group are
> > > not bound to any device driver during the whole operation. The existing
> > > code locks the device with device_lock(dev) and use device_is_bound() to
> > > check whether any driver is bound to device.
> > > 
> > > In fact, this can be achieved through the DMA ownership helpers. Replace
> > > them with iommu_group_claim/release_dma_owner() helpers.
> > > 
> > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > > ---
> > >   drivers/iommu/iommu.c | 27 +++++++++++++--------------
> > >   1 file changed, 13 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index 4f71dcd2621b..6547cb38480c 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -2807,12 +2807,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
> > >   	mutex_lock(&group->mutex);
> > > -	if (group->default_domain != group->domain) {
> > > -		dev_err_ratelimited(prev_dev, "Group not assigned to default domain\n");
> > > -		ret = -EBUSY;
> > > -		goto out;
> > > -	}
> > > -
> > >   	/*
> > >   	 * iommu group wasn't locked while acquiring device lock in
> > >   	 * iommu_group_store_type(). So, make sure that the device count hasn't
> > > @@ -2971,6 +2965,7 @@ static void iommu_group_unfreeze_dev_ops(struct iommu_group *group)
> > >   static ssize_t iommu_group_store_type(struct iommu_group *group,
> > >   				      const char *buf, size_t count)
> > >   {
> > > +	bool group_owner_claimed = false;
> > >   	struct group_device *grp_dev;
> > >   	struct device *dev;
> > >   	int ret, req_type;
> > > @@ -2992,6 +2987,14 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
> > >   	else
> > >   		return -EINVAL;
> > > +	if (req_type != IOMMU_DOMAIN_DMA_FQ ||
> > > +	    group->default_domain->type != IOMMU_DOMAIN_DMA) {
> > > +		ret = iommu_group_claim_dma_owner(group, (void *)buf);
> > > +		if (ret)
> > > +			return ret;
> > > +		group_owner_claimed = true;
> > > +	}
> > 
> > I don't get it, this should be done unconditionally. If we couldn't
> > take ownership then we simply can't progress.
> 
> The existing code allows the user to switch the default domain from
> strict to lazy invalidation mode. The default domain is not changed,
> hence it should be seamless and transparent to the device driver.

So make that a special case, get the group lock check if it is this
case and then just adjust it and exit, otherwise get ownership under
the group lock as discussed.
> 
> > which also means this needs to be
> > an externally version of iommu_group_claim_dma_owner()
> 
> Sorry! What does "an externally version of
> iommu_group_claim_dma_owner()" mean?
> 

Oops "externally locked"

> My understanding is that we should limit iommu_group_claim_dma_owner()
> use in the driver context. For this non-driver context, we should not
> use iommu_group_claim_dma_owner() directly, but hold the group->mutex
> and check the group->owner_cnt directly:
> 
>         mutex_lock(&group->mutex);
>         if (group->owner_cnt) {
>                 ret = -EPERM;
>                 goto unlock_out;
>         }
> 
> the group->mutex should be held until everything is done.

Yes, that would be fine as long as we can hold the group mutex
throughout

Jason
  

On 2/15/23 8:56 PM, Jason Gunthorpe wrote:
> On Wed, Feb 15, 2023 at 01:51:14PM +0800, Baolu Lu wrote:
>> On 2/13/23 10:19 PM, Jason Gunthorpe wrote:
>>> On Mon, Feb 13, 2023 at 03:49:39PM +0800, Lu Baolu wrote:
>>>> The iommu_group_store_type() requires the devices in the iommu group are
>>>> not bound to any device driver during the whole operation. The existing
>>>> code locks the device with device_lock(dev) and use device_is_bound() to
>>>> check whether any driver is bound to device.
>>>>
>>>> In fact, this can be achieved through the DMA ownership helpers. Replace
>>>> them with iommu_group_claim/release_dma_owner() helpers.
>>>>
>>>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>>>> ---
>>>>    drivers/iommu/iommu.c | 27 +++++++++++++--------------
>>>>    1 file changed, 13 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>> index 4f71dcd2621b..6547cb38480c 100644
>>>> --- a/drivers/iommu/iommu.c
>>>> +++ b/drivers/iommu/iommu.c
>>>> @@ -2807,12 +2807,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
>>>>    	mutex_lock(&group->mutex);
>>>> -	if (group->default_domain != group->domain) {
>>>> -		dev_err_ratelimited(prev_dev, "Group not assigned to default domain\n");
>>>> -		ret = -EBUSY;
>>>> -		goto out;
>>>> -	}
>>>> -
>>>>    	/*
>>>>    	 * iommu group wasn't locked while acquiring device lock in
>>>>    	 * iommu_group_store_type(). So, make sure that the device count hasn't
>>>> @@ -2971,6 +2965,7 @@ static void iommu_group_unfreeze_dev_ops(struct iommu_group *group)
>>>>    static ssize_t iommu_group_store_type(struct iommu_group *group,
>>>>    				      const char *buf, size_t count)
>>>>    {
>>>> +	bool group_owner_claimed = false;
>>>>    	struct group_device *grp_dev;
>>>>    	struct device *dev;
>>>>    	int ret, req_type;
>>>> @@ -2992,6 +2987,14 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
>>>>    	else
>>>>    		return -EINVAL;
>>>> +	if (req_type != IOMMU_DOMAIN_DMA_FQ ||
>>>> +	    group->default_domain->type != IOMMU_DOMAIN_DMA) {
>>>> +		ret = iommu_group_claim_dma_owner(group, (void *)buf);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +		group_owner_claimed = true;
>>>> +	}
>>> I don't get it, this should be done unconditionally. If we couldn't
>>> take ownership then we simply can't progress.
>> The existing code allows the user to switch the default domain from
>> strict to lazy invalidation mode. The default domain is not changed,
>> hence it should be seamless and transparent to the device driver.
> So make that a special case, get the group lock check if it is this
> case and then just adjust it and exit, otherwise get ownership under
> the group lock as discussed.

OK. Will do like this in the next version.

Best regards,
baolu
  

On 2/15/23 7:09 PM, Robin Murphy wrote:
> On 2023-02-15 07:28, Baolu Lu wrote:
>> On 2023/2/15 14:56, Tian, Kevin wrote:
>>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>>> Sent: Wednesday, February 15, 2023 1:51 PM
>>>>
>>>> On 2/13/23 10:19 PM, Jason Gunthorpe wrote:
>>>>> On Mon, Feb 13, 2023 at 03:49:39PM +0800, Lu Baolu wrote:
>>>>>> @@ -2992,6 +2987,14 @@ static ssize_t iommu_group_store_type(struct
>>>> iommu_group *group,
>>>>>>        else
>>>>>>            return -EINVAL;
>>>>>>
>>>>>> +    if (req_type != IOMMU_DOMAIN_DMA_FQ ||
>>>>>> +        group->default_domain->type != IOMMU_DOMAIN_DMA) {
>>>>>> +        ret = iommu_group_claim_dma_owner(group, (void *)buf);
>>>>>> +        if (ret)
>>>>>> +            return ret;
>>>>>> +        group_owner_claimed = true;
>>>>>> +    }
>>>>> I don't get it, this should be done unconditionally. If we couldn't
>>>>> take ownership then we simply can't progress.
>>>> The existing code allows the user to switch the default domain from
>>>> strict to lazy invalidation mode. The default domain is not changed,
>>>> hence it should be seamless and transparent to the device driver.
>>> Is there real usage relying on this transition for a bound device?
>>>
>>> In concept strict->lazy transition implies relaxed DMA security. It's 
>>> hard
>>> to think of a motivation of doing so while the device might be doing
>>> in-fly DMAs.
>>>
>>> Presumably such perf/security tradeoff should be planned way before
>>> binding device/driver together.
>>>
>>> btw if strict->lazy is allowed why lazy->strict is prohibited?
>>>
>>
>> We all know, strict vs. lazy is a tradeoff between performance and
>> security.
>>
>> strict -> lazy: driver works in secure mode. This transition trades off
>> security for better performance.
>>
>> lazy->strict: The driver is already working in non-safety mode. This
>> transition only results in worse performance. It makes no sense. If user
>> want to put the driver in a secure mode, they need to unbind the driver,
>> reset the device and do the lazy->strict transition.
>>
>> Robin might have better insights.
> 
> Yes, this was added for a definite use-case in ChromeOS, where 
> strict->lazy needs to support being done "live" since the device in 
> question is the storage controller for the already-mounted root 
> filesystem.

Thanks for letting us know this.

> Your reasoning seems to match what I summarised in the 
> original commit message 😄

Haha, it seems that my memory is till good. :-)

Best regards,
baolu