From patchwork Fri Dec 21 21:32:10 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chris Ball X-Patchwork-Id: 1904781 Return-Path: X-Original-To: patchwork-linux-mmc@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id 87DA03FC64 for ; Fri, 21 Dec 2012 21:32:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751996Ab2LUVcg (ORCPT ); Fri, 21 Dec 2012 16:32:36 -0500 Received: from void.printf.net ([89.145.121.20]:57009 "EHLO void.printf.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751814Ab2LUVcf (ORCPT ); Fri, 21 Dec 2012 16:32:35 -0500 Received: from 173-166-109-249-newengland.hfc.comcastbusiness.net ([173.166.109.249] helo=hydro) by void.printf.net with esmtpsa (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.69) (envelope-from ) id 1TmACj-0004dW-Al; Fri, 21 Dec 2012 21:32:29 +0000 From: Chris Ball To: Linus Torvalds Cc: zhenpeng.qian@tieto.com, Greg KH , Linux Kernel Mailing List , Linux Driver Project , linux-mmc@vger.kernel.org Subject: Re: [PATCH] mmc:core:if detect mmc card remove, don't wait for req done References: <034FBC2C8CCA484FBA5BA5FC707AC8303AD02F0D1E@EXMB02.eu.tieto.com> Date: Fri, 21 Dec 2012 16:32:10 -0500 In-Reply-To: (Linus Torvalds's message of "Thu, 20 Dec 2012 07:49:18 -0800") Message-ID: <87r4mjay79.fsf@laptop.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.4 (gnu/linux) MIME-Version: 1.0 Sender: linux-mmc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-mmc@vger.kernel.org Hi, On Thu, Dec 20 2012, Linus Torvalds wrote: > On Thu, Dec 20, 2012 at 1:32 AM, wrote: >> >> I find a bug about mmc/core/core.c, and send you a patch which I fix >> this bug. >> >> In fact, the bug is a low probability of occurrence. >> >> Hope you help me to analyze it and discuss with me. > > I've added the proper people - Chris Ball and linux-mmc - to the > participants list. > > I also wanted to check with you: the commit message implies that this > can cause an IO timeout and then a call to > msmsdcc_start_command_deferred() with a NULL cmd->mrq. Which I assume > results in a NULL pointer dereference and Oops, but that wasn't > spelled out. > > Chris, please give it a look, and perhaps edit the commit message a bit. Thanks, I've queued the patch for 3.8. I edited the commit message and modified the patch: adding a check in the host driver to avoid any other blind dereferences of cmd->mrq, and skipping the unnecessary assignment to "err". Here's the updated patch -- Qian, does it look okay to you? - Chris. From: Zhenpeng Qian Date: Thu, 20 Dec 2012 16:20:12 +0800 Subject: [PATCH] mmc: core: If card was removed, don't wait for request to finish. mmc_wait_for_req() calls __mmc_start_req(). If __mmc_start_req() detects that the card's been removed, it will return -ENOMEDIUM and not wait for the request to be finished. In this way, it can reduce io_schedule once. And if io_schedule times out and mmc_card_remove() fails, it will call host->ops->request (sometimes this occurs by insmod and rmmod fast), then will get through to msmsdcc_start_command_deferred() finally. cmd->mrq will be NULL -- it's created in mmc_start_request() -- and a NULL deref of cmd->mrq->stop will result. This was seen in msm_sdcc.c, which dereferenced cmd->mrq blindly; also add a check to that host driver to stop this happening again in other potential cases. 10 [] (__dabt_svc) from [] 11 [] (msmsdcc_start_command_deferred) from [] 12 [] (msmsdcc_start_command) from [] 13 [] (msmsdcc_request) from [] 14 [] (mmc_wait_for_req_done) from [] 15 [] (mmc_wait_for_cmd) from [] 16 [] (get_card_status) from [] 17 [] (mmc_blk_err_check) from [] 18 [] (mmc_start_req) from [] 19 [] (mmc_blk_issue_rw_rq) from [] 20 [] (mmc_blk_issue_rq) from [] 21 [] (mmc_queue_thread) from [] 22 [] (kthread) from [] Signed-off-by: Zhenpeng Qian [cjb: Rewrite commit message, add msm_sdcc.c test] Signed-off-by: Chris Ball --- drivers/mmc/core/core.c | 5 +++-- drivers/mmc/host/msm_sdcc.c | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index aaed768..87edbc0 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -470,8 +470,9 @@ EXPORT_SYMBOL(mmc_start_req); */ void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq) { - __mmc_start_req(host, mrq); - mmc_wait_for_req_done(host, mrq); + /* If the card's been removed, don't wait for the request. */ + if (!__mmc_start_req(host, mrq)) + mmc_wait_for_req_done(host, mrq); } EXPORT_SYMBOL(mmc_wait_for_req); diff --git a/drivers/mmc/host/msm_sdcc.c b/drivers/mmc/host/msm_sdcc.c index 7c0af0e..056ccd0 100644 --- a/drivers/mmc/host/msm_sdcc.c +++ b/drivers/mmc/host/msm_sdcc.c @@ -468,7 +468,7 @@ msmsdcc_start_command_deferred(struct msmsdcc_host *host, host->prog_enable = true; } - if (cmd == cmd->mrq->stop) + if (cmd->mrq && (cmd == cmd->mrq->stop)) *c |= MCI_CSPM_MCIABORT; if (snoop_cccr_abort(cmd)) @@ -559,7 +559,7 @@ msmsdcc_start_data(struct msmsdcc_host *host, struct mmc_data *data, static void msmsdcc_start_command(struct msmsdcc_host *host, struct mmc_command *cmd, u32 c) { - if (cmd == cmd->mrq->stop) + if (cmd->mrq && (cmd == cmd->mrq->stop)) c |= MCI_CSPM_MCIABORT; host->stats.cmds++;