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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <541A8062.3020008@intel.com>
Date:	Thu, 18 Sep 2014 09:49:06 +0300
From:	Adrian Hunter <adrian.hunter@...el.com>
To:	Ulf Hansson <ulf.hansson@...aro.org>
CC:	Stephen Warren <swarren@...dotorg.org>,
	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/18/2014 08:25 AM, 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().

That comment is wrong.  mmc_gpio_request_cd() has always set up the 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 ?

Ulf, this should be reverted.

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