[PATCH 0/2] powerpc/iommu: check dev->iommu_group before remove it

[PATCH 0/2] powerpc/iommu: check dev->iommu_group before remove it

[Wei Yang]

Had a talk with Alexey, who suggest me to send this for comments.

On powernv platform, the P7IOC provide limited range of DMA space. For
example, there are only 8 DMA segments on each PHB. When there are more 
than 8 PEs, some of PE's tce32_table is not initialized.

In the normal case, 8 PE is enough. If VF is enabled, 8 is not enough. This
lead to some pci_dev's iommu_table_base is NULL. Which results this device
couldn't be added to any iommu_group, and finally dev->iommu_group is left NULL.
When this VF is removed from the system, this will case kernel crash.

These two patches add the check to guard the kernel. Comments are welcome for
which place is better.

One more comment from Alexey is where we allow these non-DMA capable device
exist in the system. Maybe we should not allow to create a pci_dev for those
who can't get proper DMA space. Your comments are welcome :-)

Thanks in advance.

Wei Yang (2):
  iommu: Don't remove device when no iommu_group associated
  powerpc/iommu: check dev->iommu_group before remove a device from
    iommu_group

 arch/powerpc/kernel/iommu.c |    3 ++-
 drivers/iommu/iommu.c       |    3 +++
 2 files changed, 5 insertions(+), 1 deletions(-)

-- 
1.7.5.4

[PATCH 1/2] iommu: Don't remove device when no iommu_group associated

[Wei Yang]

In some cases, one device may not associated with any iommu_group.
For example, not enough DMA address space.

For those devices, kernel will crash when try to remove it from an iommu_group.

This patch do the check before remove it.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
---
 drivers/iommu/iommu.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index fbe9ca7..fe41946 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -379,6 +379,9 @@ void iommu_group_remove_device(struct device *dev)
 	struct iommu_group *group = dev->iommu_group;
 	struct iommu_device *tmp_device, *device = NULL;
 
+	if (!group)
+		return;
+
 	/* Pre-notify listeners that a device is being removed. */
 	blocking_notifier_call_chain(&group->notifier,
 				     IOMMU_GROUP_NOTIFY_DEL_DEVICE, dev);
-- 
1.7.5.4

[PATCH 2/2] powerpc/iommu: check dev->iommu_group before remove a device from iommu_group

[Wei Yang]

---
 arch/powerpc/kernel/iommu.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index b20ff17..5abf7c3 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
 	case BUS_NOTIFY_ADD_DEVICE:
 		return iommu_add_device(dev);
 	case BUS_NOTIFY_DEL_DEVICE:
-		iommu_del_device(dev);
+		if (dev->iommu_group)
+			iommu_del_device(dev);
 		return 0;
 	default:
 		return 0;
-- 
1.7.5.4

Re: [PATCH 2/2] powerpc/iommu: check dev->iommu_group before remove a device from iommu_group

[Alexey Kardashevskiy]

On 08/16/2013 08:08 PM, Wei Yang wrote:
> ---
>  arch/powerpc/kernel/iommu.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index b20ff17..5abf7c3 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>  	case BUS_NOTIFY_ADD_DEVICE:
>  		return iommu_add_device(dev);
>  	case BUS_NOTIFY_DEL_DEVICE:
> -		iommu_del_device(dev);
> +		if (dev->iommu_group)
> +			iommu_del_device(dev);
>  		return 0;
>  	default:
>  		return 0;
> 

This one seems redundant, no?


-- 
Alexey

Re: [PATCH 2/2] powerpc/iommu: check dev->iommu_group before remove a device from iommu_group

[Wei Yang]

On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
>On 08/16/2013 08:08 PM, Wei Yang wrote:
>> ---
>>  arch/powerpc/kernel/iommu.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>> 
>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>> index b20ff17..5abf7c3 100644
>> --- a/arch/powerpc/kernel/iommu.c
>> +++ b/arch/powerpc/kernel/iommu.c
>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>>  	case BUS_NOTIFY_ADD_DEVICE:
>>  		return iommu_add_device(dev);
>>  	case BUS_NOTIFY_DEL_DEVICE:
>> -		iommu_del_device(dev);
>> +		if (dev->iommu_group)
>> +			iommu_del_device(dev);
>>  		return 0;
>>  	default:
>>  		return 0;
>> 
>
>This one seems redundant, no?

Sorry for the late.

