cookie_alloc() is called by iommu_get_dma_cookie() and iommu_get_msi_cookie(),
but the mutex is only initialized in iommu_get_dma_cookie(), move mutex_init()
into cookie_alloc() to make sure the mutex will be initialized.
Fixes: ac9a5d522bb8 ("iommu/dma: Fix race condition during iova_domain initialization")
Reported-by: Hulk Robot <[email protected]>
Signed-off-by: Yang Yingliang <[email protected]>
---
drivers/iommu/dma-iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 1910f4f1612b..e29157380c48 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -294,6 +294,7 @@ static struct iommu_dma_cookie *cookie_alloc(enum iommu_dma_cookie_type type)
if (cookie) {
INIT_LIST_HEAD(&cookie->msi_page_list);
cookie->type = type;
+ mutex_init(&cookie->mutex);
}
return cookie;
}
@@ -311,7 +312,6 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
if (!domain->iova_cookie)
return -ENOMEM;
- mutex_init(&domain->iova_cookie->mutex);
return 0;
}
--
2.25.1
Adding Robin.
On Mon, Jun 27, 2022 at 04:55:33PM +0800, Yang Yingliang wrote:
> cookie_alloc() is called by iommu_get_dma_cookie() and iommu_get_msi_cookie(),
> but the mutex is only initialized in iommu_get_dma_cookie(), move mutex_init()
> into cookie_alloc() to make sure the mutex will be initialized.
>
> Fixes: ac9a5d522bb8 ("iommu/dma: Fix race condition during iova_domain initialization")
> Reported-by: Hulk Robot <[email protected]>
> Signed-off-by: Yang Yingliang <[email protected]>
> ---
> drivers/iommu/dma-iommu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 1910f4f1612b..e29157380c48 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -294,6 +294,7 @@ static struct iommu_dma_cookie *cookie_alloc(enum iommu_dma_cookie_type type)
> if (cookie) {
> INIT_LIST_HEAD(&cookie->msi_page_list);
> cookie->type = type;
> + mutex_init(&cookie->mutex);
> }
> return cookie;
> }
> @@ -311,7 +312,6 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
> if (!domain->iova_cookie)
> return -ENOMEM;
>
> - mutex_init(&domain->iova_cookie->mutex);
> return 0;
> }
>
> --
> 2.25.1
On 2022-07-15 08:49, Joerg Roedel wrote:
> Adding Robin.
>
> On Mon, Jun 27, 2022 at 04:55:33PM +0800, Yang Yingliang wrote:
>> cookie_alloc() is called by iommu_get_dma_cookie() and iommu_get_msi_cookie(),
>> but the mutex is only initialized in iommu_get_dma_cookie(), move mutex_init()
>> into cookie_alloc() to make sure the mutex will be initialized.
The mutex is only used in iommu_dma_init_domain(), which is only called
by iommu_setup_dma_ops() for IOMMU_DOMAIN_DMA domains. How is there a
problem here?
Robin.
>> Fixes: ac9a5d522bb8 ("iommu/dma: Fix race condition during iova_domain initialization")
>> Reported-by: Hulk Robot <[email protected]>
>> Signed-off-by: Yang Yingliang <[email protected]>
>> ---
>> drivers/iommu/dma-iommu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 1910f4f1612b..e29157380c48 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -294,6 +294,7 @@ static struct iommu_dma_cookie *cookie_alloc(enum iommu_dma_cookie_type type)
>> if (cookie) {
>> INIT_LIST_HEAD(&cookie->msi_page_list);
>> cookie->type = type;
>> + mutex_init(&cookie->mutex);
>> }
>> return cookie;
>> }
>> @@ -311,7 +312,6 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
>> if (!domain->iova_cookie)
>> return -ENOMEM;
>>
>> - mutex_init(&domain->iova_cookie->mutex);
>> return 0;
>> }
>>
>> --
>> 2.25.1
Hi,
On 2022/7/15 17:28, Robin Murphy wrote:
> On 2022-07-15 08:49, Joerg Roedel wrote:
>> Adding Robin.
>>
>> On Mon, Jun 27, 2022 at 04:55:33PM +0800, Yang Yingliang wrote:
>>> cookie_alloc() is called by iommu_get_dma_cookie() and
>>> iommu_get_msi_cookie(),
>>> but the mutex is only initialized in iommu_get_dma_cookie(), move
>>> mutex_init()
>>> into cookie_alloc() to make sure the mutex will be initialized.
>
> The mutex is only used in iommu_dma_init_domain(), which is only
> called by iommu_setup_dma_ops() for IOMMU_DOMAIN_DMA domains. How is
> there a problem here?
It's no problem now, but I thinks it's better to initialize the 'mutex'
in cookie_alloc() to make code stronger.
Thanks,
Yang
>
> Robin.
>
>>> Fixes: ac9a5d522bb8 ("iommu/dma: Fix race condition during
>>> iova_domain initialization")
>>> Reported-by: Hulk Robot <[email protected]>
>>> Signed-off-by: Yang Yingliang <[email protected]>
>>> ---
>>> drivers/iommu/dma-iommu.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>> index 1910f4f1612b..e29157380c48 100644
>>> --- a/drivers/iommu/dma-iommu.c
>>> +++ b/drivers/iommu/dma-iommu.c
>>> @@ -294,6 +294,7 @@ static struct iommu_dma_cookie
>>> *cookie_alloc(enum iommu_dma_cookie_type type)
>>> if (cookie) {
>>> INIT_LIST_HEAD(&cookie->msi_page_list);
>>> cookie->type = type;
>>> + mutex_init(&cookie->mutex);
>>> }
>>> return cookie;
>>> }
>>> @@ -311,7 +312,6 @@ int iommu_get_dma_cookie(struct iommu_domain
>>> *domain)
>>> if (!domain->iova_cookie)
>>> return -ENOMEM;
>>> - mutex_init(&domain->iova_cookie->mutex);
>>> return 0;
>>> }
>>> --
>>> 2.25.1
> .
On 2022-07-18 07:42, Yang Yingliang wrote:
> Hi,
>
> On 2022/7/15 17:28, Robin Murphy wrote:
>> On 2022-07-15 08:49, Joerg Roedel wrote:
>>> Adding Robin.
>>>
>>> On Mon, Jun 27, 2022 at 04:55:33PM +0800, Yang Yingliang wrote:
>>>> cookie_alloc() is called by iommu_get_dma_cookie() and
>>>> iommu_get_msi_cookie(),
>>>> but the mutex is only initialized in iommu_get_dma_cookie(), move
>>>> mutex_init()
>>>> into cookie_alloc() to make sure the mutex will be initialized.
>>
>> The mutex is only used in iommu_dma_init_domain(), which is only
>> called by iommu_setup_dma_ops() for IOMMU_DOMAIN_DMA domains. How is
>> there a problem here?
> It's no problem now, but I thinks it's better to initialize the 'mutex'
> in cookie_alloc() to make code stronger.
Stronger against what, though? The sole reason this mutex exists at all
is as a temporary measure to protect the IOVA domain from concurrent
initialisation - I suppose in hindsight it might have made sense to
define it inside the union, but I don't see much point in churning that
now. I'd rather spend the time on continuing to get the
iommu_probe_device() path sorted out so that we don't have the whole
problematic driver-probe-time-replay mess in the first place and this
mutex can be reverted ASAP.
>>>> Fixes: ac9a5d522bb8 ("iommu/dma: Fix race condition during
>>>> iova_domain initialization")
>>>> Reported-by: Hulk Robot <[email protected]>
Please teach your robot to care about things that actually matter. All
it's "reporting" here is that it isn't clever enough to follow control
flow across multiple compilation units, otherwise it should have seen
the matching iommu_is_dma_domain() checks between __iommu_domain_alloc()
and iommu_setup_dma_ops(). Having to explain this is not a good use of
the kernel community's time and effort.
Thanks,
Robin.
>>>> Signed-off-by: Yang Yingliang <[email protected]>
>>>> ---
>>>> drivers/iommu/dma-iommu.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>>> index 1910f4f1612b..e29157380c48 100644
>>>> --- a/drivers/iommu/dma-iommu.c
>>>> +++ b/drivers/iommu/dma-iommu.c
>>>> @@ -294,6 +294,7 @@ static struct iommu_dma_cookie
>>>> *cookie_alloc(enum iommu_dma_cookie_type type)
>>>> if (cookie) {
>>>> INIT_LIST_HEAD(&cookie->msi_page_list);
>>>> cookie->type = type;
>>>> + mutex_init(&cookie->mutex);
>>>> }
>>>> return cookie;
>>>> }
>>>> @@ -311,7 +312,6 @@ int iommu_get_dma_cookie(struct iommu_domain
>>>> *domain)
>>>> if (!domain->iova_cookie)
>>>> return -ENOMEM;
>>>> - mutex_init(&domain->iova_cookie->mutex);
>>>> return 0;
>>>> }
>>>> --
>>>> 2.25.1
>> .