Re: Kernel segmentation fault unbinding eMMC on AML-S905X-CC

[Remi Pommarel <repk@triplefau.lt>]

On Wed, Jan 09, 2019 at 10:48:50PM +0000, Elie Roudninski wrote:
...

> [  444.060161] Call trace:
> [  444.062580]  queued_spin_lock_slowpath+0x228/0x2d0
> [  444.067322]  meson_mmc_irq+0x290/0x2a0
> [  444.071032]  __free_irq+0x184/0x318
> [  444.074480]  free_irq+0x40/0x80
> [  444.077586]  devm_irq_release+0x24/0x30
> [  444.081382]  release_nodes+0x1e0/0x2e0
> [  444.085089]  devres_release_all+0x60/0x88
> [  444.089057]  device_release_driver_internal+0x1c8/0x248
> [  444.094231]  device_release_driver+0x28/0x38
> [  444.098459]  unbind_store+0xdc/0x148
> [  444.101994]  drv_attr_store+0x40/0x58
> [  444.105627]  sysfs_kf_write+0x5c/0x78
> [  444.109240]  kernfs_fop_write+0xe8/0x1e0
> [  444.113126]  __vfs_write+0x60/0x190
> [  444.116570]  vfs_write+0xac/0x1b0
> [  444.119848]  ksys_write+0x6c/0xd0
> [  444.123125]  __arm64_sys_write+0x24/0x30
> [  444.127015]  el0_svc_common+0x94/0xe8
> [  444.130629]  el0_svc_handler+0x38/0x80
> [  444.134340]  el0_svc+0x8/0xc
> [  444.137187] Code: d37c0401 910020c0 8b010041 f865d8e5 (f8256826)
> [  444.143223] ---[ end trace 62541ec2ffe020f2 ]---
> Segmentation fault

...

Here are my two cents on this.

The irq that is being freed was allocated through device management
(i.e. with devm_request_threaded_irq()). So this irq will be released
after meson_mmc_remove() completion, thus after that mmc_free_host()
has been called. Because this irq has been registered with IRQF_SHARED
and if you have CONFIG_DEBUG_SHIRQ (do you ?) the irq handler will be
called while the irq is freed.

So meson_mmc_irq could be called after mmc_free_host() has freed the
meson_host pointer causing it to dereference an invalid pointer.

So if you have CONFIG_DEBUG_SHIRQ could you please test the following
patch:

------------------------------- 8< ----------------------------------
diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index c2690c1a50ff..3a9679531520 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -179,6 +179,8 @@ struct meson_host {
 	struct sd_emmc_desc *descs;
 	dma_addr_t descs_dma_addr;
 
+	int irq;
+
 	bool vqmmc_enabled;
 };
 
@@ -1231,7 +1233,7 @@ static int meson_mmc_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct meson_host *host;
 	struct mmc_host *mmc;
-	int ret, irq;
+	int ret;
 
 	mmc = mmc_alloc_host(sizeof(struct meson_host), &pdev->dev);
 	if (!mmc)
@@ -1276,8 +1278,8 @@ static int meson_mmc_probe(struct platform_device *pdev)
 		goto free_host;
 	}
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq <= 0) {
+	host->irq = platform_get_irq(pdev, 0);
+	if (host->irq <= 0) {
 		dev_err(&pdev->dev, "failed to get interrupt resource.\n");
 		ret = -EINVAL;
 		goto free_host;
@@ -1331,9 +1333,8 @@ static int meson_mmc_probe(struct platform_device *pdev)
 	writel(IRQ_CRC_ERR | IRQ_TIMEOUTS | IRQ_END_OF_CHAIN,
 	       host->regs + SD_EMMC_IRQ_EN);
 
-	ret = devm_request_threaded_irq(&pdev->dev, irq, meson_mmc_irq,
-					meson_mmc_irq_thread, IRQF_SHARED,
-					NULL, host);
+	ret = request_threaded_irq(host->irq, meson_mmc_irq,
+			meson_mmc_irq_thread, IRQF_SHARED, NULL, host);
 	if (ret)
 		goto err_init_clk;
 
@@ -1387,6 +1388,7 @@ static int meson_mmc_remove(struct platform_device *pdev)
 
 	/* disable interrupts */
 	writel(0, host->regs + SD_EMMC_IRQ_EN);
+	free_irq(host->irq, host);
 
 	dma_free_coherent(host->dev, SD_EMMC_DESC_BUF_LEN,
 			  host->descs, host->descs_dma_addr);
------------------------------- 8< ----------------------------------

This basically freed the irq in the meson_mmc_remove() so the
meson_mmc_irq() is called before mmc_free_host().

-- 
Rémi Pommarel

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic