| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 06 Mar 2010 14:54:15 +0200
From: Adrian Hunter <adrian.hunter@...ia.com>
To: Thomas Gleixner <tglx@...utronix.de>
CC: LKML <linux-kernel@...r.kernel.org>,
"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
Madhusudhan Chikkature <madhu.cr@...com>
Subject: Re: [PATCH] mmc: omap_hsmmc: Fix conditional locking
Thomas Gleixner wrote:
> Conditional locking on (!in_interrupt()) is broken by design and there
> is no reason to keep the host->irq_lock across the call to
> mmc_request_done(). Also the host->protect_card magic hack does not
> depend on the context
Card protect does depend on not being invoked in the middle of a request.
For example, it could result in a open-ended read being started but never
stopped.
in_interrupt() can be replaced with a new flag 'in_request' that is set
in omap_hsmmc_request() and cleared after mmc_request_done()
Also, as you allude to in another email, the spinlock, while preventing an
oops, does not stop the unwanted interrupt, which will cause problems.
I can make the necessary changes but it will be a few days before
I have the time.
>
> Fix the mess by dropping host->irq_lock before calling
> mmc_request_done().
>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
>
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 4b23225..99a3383 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -468,14 +468,6 @@ omap_hsmmc_start_command(struct omap_hsmmc_host *host, struct mmc_command *cmd,
> if (host->use_dma)
> cmdreg |= DMA_EN;
>
> - /*
> - * In an interrupt context (i.e. STOP command), the spinlock is unlocked
> - * by the interrupt handler, otherwise (i.e. for a new request) it is
> - * unlocked here.
> - */
> - if (!in_interrupt())
> - spin_unlock_irqrestore(&host->irq_lock, host->flags);
> -
> OMAP_HSMMC_WRITE(host->base, ARG, cmd->arg);
> OMAP_HSMMC_WRITE(host->base, CMD, cmdreg);
> }
> @@ -506,7 +498,9 @@ omap_hsmmc_xfer_done(struct omap_hsmmc_host *host, struct mmc_data *data)
> }
>
> host->mrq = NULL;
> + spin_unlock(&host->irq_lock);
> mmc_request_done(host->mmc, mrq);
> + spin_lock(&host->irq_lock);
> return;
> }
>
> @@ -523,7 +517,9 @@ omap_hsmmc_xfer_done(struct omap_hsmmc_host *host, struct mmc_data *data)
>
> if (!data->stop) {
> host->mrq = NULL;
> + spin_unlock(&host->irq_lock);
> mmc_request_done(host->mmc, data->mrq);
> + spin_lock(&host->irq_lock);
> return;
> }
> omap_hsmmc_start_command(host, data->stop, NULL);
> @@ -551,7 +547,9 @@ omap_hsmmc_cmd_done(struct omap_hsmmc_host *host, struct mmc_command *cmd)
> }
> if ((host->data == NULL && !host->response_busy) || cmd->error) {
> host->mrq = NULL;
> + spin_unlock(&host->irq_lock);
> mmc_request_done(host->mmc, cmd->mrq);
> + spin_lock(&host->irq_lock);
> }
> }
>
> @@ -1077,37 +1075,31 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
> struct omap_hsmmc_host *host = mmc_priv(mmc);
> int err;
>
> + spin_lock_irqsave(&host->irq_lock, host->flags);
> /*
> - * Prevent races with the interrupt handler because of unexpected
> - * interrupts, but not if we are already in interrupt context i.e.
> - * retries.
> + * Protect the card from I/O if there is a possibility
> + * it can be removed.
> */
> - if (!in_interrupt()) {
> - spin_lock_irqsave(&host->irq_lock, host->flags);
> - /*
> - * Protect the card from I/O if there is a possibility
> - * it can be removed.
> - */
> - if (host->protect_card) {
> - if (host->reqs_blocked < 3) {
> - /*
> - * Ensure the controller is left in a consistent
> - * state by resetting the command and data state
> - * machines.
> - */
> - omap_hsmmc_reset_controller_fsm(host, SRD);
> - omap_hsmmc_reset_controller_fsm(host, SRC);
> - host->reqs_blocked += 1;
> - }
> - req->cmd->error = -EBADF;
> - if (req->data)
> - req->data->error = -EBADF;
> - spin_unlock_irqrestore(&host->irq_lock, host->flags);
> - mmc_request_done(mmc, req);
> - return;
> - } else if (host->reqs_blocked)
> - host->reqs_blocked = 0;
> - }
> + if (host->protect_card) {
> + if (host->reqs_blocked < 3) {
> + /*
> + * Ensure the controller is left in a consistent
> + * state by resetting the command and data state
> + * machines.
> + */
> + omap_hsmmc_reset_controller_fsm(host, SRD);
> + omap_hsmmc_reset_controller_fsm(host, SRC);
> + host->reqs_blocked += 1;
> + }
> + req->cmd->error = -EBADF;
> + if (req->data)
> + req->data->error = -EBADF;
> + spin_unlock_irqrestore(&host->irq_lock, host->flags);
> + mmc_request_done(mmc, req);
> + return;
> + } else if (host->reqs_blocked)
> + host->reqs_blocked = 0;
> +
> WARN_ON(host->mrq != NULL);
> host->mrq = req;
> err = omap_hsmmc_prepare_data(host, req);
> @@ -1116,13 +1108,13 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
> if (req->data)
> req->data->error = err;
> host->mrq = NULL;
> - if (!in_interrupt())
> - spin_unlock_irqrestore(&host->irq_lock, host->flags);
> + spin_unlock_irqrestore(&host->irq_lock, host->flags);
> mmc_request_done(mmc, req);
> return;
> }
>
> omap_hsmmc_start_command(host, req->cmd, req->data);
> + spin_unlock_irqrestore(&host->irq_lock, host->flags);
> }
>
> /* Routine to configure clock values. Exposed API to core */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists