linux-kernel - Re: [PATCH] mmc: don't request CD IRQ until mmc_start

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <541B0ACA.6020808@wwwdotorg.org>
Date:	Thu, 18 Sep 2014 10:39:38 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Adrian Hunter <adrian.hunter@...el.com>,
	Ulf Hansson <ulf.hansson@...aro.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>,
	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 09/17/2014 11:25 PM, Adrian Hunter wrote:
> On 09/17/2014 10:57 PM, Stephen Warren wrote:
>> On 09/17/2014 01:55 PM, Ulf Hansson wrote:
>>> On 12 September 2014 19:18, 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 for
>>>> any other direct users of mmc_gpio_request_cd().
>>>>
>>>> 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, and I *assume* that mmc_start_host() is called somehow for
>>>> all host controllers. For SDHCI hosts at least, the typical call path
>>>> that does this is: *_probe() -> sdhci_add_host() -> mmc_add_host() ->
>>>> mmc_start_host(). So, remove the call to mmc_gpiod_request_cd_irq() from
>>>> mmc_gpio_request_cd(). This matches mmc_gpio*d*_request_cd(), which
>>>> already doesn't call mmc_gpiod_request_cd_irq().
>>>>
>>>> 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.
>>>>
>>>> This fixes what I might conclude to be a mistake in commit 740a221ef0e5
>>>> ("mmc: slot-gpio: Add GPIO descriptor based CD GPIO API"), which added the
>>>> call from mmc_start_host() to mmc_gpiod_request_cd_irq(), but also added
>>>> incorrectly added a call from mmc_gpio_request_cd() to
>>>> mmc_gpiod_request_cd_irq().
>>>>
>>>> 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>
>>>
>>> Hi Stephen,
>>>
>>> Thanks for looking into this. It seems like this issue has been
>>> present for quite a while.
>>> I believe your patch should have a stable tag for 3.15+ as well,
>>> unless you object I will add it.
>>
>> Yes, that probably makes sense, thanks.
>
> Doesn't this patch break the drivers that call mmc_gpio_request_cd() after
> mmc_add_host() like mmc_spi.c or sdhci-sirf.c or tmio_mmc_pio.c ?

Oh, if there are drivers that do that, this patch might cause an issue.

But why are they doing that? Shouldn't all the drivers set up the same 
kinds of resources in the same order and way?

--
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/