[PATCH] sdio: Fix crash in mmc_attach_sdio() error path

[PATCH] sdio: Fix crash in mmc_attach_sdio() error path

[Daniel Drake]

At http://dev.laptop.org/ticket/9707 we are seeing a crash in
the error path of mmc_attach_sdio()

BUG: unable to handle kernel paging request at 6b6b6c57
IP: [<b066d6e2>] sdio_remove_func+0x9/0x27
Call Trace:
[<b066cfb4>] ? mmc_sdio_remove+0x34/0x65
[<b066d1fc>] ? mmc_attach_sdio+0x217/0x240
[<b066a22f>] ? mmc_rescan+0x1a2/0x20f
[<b042e9a0>] ? worker_thread+0x156/0x1e1

Matt Fleming pointed out that this is because more cards are being
removed than have been initialized/added in the error path.

Fix the error path to handle failures gracefully. Based on earlier
patch from Matt.

Signed-off-by: Daniel Drake <dsd@laptop.org>
---
 drivers/mmc/core/sdio.c     |    6 ++++--
 drivers/mmc/core/sdio_bus.c |    7 +++++--
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index cdb845b..fa07d4f 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -516,7 +516,8 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 	 * The number of functions on the card is encoded inside
 	 * the ocr.
 	 */
-	card->sdio_funcs = funcs = (ocr & 0x70000000) >> 28;
+	funcs = (ocr & 0x70000000) >> 28;
+	card->sdio_funcs = 0;
 
 	/*
 	 * If needed, disconnect card detection pull-up resistor.
@@ -528,10 +529,11 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
 	/*
 	 * Initialize (but don't add) all present functions.
 	 */
-	for (i = 0;i < funcs;i++) {
+	for (i = 0;i < funcs;i++, card->sdio_funcs++) {
 		err = sdio_init_func(host->card, i + 1);
 		if (err)
 			goto remove;
+		card->sdio_funcs = i + 1;
 	}
 
 	mmc_release_host(host);
diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index d37464e..9e060c8 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -248,12 +248,15 @@ int sdio_add_func(struct sdio_func *func)
 /*
  * Unregister a SDIO function with the driver model, and
  * (eventually) free it.
+ * This function can be called through error paths where sdio_add_func() was
+ * never executed (because a failure occurred at an earlier point).
  */
 void sdio_remove_func(struct sdio_func *func)
 {
-	if (sdio_func_present(func))
-		device_del(&func->dev);
+	if (!sdio_func_present(func))
+		return;
 
+	device_del(&func->dev);
 	put_device(&func->dev);
 }
 
-- 
1.6.2.5

Re: [PATCH] sdio: Fix crash in mmc_attach_sdio() error path

[Matt Fleming]

On Tue, Dec 01, 2009 at 03:13:00PM +0000, Daniel Drake wrote:
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index cdb845b..fa07d4f 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -516,7 +516,8 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>  	 * The number of functions on the card is encoded inside
>  	 * the ocr.
>  	 */
> -	card->sdio_funcs = funcs = (ocr & 0x70000000) >> 28;
> +	funcs = (ocr & 0x70000000) >> 28;
> +	card->sdio_funcs = 0;
>  
>  	/*
>  	 * If needed, disconnect card detection pull-up resistor.
> @@ -528,10 +529,11 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>  	/*
>  	 * Initialize (but don't add) all present functions.
>  	 */
> -	for (i = 0;i < funcs;i++) {
> +	for (i = 0;i < funcs;i++, card->sdio_funcs++) {
>  		err = sdio_init_func(host->card, i + 1);
>  		if (err)
>  			goto remove;
> +		card->sdio_funcs = i + 1;
>  	}

I don't understand what you're trying to do here. The card->sdio_funcs++
should take care of incrementing the sdio_funcs count properly. When the
loop terminates both "i" and "card->sdio_funcs" will be equal to
"funcs", unless we jump to the remove label. Unless I've missed
something?

Isn't this hunk below fixing a slightly different bug? Admittedly, it
could cause a crash, but I think it warrants a separate patch and
changelog.

I assumed you would be sending a patch just for this bug below and not
for the one above (which I is why I submitted one). I wasn't very clear
about that though ;-)

> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
> index d37464e..9e060c8 100644
> --- a/drivers/mmc/core/sdio_bus.c
> +++ b/drivers/mmc/core/sdio_bus.c
> @@ -248,12 +248,15 @@ int sdio_add_func(struct sdio_func *func)
>  /*
>   * Unregister a SDIO function with the driver model, and
>   * (eventually) free it.
> + * This function can be called through error paths where sdio_add_func() was
> + * never executed (because a failure occurred at an earlier point).
>   */
>  void sdio_remove_func(struct sdio_func *func)
>  {
> -	if (sdio_func_present(func))
> -		device_del(&func->dev);
> +	if (!sdio_func_present(func))
> +		return;
>  
> +	device_del(&func->dev);
>  	put_device(&func->dev);
>  }
>  
> -- 
> 1.6.2.5
> 

Re: [PATCH] sdio: Fix crash in mmc_attach_sdio() error path

[Daniel Drake]

On Tue, 2009-12-01 at 15:53 +0000, Matt Fleming wrote:
> I don't understand what you're trying to do here. The card->sdio_funcs++
> should take care of incrementing the sdio_funcs count properly. When the
> loop terminates both "i" and "card->sdio_funcs" will be equal to
> "funcs", unless we jump to the remove label. Unless I've missed
> something?

Sorry, that was indeed a small mistake.

> Isn't this hunk below fixing a slightly different bug? Admittedly, it
> could cause a crash, but I think it warrants a separate patch and
> changelog.
> 
> I assumed you would be sending a patch just for this bug below and not
> for the one above (which I is why I submitted one). I wasn't very clear
> about that though ;-)

OK, I'll resend that part alone.

Thanks,
Daniel