Yes, these two patches have the same purpose to guard the system, while in two
different places.  One is in powernv platform, the other is in the generic iommu 
driver.

The one in powernv platform is used to correct the original logic.

The one in generic iommu driver is to keep system safe in case other platform to
call iommu_group_remove_device() without the check.

>
>
>-- 
>Alexey

-- 
Richard Yang
Help you, Help me

Re: [PATCH 2/2] powerpc/iommu: check dev->iommu_group before remove a device from iommu_group

[Alexey Kardashevskiy]

On 08/19/2013 11:29 AM, Wei Yang wrote:
> On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
>> On 08/16/2013 08:08 PM, Wei Yang wrote:
>>> ---
>>>  arch/powerpc/kernel/iommu.c |    3 ++-
>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>>> index b20ff17..5abf7c3 100644
>>> --- a/arch/powerpc/kernel/iommu.c
>>> +++ b/arch/powerpc/kernel/iommu.c
>>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>>>  	case BUS_NOTIFY_ADD_DEVICE:
>>>  		return iommu_add_device(dev);
>>>  	case BUS_NOTIFY_DEL_DEVICE:
>>> -		iommu_del_device(dev);
>>> +		if (dev->iommu_group)
>>> +			iommu_del_device(dev);
>>>  		return 0;
>>>  	default:
>>>  		return 0;
>>>
>>
>> This one seems redundant, no?
> 
> Sorry for the late.
> 
> Yes, these two patches have the same purpose to guard the system, while in two
> different places.  One is in powernv platform, the other is in the generic iommu 
> driver.
> 
> The one in powernv platform is used to correct the original logic.
> 
> The one in generic iommu driver is to keep system safe in case other platform to
> call iommu_group_remove_device() without the check.


But I am moving bus notifier to powernv code (posted a patch last week,
otherwise Freescale's IOMMU conflicted) so this won't be the case.



-- 
Alexey

Re: [PATCH 2/2] powerpc/iommu: check dev->iommu_group before remove a device from iommu_group

[Wei Yang]

On Mon, Aug 19, 2013 at 11:39:49AM +1000, Alexey Kardashevskiy wrote:
>On 08/19/2013 11:29 AM, Wei Yang wrote:
>> On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
>>> On 08/16/2013 08:08 PM, Wei Yang wrote:
>>>> ---
>>>>  arch/powerpc/kernel/iommu.c |    3 ++-
>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>>>> index b20ff17..5abf7c3 100644
>>>> --- a/arch/powerpc/kernel/iommu.c
>>>> +++ b/arch/powerpc/kernel/iommu.c
>>>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>>>>  	case BUS_NOTIFY_ADD_DEVICE:
>>>>  		return iommu_add_device(dev);
>>>>  	case BUS_NOTIFY_DEL_DEVICE:
>>>> -		iommu_del_device(dev);
>>>> +		if (dev->iommu_group)
>>>> +			iommu_del_device(dev);
>>>>  		return 0;
>>>>  	default:
>>>>  		return 0;
>>>>
>>>
>>> This one seems redundant, no?
>> 
>> Sorry for the late.
>> 
>> Yes, these two patches have the same purpose to guard the system, while in two
>> different places.  One is in powernv platform, the other is in the generic iommu 
>> driver.
>> 
>> The one in powernv platform is used to correct the original logic.
>> 
>> The one in generic iommu driver is to keep system safe in case other platform to
>> call iommu_group_remove_device() without the check.
>
>
>But I am moving bus notifier to powernv code (posted a patch last week,
>otherwise Freescale's IOMMU conflicted) so this won't be the case.

Yes, I see the patch.

This means other platforms, besides powernv, will check the dev->iommu_group
before remove the device? This would be a convention?

If this is the case, the second patch is enough. We don't need to check it in
generic iommu driver.

Since I am not very familiar with the code convention, I post these two
patches together. This doesn't mean I need to push both of them. Your comments
are welcome, lets me understand which one is more suitable in this case.

>
>
>
>-- 
>Alexey

-- 
Richard Yang
Help you, Help me

Re: [PATCH 2/2] powerpc/iommu: check dev->iommu_group before remove a device from iommu_group

[Alexey Kardashevskiy]

On 08/19/2013 11:55 AM, Wei Yang wrote:
> On Mon, Aug 19, 2013 at 11:39:49AM +1000, Alexey Kardashevskiy wrote:
>> On 08/19/2013 11:29 AM, Wei Yang wrote:
>>> On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
>>>> On 08/16/2013 08:08 PM, Wei Yang wrote:
>>>>> ---
>>>>>  arch/powerpc/kernel/iommu.c |    3 ++-
>>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>>>>> index b20ff17..5abf7c3 100644
>>>>> --- a/arch/powerpc/kernel/iommu.c
>>>>> +++ b/arch/powerpc/kernel/iommu.c
>>>>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>>>>>  	case BUS_NOTIFY_ADD_DEVICE:
>>>>>  		return iommu_add_device(dev);
>>>>>  	case BUS_NOTIFY_DEL_DEVICE:
>>>>> -		iommu_del_device(dev);
>>>>> +		if (dev->iommu_group)
>>>>> +			iommu_del_device(dev);
>>>>>  		return 0;
>>>>>  	default:
>>>>>  		return 0;
>>>>>
>>>>
>>>> This one seems redundant, no?
>>>
>>> Sorry for the late.
>>>
>>> Yes, these two patches have the same purpose to guard the system, while in two
>>> different places.  One is in powernv platform, the other is in the generic iommu 
>>> driver.
>>>
>>> The one in powernv platform is used to correct the original logic.
>>>
>>> The one in generic iommu driver is to keep system safe in case other platform to
>>> call iommu_group_remove_device() without the check.
>>
>>
>> But I am moving bus notifier to powernv code (posted a patch last week,
>> otherwise Freescale's IOMMU conflicted) so this won't be the case.
> 
> Yes, I see the patch.
> 
> This means other platforms, besides powernv, will check the dev->iommu_group
> before remove the device? This would be a convention?
> 
> If this is the case, the second patch is enough. We don't need to check it in
> generic iommu driver.
> 
> Since I am not very familiar with the code convention, I post these two
> patches together. This doesn't mean I need to push both of them. Your comments
> are welcome, lets me understand which one is more suitable in this case.


Ok. So. I included the check in the bus notifier which I moved to powernv
platform, I guess I'll repost the series soon.

Good luck with pushing the fix for drivers/iommu/iommu.c :)



-- 
Alexey

Re: [PATCH 2/2] powerpc/iommu: check dev->iommu_group before remove a device from iommu_group

[Wei Yang]

On Thu, Aug 22, 2013 at 05:23:34PM +1000, Alexey Kardashevskiy wrote:
>On 08/19/2013 11:55 AM, Wei Yang wrote:
>> On Mon, Aug 19, 2013 at 11:39:49AM +1000, Alexey Kardashevskiy wrote:
>>> On 08/19/2013 11:29 AM, Wei Yang wrote:
>>>> On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
>>>>> On 08/16/2013 08:08 PM, Wei Yang wrote:
>>>>>> ---
>>>>>>  arch/powerpc/kernel/iommu.c |    3 ++-
>>>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>>>>>> index b20ff17..5abf7c3 100644
>>>>>> --- a/arch/powerpc/kernel/iommu.c
>>>>>> +++ b/arch/powerpc/kernel/iommu.c
>>>>>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>>>>>>  	case BUS_NOTIFY_ADD_DEVICE:
>>>>>>  		return iommu_add_device(dev);
>>>>>>  	case BUS_NOTIFY_DEL_DEVICE:
>>>>>> -		iommu_del_device(dev);
>>>>>> +		if (dev->iommu_group)
>>>>>> +			iommu_del_device(dev);
>>>>>>  		return 0;
>>>>>>  	default:
>>>>>>  		return 0;
>>>>>>
>>>>>
>>>>> This one seems redundant, no?
>>>>
>>>> Sorry for the late.
>>>>
>>>> Yes, these two patches have the same purpose to guard the system, while in two
>>>> different places.  One is in powernv platform, the other is in the generic iommu 
>>>> driver.
>>>>
>>>> The one in powernv platform is used to correct the original logic.
>>>>
>>>> The one in generic iommu driver is to keep system safe in case other platform to
>>>> call iommu_group_remove_device() without the check.
>>>
>>>
>>> But I am moving bus notifier to powernv code (posted a patch last week,
>>> otherwise Freescale's IOMMU conflicted) so this won't be the case.
>> 
>> Yes, I see the patch.
>> 
>> This means other platforms, besides powernv, will check the dev->iommu_group
>> before remove the device? This would be a convention?
>> 
>> If this is the case, the second patch is enough. We don't need to check it in
>> generic iommu driver.
>> 
>> Since I am not very familiar with the code convention, I post these two
>> patches together. This doesn't mean I need to push both of them. Your comments
>> are welcome, lets me understand which one is more suitable in this case.
>
>
>Ok. So. I included the check in the bus notifier which I moved to powernv
>platform, I guess I'll repost the series soon.

Thanks, this check will guard the powernv platform.

>
>Good luck with pushing the fix for drivers/iommu/iommu.c :)
>

Alex,

Sorry for not including you in the very beginning, which may spend you more
efforts to track previous mails in this thread.

Do you think it is reasonable to check the dev->iommu_group in
iommu_group_remove_device()? Or we can count on the bus notifier to check it?

Welcome your suggestions~

>
>
>-- 
>Alexey

-- 
Richard Yang
Help you, Help me

Re: [PATCH 2/2] powerpc/iommu: check dev->iommu_group before remove a device from iommu_group

[Alex Williamson]

On Thu, 2013-08-22 at 15:52 +0800, Wei Yang wrote:
> On Thu, Aug 22, 2013 at 05:23:34PM +1000, Alexey Kardashevskiy wrote:
> >On 08/19/2013 11:55 AM, Wei Yang wrote:
> >> On Mon, Aug 19, 2013 at 11:39:49AM +1000, Alexey Kardashevskiy wrote:
> >>> On 08/19/2013 11:29 AM, Wei Yang wrote:
> >>>> On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
> >>>>> On 08/16/2013 08:08 PM, Wei Yang wrote:
> >>>>>> ---
> >>>>>>  arch/powerpc/kernel/iommu.c |    3 ++-
> >>>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
> >>>>>>
> >>>>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> >>>>>> index b20ff17..5abf7c3 100644
> >>>>>> --- a/arch/powerpc/kernel/iommu.c
> >>>>>> +++ b/arch/powerpc/kernel/iommu.c
> >>>>>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
> >>>>>>  	case BUS_NOTIFY_ADD_DEVICE:
> >>>>>>  		return iommu_add_device(dev);
> >>>>>>  	case BUS_NOTIFY_DEL_DEVICE:
> >>>>>> -		iommu_del_device(dev);
> >>>>>> +		if (dev->iommu_group)
> >>>>>> +			iommu_del_device(dev);
> >>>>>>  		return 0;
> >>>>>>  	default:
> >>>>>>  		return 0;
> >>>>>>
> >>>>>
> >>>>> This one seems redundant, no?
> >>>>
> >>>> Sorry for the late.
> >>>>
> >>>> Yes, these two patches have the same purpose to guard the system, while in two
> >>>> different places.  One is in powernv platform, the other is in the generic iommu 
> >>>> driver.
> >>>>
> >>>> The one in powernv platform is used to correct the original logic.
> >>>>
> >>>> The one in generic iommu driver is to keep system safe in case other platform to
> >>>> call iommu_group_remove_device() without the check.
> >>>
> >>>
> >>> But I am moving bus notifier to powernv code (posted a patch last week,
> >>> otherwise Freescale's IOMMU conflicted) so this won't be the case.
> >> 
> >> Yes, I see the patch.
> >> 
> >> This means other platforms, besides powernv, will check the dev->iommu_group
> >> before remove the device? This would be a convention?
> >> 
> >> If this is the case, the second patch is enough. We don't need to check it in
> >> generic iommu driver.
> >> 
> >> Since I am not very familiar with the code convention, I post these two
> >> patches together. This doesn't mean I need to push both of them. Your comments
> >> are welcome, lets me understand which one is more suitable in this case.
> >
> >
> >Ok. So. I included the check in the bus notifier which I moved to powernv
> >platform, I guess I'll repost the series soon.
> 
> Thanks, this check will guard the powernv platform.
> 
> >
> >Good luck with pushing the fix for drivers/iommu/iommu.c :)
> >
> 
> Alex,
> 
> Sorry for not including you in the very beginning, which may spend you more
> efforts to track previous mails in this thread.
> 
> Do you think it is reasonable to check the dev->iommu_group in
> iommu_group_remove_device()? Or we can count on the bus notifier to check it?
> 
> Welcome your suggestions~

I don't really see the point of patch 1/2. iommu_group_remove_device()
is specifically to remove a device from an iommu_group, so why would you
call it on a device that's not part of an iommu_group.  If you want to
avoid testing dev->iommu_group, then implement the .remove_device
callback rather than using the notifier.  Thanks,

Alex

Re: [PATCH 2/2] powerpc/iommu: check dev->iommu_group before remove a device from iommu_group

[Wei Yang]

On Thu, Aug 22, 2013 at 09:28:23AM -0600, Alex Williamson wrote:
>On Thu, 2013-08-22 at 15:52 +0800, Wei Yang wrote:
>> On Thu, Aug 22, 2013 at 05:23:34PM +1000, Alexey Kardashevskiy wrote:
>> >On 08/19/2013 11:55 AM, Wei Yang wrote:
>> >> On Mon, Aug 19, 2013 at 11:39:49AM +1000, Alexey Kardashevskiy wrote:
>> >>> On 08/19/2013 11:29 AM, Wei Yang wrote:
>> >>>> On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
>> >>>>> On 08/16/2013 08:08 PM, Wei Yang wrote:
>> >>>>>> ---
>> >>>>>>  arch/powerpc/kernel/iommu.c |    3 ++-
>> >>>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>> >>>>>>
>> >>>>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>> >>>>>> index b20ff17..5abf7c3 100644
>> >>>>>> --- a/arch/powerpc/kernel/iommu.c
>> >>>>>> +++ b/arch/powerpc/kernel/iommu.c
>> >>>>>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>> >>>>>>  	case BUS_NOTIFY_ADD_DEVICE:
>> >>>>>>  		return iommu_add_device(dev);
>> >>>>>>  	case BUS_NOTIFY_DEL_DEVICE:
>> >>>>>> -		iommu_del_device(dev);
>> >>>>>> +		if (dev->iommu_group)
>> >>>>>> +			iommu_del_device(dev);
>> >>>>>>  		return 0;
>> >>>>>>  	default:
>> >>>>>>  		return 0;
>> >>>>>>
>> >>>>>
>> >>>>> This one seems redundant, no?
>> >>>>
>> >>>> Sorry for the late.
>> >>>>
>> >>>> Yes, these two patches have the same purpose to guard the system, while in two
>> >>>> different places.  One is in powernv platform, the other is in the generic iommu 
>> >>>> driver.
>> >>>>
>> >>>> The one in powernv platform is used to correct the original logic.
>> >>>>
>> >>>> The one in generic iommu driver is to keep system safe in case other platform to
>> >>>> call iommu_group_remove_device() without the check.
>> >>>
>> >>>
>> >>> But I am moving bus notifier to powernv code (posted a patch last week,
>> >>> otherwise Freescale's IOMMU conflicted) so this won't be the case.
>> >> 
>> >> Yes, I see the patch.
>> >> 
>> >> This means other platforms, besides powernv, will check the dev->iommu_group
>> >> before remove the device? This would be a convention?
>> >> 
>> >> If this is the case, the second patch is enough. We don't need to check it in
>> >> generic iommu driver.
>> >> 
>> >> Since I am not very familiar with the code convention, I post these two
>> >> patches together. This doesn't mean I need to push both of them. Your comments
>> >> are welcome, lets me understand which one is more suitable in this case.
>> >
>> >
>> >Ok. So. I included the check in the bus notifier which I moved to powernv
>> >platform, I guess I'll repost the series soon.
>> 
>> Thanks, this check will guard the powernv platform.
>> 
>> >
>> >Good luck with pushing the fix for drivers/iommu/iommu.c :)
>> >
>> 
>> Alex,
>> 
>> Sorry for not including you in the very beginning, which may spend you more
>> efforts to track previous mails in this thread.
>> 
>> Do you think it is reasonable to check the dev->iommu_group in
>> iommu_group_remove_device()? Or we can count on the bus notifier to check it?
>> 
>> Welcome your suggestions~
>
>I don't really see the point of patch 1/2. iommu_group_remove_device()
>is specifically to remove a device from an iommu_group, so why would you
>call it on a device that's not part of an iommu_group.  If you want to
>avoid testing dev->iommu_group, then implement the .remove_device
>callback rather than using the notifier.  Thanks,
>

You mean the .remove_device like intel_iommu_remove_device()? 

Hmm... this function didn't check the dev->iommu_group and just call
iommu_group_remove_device(). I see this guard is put in iommu_bus_notifier(), 
which will check dev->iommu_group before invoke .remove_device.

Let me explain the case to triger the problem a little. 

On some platform, like powernv, we implement another bus notifier when devices
are added or removed in the system. Like Alexey mentioned, he missed the check
for dev->iommu_group in the notifier before removing it from iommu_group. This
trigger the crash.

So do you think it is reasonable to guard the kernel in
iommu_group_remove_device(), or we give the platform developers the
responsibility to check the dev->iommu_group before calling it?

Thanks~

>Alex

-- 
Richard Yang
Help you, Help me

Re: [PATCH 2/2] powerpc/iommu: check dev->iommu_group before remove a device from iommu_group

[Alex Williamson]

On Thu, 2013-08-22 at 23:41 +0800, Wei Yang wrote:
> On Thu, Aug 22, 2013 at 09:28:23AM -0600, Alex Williamson wrote:
> >On Thu, 2013-08-22 at 15:52 +0800, Wei Yang wrote:
> >> On Thu, Aug 22, 2013 at 05:23:34PM +1000, Alexey Kardashevskiy wrote:
> >> >On 08/19/2013 11:55 AM, Wei Yang wrote:
> >> >> On Mon, Aug 19, 2013 at 11:39:49AM +1000, Alexey Kardashevskiy wrote:
> >> >>> On 08/19/2013 11:29 AM, Wei Yang wrote:
> >> >>>> On Fri, Aug 16, 2013 at 08:15:36PM +1000, Alexey Kardashevskiy wrote:
> >> >>>>> On 08/16/2013 08:08 PM, Wei Yang wrote:
> >> >>>>>> ---
> >> >>>>>>  arch/powerpc/kernel/iommu.c |    3 ++-
> >> >>>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
> >> >>>>>>
> >> >>>>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> >> >>>>>> index b20ff17..5abf7c3 100644
> >> >>>>>> --- a/arch/powerpc/kernel/iommu.c
> >> >>>>>> +++ b/arch/powerpc/kernel/iommu.c
> >> >>>>>> @@ -1149,7 +1149,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
> >> >>>>>>  	case BUS_NOTIFY_ADD_DEVICE:
> >> >>>>>>  		return iommu_add_device(dev);
> >> >>>>>>  	case BUS_NOTIFY_DEL_DEVICE:
> >> >>>>>> -		iommu_del_device(dev);
> >> >>>>>> +		if (dev->iommu_group)
> >> >>>>>> +			iommu_del_device(dev);
> >> >>>>>>  		return 0;
> >> >>>>>>  	default:
> >> >>>>>>  		return 0;
> >> >>>>>>
> >> >>>>>
> >> >>>>> This one seems redundant, no?
> >> >>>>
> >> >>>> Sorry for the late.
> >> >>>>
> >> >>>> Yes, these two patches have the same purpose to guard the system, while in two
> >> >>>> different places.  One is in powernv platform, the other is in the generic iommu 
> >> >>>> driver.
> >> >>>>
> >> >>>> The one in powernv platform is used to correct the original logic.
> >> >>>>
> >> >>>> The one in generic iommu driver is to keep system safe in case other platform to
> >> >>>> call iommu_group_remove_device() without the check.
> >> >>>
> >> >>>
> >> >>> But I am moving bus notifier to powernv code (posted a patch last week,
> >> >>> otherwise Freescale's IOMMU conflicted) so this won't be the case.
> >> >> 
> >> >> Yes, I see the patch.
> >> >> 
> >> >> This means other platforms, besides powernv, will check the dev->iommu_group
> >> >> before remove the device? This would be a convention?
> >> >> 
> >> >> If this is the case, the second patch is enough. We don't need to check it in
> >> >> generic iommu driver.
> >> >> 
> >> >> Since I am not very familiar with the code convention, I post these two
> >> >> patches together. This doesn't mean I need to push both of them. Your comments
> >> >> are welcome, lets me understand which one is more suitable in this case.
> >> >
> >> >
> >> >Ok. So. I included the check in the bus notifier which I moved to powernv
> >> >platform, I guess I'll repost the series soon.
> >> 
> >> Thanks, this check will guard the powernv platform.
> >> 
> >> >
> >> >Good luck with pushing the fix for drivers/iommu/iommu.c :)
> >> >
> >> 
> >> Alex,
> >> 
> >> Sorry for not including you in the very beginning, which may spend you more
> >> efforts to track previous mails in this thread.
> >> 
> >> Do you think it is reasonable to check the dev->iommu_group in
> >> iommu_group_remove_device()? Or we can count on the bus notifier to check it?
> >> 
> >> Welcome your suggestions~
> >
> >I don't really see the point of patch 1/2. iommu_group_remove_device()
> >is specifically to remove a device from an iommu_group, so why would you
> >call it on a device that's not part of an iommu_group.  If you want to
> >avoid testing dev->iommu_group, then implement the .remove_device
> >callback rather than using the notifier.  Thanks,
> >
> 
> You mean the .remove_device like intel_iommu_remove_device()? 
> 
> Hmm... this function didn't check the dev->iommu_group and just call
> iommu_group_remove_device(). I see this guard is put in iommu_bus_notifier(), 
> which will check dev->iommu_group before invoke .remove_device.
> 
> Let me explain the case to triger the problem a little. 
> 
> On some platform, like powernv, we implement another bus notifier when devices
> are added or removed in the system. Like Alexey mentioned, he missed the check
> for dev->iommu_group in the notifier before removing it from iommu_group. This
> trigger the crash.
> 
> So do you think it is reasonable to guard the kernel in
> iommu_group_remove_device(), or we give the platform developers the
> responsibility to check the dev->iommu_group before calling it?

I don't see it as we need either patch 1/2 or patch 2/2.  We absolutely
need some form of patch 2/2.  Patch 1/2 isn't necessarily bad, but it
facilitates sloppy usage.  The iommu driver shouldn't be calling
iommu_group_remove_device() on arbitrary devices that may or may not be
part of an iommu_group.  Perhaps patch 1/2 should be:

if (WARN_ON(!group))
	return;

Thanks,

Alex

Re: [PATCH 2/2] powerpc/iommu: check dev->iommu_group before remove a device from iommu_group

[Wei Yang]

On Thu, Aug 22, 2013 at 10:17:20AM -0600, Alex Williamson wrote:
>On Thu, 2013-08-22 at 23:41 +0800, Wei Yang wrote:
>> >> 
>> >> Alex,
>> >> 
>> >> Sorry for not including you in the very beginning, which may spend you more
>> >> efforts to track previous mails in this thread.
>> >> 
>> >> Do you think it is reasonable to check the dev->iommu_group in
>> >> iommu_group_remove_device()? Or we can count on the bus notifier to check it?
>> >> 
>> >> Welcome your suggestions~
>> >
>> >I don't really see the point of patch 1/2. iommu_group_remove_device()
>> >is specifically to remove a device from an iommu_group, so why would you
>> >call it on a device that's not part of an iommu_group.  If you want to
>> >avoid testing dev->iommu_group, then implement the .remove_device
>> >callback rather than using the notifier.  Thanks,
>> >
>> 
>> You mean the .remove_device like intel_iommu_remove_device()? 
>> 
>> Hmm... this function didn't check the dev->iommu_group and just call
>> iommu_group_remove_device(). I see this guard is put in iommu_bus_notifier(), 
>> which will check dev->iommu_group before invoke .remove_device.
>> 
>> Let me explain the case to triger the problem a little. 
>> 
>> On some platform, like powernv, we implement another bus notifier when devices
>> are added or removed in the system. Like Alexey mentioned, he missed the check
>> for dev->iommu_group in the notifier before removing it from iommu_group. This
>> trigger the crash.
>> 
>> So do you think it is reasonable to guard the kernel in
>> iommu_group_remove_device(), or we give the platform developers the
>> responsibility to check the dev->iommu_group before calling it?
>
>I don't see it as we need either patch 1/2 or patch 2/2.  We absolutely
>need some form of patch 2/2.  Patch 1/2 isn't necessarily bad, but it
>facilitates sloppy usage.  The iommu driver shouldn't be calling
>iommu_group_remove_device() on arbitrary devices that may or may not be
>part of an iommu_group.  Perhaps patch 1/2 should be:
>
>if (WARN_ON(!group))
>	return;
>

Agree, this one sounds more reasonable. :-)

Since patch 2/2 is merged by Alexey, I will re-send patch 1/2 alone.

Thanks for your comments ~

>Thanks,
>
>Alex
>
>_______________________________________________
>Linuxppc-dev mailing list
>Linuxppc-dev@lists.ozlabs.org
>https://lists.ozlabs.org/listinfo/linuxppc-dev

-- 
Richard Yang
Help you, Help me