| 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: Tue, 23 Sep 2014 08:59:07 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Stephen Warren <swarren@...dotorg.org>
Cc: Chris Ball <chris@...ntf.net>,
linux-mmc <linux-mmc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Stephen Warren <swarren@...dia.com>,
Russell King <linux@....linux.org.uk>,
Adrian Hunter <adrian.hunter@...el.com>,
Alexandre Courbot <acourbot@...dia.com>,
Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH] mmc: don't request CD IRQ until mmc_start_host()
On 22 September 2014 17:57, Stephen Warren <swarren@...dotorg.org> wrote:
> From: Stephen Warren <swarren@...dia.com>
>
> As soon as the CD IRQ is requested, it can trigger, since it's an
> externally controlled event. If it does, delayed_work host->detect will
> be scheduled.
>
> Many host controller probe()s are roughly structured as:
>
> *_probe() {
> host = sdhci_pltfm_init();
> mmc_of_parse(host->mmc);
> rc = sdhci_add_host(host);
> if (rc) {
> sdhci_pltfm_free();
> return rc;
> }
>
> In 3.17, CD IRQs can are enabled quite early via *_probe() ->
> mmc_of_parse() -> mmc_gpio_request_cd() -> mmc_gpiod_request_cd_irq().
>
> Note that in linux-next, mmc_of_parse() calls mmc_gpio*d*_request_cd()
> rather than mmc_gpio_request_cd(), and mmc_gpio*d*_request_cd() doesn't
> call mmc_gpiod_request_cd_irq(). However, this issue still exists if
> mmc_gpio_request_cd() is called directly before mmc_start_host().
>
> sdhci_add_host() may fail part way through (e.g. due to deferred
> probe for a vmmc regulator), and sdhci_pltfm_free() does nothing to
> unrequest the CD IRQ nor cancel the delayed_work. sdhci_pltfm_free() is
> coded to assume that if sdhci_add_host() failed, then the delayed_work
> cannot (or should not) have been triggered.
>
> This can lead to the following with CONFIG_DEBUG_OBJECTS_* enabled, when
> kfree(host) is eventually called inside sdhci_pltfm_free():
>
> WARNING: CPU: 2 PID: 6 at lib/debugobjects.c:263 debug_print_object+0x8c/0xb4()
> ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x18
>
> The object being complained about is host->detect.
>
> There's no need to request the CD IRQ so early; mmc_start_host() already
> requests it. For most SDHCI hosts at least, the typical call path that
> does this is: *_probe() -> sdhci_add_host() -> mmc_add_host() ->
> mmc_start_host(). Therefore, remove the call to mmc_gpiod_request_cd_irq()
> from mmc_gpio_request_cd(). This also matches mmc_gpio*d*_request_cd(),
> which already doesn't call mmc_gpiod_request_cd_irq().
>
> However, some host controller drivers call mmc_gpio_request_cd() after
> mmc_start_host() has already been called, and assume that this will also
> call mmc_gpiod_request_cd_irq(). Update those drivers to explicitly call
> mmc_gpiod_request_cd_irq() themselves. Ideally, these drivers should be
> modified to move their call to mmc_gpio_request_cd() before their call
> to mmc_add_host(). However that's too large a change for stable.
>
> This solves the problem (eliminates the kernel error message above),
> since it guarantees that the IRQ can't trigger before mmc_start_host()
> is called.
>
> The critical point here is that once sdhci_add_host() calls
> mmc_add_host() -> mmc_start_host(), sdhci_add_host() is coded not to
> fail. In other words, if there's a chance that mmc_start_host() may have
> been called, and CD IRQs triggered, and the delayed_work scheduled,
> sdhci_add_host() won't fail, and so cleanup is no longer via
> sdhci_pltfm_free() (which doesn't free the IRQ or cancel the work queue)
> but instead must be via sdhci_remove_host(), which calls mmc_remove_host()
> -> mmc_stop_host(), which does free the IRQ and cancel the work queue.
>
> CC: Russell King <linux@....linux.org.uk>
> Cc: Adrian Hunter <adrian.hunter@...el.com>
> Cc: Alexandre Courbot <acourbot@...dia.com>
> Cc: Linus Walleij <linus.walleij@...aro.org>
> Signed-off-by: Stephen Warren <swarren@...dia.com>
> ---
> Note: This is a patch for v3.17 (and perhaps earlier); linux-next doesn't
> have this problem due to other code re-structing. However, I think those
> patches are too large to backport.
>
> v3: Explicitly call mmc_gpiod_request_cd_irq() from drivers that call
> mmc_gpio_request_cd() after calling mmc_add_host(), rather than modifying
> the function signature of mmc_gpio_request_cd().
>
> v2: Rather than completely removing mmc_gpio_request_cd()'s call to
> mmc_gpiod_request_cd_irq(), make the call conditional.
I have applied this for next and added a stable tag for 3.15+.
Thanks!
Kind regards
Uffe
> ---
> drivers/mmc/core/slot-gpio.c | 2 --
> drivers/mmc/host/mmc_spi.c | 1 +
> drivers/mmc/host/sdhci-sirf.c | 1 +
> drivers/mmc/host/tmio_mmc_pio.c | 1 +
> 4 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
> index 5f89cb83d5f0..187f48a5795a 100644
> --- a/drivers/mmc/core/slot-gpio.c
> +++ b/drivers/mmc/core/slot-gpio.c
> @@ -221,8 +221,6 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio,
> ctx->override_cd_active_level = true;
> ctx->cd_gpio = gpio_to_desc(gpio);
>
> - mmc_gpiod_request_cd_irq(host);
> -
> return 0;
> }
> EXPORT_SYMBOL(mmc_gpio_request_cd);
> diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> index cc8d4a6099cd..e4a07546f8b6 100644
> --- a/drivers/mmc/host/mmc_spi.c
> +++ b/drivers/mmc/host/mmc_spi.c
> @@ -1436,6 +1436,7 @@ static int mmc_spi_probe(struct spi_device *spi)
> host->pdata->cd_debounce);
> if (status != 0)
> goto fail_add_host;
> + mmc_gpiod_request_cd_irq(mmc);
> }
>
> if (host->pdata && host->pdata->flags & MMC_SPI_USE_RO_GPIO) {
> diff --git a/drivers/mmc/host/sdhci-sirf.c b/drivers/mmc/host/sdhci-sirf.c
> index 17004531d089..b6db259aea9e 100644
> --- a/drivers/mmc/host/sdhci-sirf.c
> +++ b/drivers/mmc/host/sdhci-sirf.c
> @@ -94,6 +94,7 @@ static int sdhci_sirf_probe(struct platform_device *pdev)
> ret);
> goto err_request_cd;
> }
> + mmc_gpiod_request_cd_irq(host->mmc);
> }
>
> return 0;
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index faf0924e71cb..59d9a7249b2e 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -1103,6 +1103,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host **host,
> tmio_mmc_host_remove(_host);
> return ret;
> }
> + mmc_gpiod_request_cd_irq(mmc);
> }
>
> *host = _host;
> --
> 1.9.1
>
--
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