iommu: Prevent oops in iommu_get() and while arch_iommu is in use

PATCH In review Sakari Ailus 2 participants 6 messages Mar 25, 2011

Subsystems (3)

Arm (32-bit) Arm sub-architectures OMAP2+ support

lore

Share & tools

lore

Sakari Ailus

author

2011-03-25 15:17 UTC #0 lore raw

cover iommu: Prevent oops in iommu_get() and while arch_iommu is in use

Hi,

[Resend: the patches were accidentally sent to linux-media instead. Apologies.]

This patchset is aimed to fix a problem in arch_iommu implementation references. When an actual arch_iommu implementation is not loaded while iommu_get() is being called results to a kernel oops, as well as removing an arch_iommu implementation which is in use.

This patchset fixes both issues.

The patchset assumes the arch_iommu is uninstalled at module unload time. Is this an acceptable requirement?

Serialisation of the access to arch_iommu is done using mutex called arch_iommu_mutex.

module_put() doesn't need to have the arch_iommu_mutex since when this gets called there won't be any users on the arch_iommu anyway.

Comments are welcome. :-)

Cheers,

Signature · 2 lines

Sakari Ailus
[email protected]

Sakari Ailus

author

2011-03-25 15:18 UTC #1 lore raw

PATCH 1/4 omap iommu: Check existence of arch_iommu

Check that the arch_iommu has been installed before trying to use it. This will lead to kernel oops if the arch_iommu isn't there.

Signed-off-by Sakari Ailus <[email protected]>

---
 arch/arm/plat-omap/iommu.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

arch/arm/plat-omap/iommu.c +3 -0

index b1107c0..f0fea0b 100644
@@ -104,6 +104,9 @@ static int iommu_enable(struct iommu *obj)
 	if (!obj)
 		return -EINVAL;
 
+	if (!arch_iommu)
+		return -ENOENT;
+
 	clk_enable(obj->clk);
 
 	err = arch_iommu->enable(obj);

Signature · 1 line

1.7.2.3

Sakari Ailus

author

2011-03-25 15:18 UTC #2 lore raw

PATCH 2/4 omap iommu: Add module information to struct iommu_functions

In reply to Sakari Ailus

Whichever module that implements the struct, may not be unloaded while it's in use. Prepare to this by adding module reference to the structure.

Signed-off-by Sakari Ailus <[email protected]>

---
 arch/arm/plat-omap/include/plat/iommu.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

arch/arm/plat-omap/include/plat/iommu.h +1 -0

index 69230d6..26fefb4 100644
@@ -79,6 +79,7 @@ struct iotlb_lock {
 /* architecture specific functions */
 struct iommu_functions {
 	unsigned long	version;
+	struct module *module;
 
 	int (*enable)(struct iommu *obj);
 	void (*disable)(struct iommu *obj);

Signature · 1 line

1.7.2.3

Sakari Ailus

author

2011-03-25 15:18 UTC #3 lore raw

PATCH 3/4 omap2 iommu: Set module information in omap2_iommu_ops

In reply to Sakari Ailus

Set omap2_iommu_ops.module to point to THIS_MODULE.

Signed-off-by Sakari Ailus <[email protected]>

---
 arch/arm/mach-omap2/iommu2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

arch/arm/mach-omap2/iommu2.c +2 -0

index 14ee686..cf0c32a 100644
@@ -342,6 +342,8 @@ static const struct iommu_functions omap2_iommu_ops = {
 	.save_ctx	= omap2_iommu_save_ctx,
 	.restore_ctx	= omap2_iommu_restore_ctx,
 	.dump_ctx	= omap2_iommu_dump_ctx,
+
+	.module		= THIS_MODULE,
 };
 
 static int __init omap2_iommu_init(void)

Signature · 1 line

1.7.2.3

Sakari Ailus

author

2011-03-25 15:18 UTC #4 lore raw

PATCH 4/4 omap iommu: Prevent iommu implementations from being unloaded while in use

In reply to Sakari Ailus

Prevent module implementing arch_iommu from being unloaded while the arch_iommu is in use, essentially while the iommu has been acquired using iommu_get() by increasing module use count.

This assumes that the arch_iommu has to be uninstalled at module unload time.

Signed-off-by Sakari Ailus <[email protected]>

---
 arch/arm/plat-omap/iommu.c |   31 +++++++++++++++++++++++++++++--
 1 files changed, 29 insertions(+), 2 deletions(-)

arch/arm/plat-omap/iommu.c +29 -2

index f0fea0b..430ed94 100644
@@ -35,6 +35,7 @@ static const struct iommu_functions *arch_iommu;
 
 static struct platform_driver omap_iommu_driver;
 static struct kmem_cache *iopte_cachep;
+static struct mutex arch_iommu_mutex;
 
 /**
  * install_iommu_arch - Install archtecure specific iommu functions

@@ -45,10 +46,17 @@ static struct kmem_cache *iopte_cachep;
  **/
 int install_iommu_arch(const struct iommu_functions *ops)
 {
-	if (arch_iommu)
+	mutex_lock(&arch_iommu_mutex);
+
+	if (arch_iommu) {
+		mutex_unlock(&arch_iommu_mutex);
 		return -EBUSY;
+	}
 
 	arch_iommu = ops;
+
+	mutex_unlock(&arch_iommu_mutex);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(install_iommu_arch);

@@ -58,13 +66,22 @@ EXPORT_SYMBOL_GPL(install_iommu_arch);
  * @ops:	a pointer to architecture specific iommu functions
  *
  * This interface uninstalls the iommu algorighm installed previously.
+ *
+ * Note: This function may only be called at module_exit() function of
+ * a module which implements arch_iommu. Otherwise another mechanism
+ * from the module use count only to ensure the arch_iommu stays there
+ * has to be implemented.
  **/
 void uninstall_iommu_arch(const struct iommu_functions *ops)
 {
+	mutex_lock(&arch_iommu_mutex);
+
 	if (arch_iommu != ops)
 		pr_err("%s: not your arch\n", __func__);
 
 	arch_iommu = NULL;
+
+	mutex_unlock(&arch_iommu_mutex);
 }
 EXPORT_SYMBOL_GPL(uninstall_iommu_arch);

@@ -104,14 +121,20 @@ static int iommu_enable(struct iommu *obj)
 	if (!obj)
 		return -EINVAL;
 
-	if (!arch_iommu)
+	mutex_lock(&arch_iommu_mutex);
+	if (!arch_iommu || !try_module_get(arch_iommu->module)) {
+		mutex_unlock(&arch_iommu_mutex);
 		return -ENOENT;
+	}
 
 	clk_enable(obj->clk);
 
 	err = arch_iommu->enable(obj);
 
 	clk_disable(obj->clk);
+
+	mutex_unlock(&arch_iommu_mutex);
+
 	return err;
 }

@@ -125,6 +148,8 @@ static void iommu_disable(struct iommu *obj)
 	arch_iommu->disable(obj);
 
 	clk_disable(obj->clk);
+
+	module_put(arch_iommu->module);
 }
 
 /*

@@ -1058,6 +1083,8 @@ static int __init omap_iommu_init(void)
 		return -ENOMEM;
 	iopte_cachep = p;
 
+	mutex_init(&arch_iommu_mutex);
+
 	return platform_driver_register(&omap_iommu_driver);
 }
 module_init(omap_iommu_init);

Signature · 1 line

1.7.2.3

David Cohen

2011-03-25 15:44 UTC #5 lore raw

In reply to Sakari Ailus

Quoted 1 line · Sakari Ailus

Hi,

Hi Sakari,

Quoted 10 lines

[Resend: the patches were accidentally sent to linux-media instead. Apologies.]
This patchset is aimed to fix a problem in arch_iommu implementation references. When an actual arch_iommu implementation is not loaded while iommu_get() is being called results to a kernel oops, as well as removing an arch_iommu implementation which is in use.
This patchset fixes both issues.

Sounds nice.

Quoted 3 lines

The patchset assumes the arch_iommu is uninstalled at module unload time. Is this an acceptable requirement?

I can't see a reason why this assumption could be wrong. In my point of view it's acceptable. Let's see Hiroshi's.

Regards,

David Cohen

Quoted 11 lines

Serialisation of the access to arch_iommu is done using mutex called arch_iommu_mutex.
module_put() doesn't need to have the arch_iommu_mutex since when this gets called there won't be any users on the arch_iommu anyway.
Comments are welcome. :-)
Cheers,

Quoted 8 lines

--
Sakari Ailus
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html