[PATCH] iommu:Check that iommu_device_create has completed successfully in alloc_iommu

[PATCH] iommu:Check that iommu_device_create has completed successfully in alloc_iommu

[Nicholas Krause]

This adds the proper check to alloc_iommu to make sure that the call
to iommu_device_create has completed successfully and if not return
to the caller the error code returned after freeing up resources
allocated previously by alloc_iommu.

Signed-off-by: Nicholas Krause <xerofoify-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/iommu/dmar.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 80e3c17..27333b6 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -1069,9 +1069,12 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
 		iommu->iommu_dev = iommu_device_create(NULL, iommu,
 						       intel_iommu_groups,
 						       "%s", iommu->name);
+	if (IS_ERR(iommu->iommu_dev)) {
+		err = PTR_ERR(iommu->iommu_dev);
+		goto err_unmap;
+	}
 
 	return 0;
-
 err_unmap:
 	unmap_iommu(iommu);
 error_free_seq_id:
-- 
2.5.0

Re: [PATCH] iommu:Check that iommu_device_create has completed successfully in alloc_iommu

[Wan ZongShun]

2015-12-29 13:10 GMT+08:00 Nicholas Krause <xerofoify-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> This adds the proper check to alloc_iommu to make sure that the call
> to iommu_device_create has completed successfully and if not return
> to the caller the error code returned after freeing up resources
> allocated previously by alloc_iommu.
>
> Signed-off-by: Nicholas Krause <xerofoify-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/iommu/dmar.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index 80e3c17..27333b6 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -1069,9 +1069,12 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
>                 iommu->iommu_dev = iommu_device_create(NULL, iommu,
>                                                        intel_iommu_groups,
>                                                        "%s", iommu->name);
> +       if (IS_ERR(iommu->iommu_dev)) {
> +               err = PTR_ERR(iommu->iommu_dev);
> +               goto err_unmap;
> +       }

If so, will this bad 'iommu->iommu_dev' break your iommu work?  It
seems not necessary.

>
>         return 0;
> -
>  err_unmap:
>         unmap_iommu(iommu);
>  error_free_seq_id:
> --
> 2.5.0
>
> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu



-- 
---
Vincent Wan(Zongshun)
www.mcuos.com

Re: [PATCH] iommu:Check that iommu_device_create has completed successfully in alloc_iommu

[nick]

On 2015-12-29 02:15 AM, Wan ZongShun wrote:
> 2015-12-29 13:10 GMT+08:00 Nicholas Krause <xerofoify-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> This adds the proper check to alloc_iommu to make sure that the call
>> to iommu_device_create has completed successfully and if not return
>> to the caller the error code returned after freeing up resources
>> allocated previously by alloc_iommu.
>>
>> Signed-off-by: Nicholas Krause <xerofoify-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/iommu/dmar.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
>> index 80e3c17..27333b6 100644
>> --- a/drivers/iommu/dmar.c
>> +++ b/drivers/iommu/dmar.c
>> @@ -1069,9 +1069,12 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
>>                 iommu->iommu_dev = iommu_device_create(NULL, iommu,
>>                                                        intel_iommu_groups,
>>                                                        "%s", iommu->name);
>> +       if (IS_ERR(iommu->iommu_dev)) {
>> +               err = PTR_ERR(iommu->iommu_dev);
>> +               goto err_unmap;
>> +       }
> 
> If so, will this bad 'iommu->iommu_dev' break your iommu work?  It
> seems not necessary.
> 
My concern is that if the iommu device is allocated successfully but not loaded
successfully into sysfs does this not make the device useless to some degree 
and therefore there is no point in having this device being allocated without
it's information being exported in sysfs. Again if it's fine without being 
exported in sysfs that's fine and in that case we should at least print to
the console with KERN_CRIT in order to warn of this critical error to the user.
Again I took the first approach but if you/the maintainer Joerg Roedel feel that
the second idea is better I will be glad to send a patch doing that :).
There are also other callers doing this as I found here so depending on 
how we solve this one I will do the others in a patch series:
http://lxr.free-electrons.com/ident?i=iommu_device_create
Hopes that this explains the patch,
Nick
>>
>>         return 0;
>> -
>>  err_unmap:
>>         unmap_iommu(iommu);
>>  error_free_seq_id:
>> --
>> 2.5.0
>>
>> _______________________________________________
>> iommu mailing list
>> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
> 
> 
> 

Re: [PATCH] iommu:Check that iommu_device_create has completed successfully in alloc_iommu

[Joerg Roedel]

On Tue, Dec 29, 2015 at 12:10:11AM -0500, Nicholas Krause wrote:
> This adds the proper check to alloc_iommu to make sure that the call
> to iommu_device_create has completed successfully and if not return
> to the caller the error code returned after freeing up resources
> allocated previously by alloc_iommu.
> 
> Signed-off-by: Nicholas Krause <xerofoify-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/iommu/dmar.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index 80e3c17..27333b6 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -1069,9 +1069,12 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
>  		iommu->iommu_dev = iommu_device_create(NULL, iommu,
>  						       intel_iommu_groups,
>  						       "%s", iommu->name);
> +	if (IS_ERR(iommu->iommu_dev)) {
> +		err = PTR_ERR(iommu->iommu_dev);
> +		goto err_unmap;
> +	}

This leaves a dangling pointer to the iommu in drhd->iommu.

>  
>  	return 0;
> -

That blank line improves readability of the code, please leave it in.

>  err_unmap:
>  	unmap_iommu(iommu);
>  error_free_seq_id:

Re: [PATCH] iommu:Check that iommu_device_create has completed successfully in alloc_iommu

[Nicholas Krause]

On January 4, 2016 11:16:16 AM EST, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
>On Tue, Dec 29, 2015 at 12:10:11AM -0500, Nicholas Krause wrote:
>> This adds the proper check to alloc_iommu to make sure that the call
>> to iommu_device_create has completed successfully and if not return
>> to the caller the error code returned after freeing up resources
>> allocated previously by alloc_iommu.
>> 
>> Signed-off-by: Nicholas Krause <xerofoify-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/iommu/dmar.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
>> index 80e3c17..27333b6 100644
>> --- a/drivers/iommu/dmar.c
>> +++ b/drivers/iommu/dmar.c
>> @@ -1069,9 +1069,12 @@ static int alloc_iommu(struct dmar_drhd_unit
>*drhd)
>>  		iommu->iommu_dev = iommu_device_create(NULL, iommu,
>>  						       intel_iommu_groups,
>>  						       "%s", iommu->name);
>> +	if (IS_ERR(iommu->iommu_dev)) {
>> +		err = PTR_ERR(iommu->iommu_dev);
>> +		goto err_unmap;
>> +	}
>
>This leaves a dangling pointer to the iommu in drhd->iommu.
Yes that's true and was wondering if you want me to fix it up to set the pointer back to null first or if preferred around 
safe state. 
Nick 
>
>>  
>>  	return 0;
>> -
>
>That blank line improves readability of the code, please leave it in.
>
>>  err_unmap:
>>  	unmap_iommu(iommu);
>>  error_free_seq_id:

Re: [PATCH] iommu:Check that iommu_device_create has completed successfully in alloc_iommu

[Joerg Roedel]

On Mon, Jan 04, 2016 at 11:27:10AM -0500, Nicholas Krause wrote:
> Yes that's true and was wondering if you want me to fix it up to set
> the pointer back to null first or if preferred around safe state. 

The best solution is to set the dmar->iommu pointer after the error
check for iommu_device_create().



	Joerg

[PATCH] iommu:Check that iommu_device_create has completed successfully in alloc_iommu

[Nicholas Krause]

This adds the proper check to alloc_iommu to make sure that the call
to iommu_device_create has completed successfully and if not return
to the caller the error code returned after freeing up resources
allocated previously by alloc_iommu.

Signed-off-by: Nicholas Krause <xerofoify-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/iommu/dmar.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 80e3c17..a496d73 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -1069,6 +1069,12 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
 		iommu->iommu_dev = iommu_device_create(NULL, iommu,
 						       intel_iommu_groups,
 						       "%s", iommu->name);
+
+	if (IS_ERR(iommu->iommu_dev)) {
+		drhd->iommu = NULL;
+		err = PTR_ERR(iommu->iommu_dev);
+		goto err_unmap;
+	}
 
 	return 0;
 
-- 
2.5.0