| 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
| ||
|
Message-ID: <39569ebb-d9a2-4f81-9abe-aec98f3c9f67@intel.com>
Date: Mon, 26 Jan 2026 15:43:14 +0200
From: Adrian Hunter <adrian.hunter@...el.com>
To: Penghe Geng <pgeng@...dia.com>, Ulf Hansson <ulf.hansson@...aro.org>
CC: <linux-mmc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<stable@...r.kernel.org>
Subject: Re: [PATCH mmc v1] mmc: core: Fix bitfield race between retune and
host claiming
On 15/01/2026 23:46, Penghe Geng wrote:
> The host->claimed flag shares a bitfield storage word with several
> retune flags (retune_now, retune_paused, can_retune, doing_retune,
> doing_init_tune). Updating those flags without host->lock can RMW the
> shared word and clear claimed, triggering spurious
> WARN_ON(!host->claimed).
Thanks for finding this!
The design is that those members are protected by the host->claimed
lock itself.
mmc operations are primarily single-threaded, protected by the
host->claimed lock, although the block driver does allow multiple
transfers at the same time in some cases.
There are also other contexts like interrupt handlers.
Can you provide some information about when WARN_ON(!host->claimed)
is being hit? Including the stack dump?
What kernel version?
Is it eMMC, SDIO or SD card?
Is CQE being used?
Are there any I/O errors happening also?
What controller driver is it?
In any case, the use of bit fields seems to add complexity unnecessarily,
so we should consider converting some or all of them to bool.
>
> Serialize all retune bitfield updates with host->lock. Provide lockless
> __mmc_retune_* helpers so callers that already hold host->lock can
> avoid deadlocks while public wrappers serialize updates. Also protect
> doing_init_tune and the CQE retune_now assignment with host->lock.
>
> Fixes: dfa13ebbe334 ("mmc: host: Add facility to support re-tuning")
> Cc: stable@...r.kernel.org
> Signed-off-by: Penghe Geng <pgeng@...dia.com>
> ---
> drivers/mmc/core/host.c | 60 +++++++++++++++++++++++++++++++---------
> drivers/mmc/core/host.h | 35 ++++++++++++++++++++++-
> drivers/mmc/core/mmc.c | 6 ++++
> drivers/mmc/core/queue.c | 3 ++
> include/linux/mmc/host.h | 4 +++
> 5 files changed, 94 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 88c95dbfd9cf..0b6b4a31f629 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -109,7 +109,11 @@ void mmc_unregister_host_class(void)
> */
> void mmc_retune_enable(struct mmc_host *host)
> {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> host->can_retune = 1;
> + spin_unlock_irqrestore(&host->lock, flags);
> if (host->retune_period)
> mod_timer(&host->retune_timer,
> jiffies + host->retune_period * HZ);
> @@ -121,18 +125,31 @@ void mmc_retune_enable(struct mmc_host *host)
> */
> void mmc_retune_pause(struct mmc_host *host)
> {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> if (!host->retune_paused) {
> host->retune_paused = 1;
> - mmc_retune_hold(host);
> + __mmc_retune_hold(host);
> }
> + spin_unlock_irqrestore(&host->lock, flags);
> }
> EXPORT_SYMBOL(mmc_retune_pause);
>
> void mmc_retune_unpause(struct mmc_host *host)
> {
> + unsigned long flags;
> + bool released;
> +
> + spin_lock_irqsave(&host->lock, flags);
> if (host->retune_paused) {
> host->retune_paused = 0;
> - mmc_retune_release(host);
> + released = __mmc_retune_release(host);
> + spin_unlock_irqrestore(&host->lock, flags);
> + if (!released)
> + WARN_ON(1);
> + } else {
> + spin_unlock_irqrestore(&host->lock, flags);
> }
> }
> EXPORT_SYMBOL(mmc_retune_unpause);
> @@ -145,8 +162,12 @@ EXPORT_SYMBOL(mmc_retune_unpause);
> */
> void mmc_retune_disable(struct mmc_host *host)
> {
> + unsigned long flags;
> +
> mmc_retune_unpause(host);
> + spin_lock_irqsave(&host->lock, flags);
> host->can_retune = 0;
> + spin_unlock_irqrestore(&host->lock, flags);
> timer_delete_sync(&host->retune_timer);
> mmc_retune_clear(host);
> }
> @@ -159,16 +180,22 @@ EXPORT_SYMBOL(mmc_retune_timer_stop);
>
> void mmc_retune_hold(struct mmc_host *host)
> {
> - if (!host->hold_retune)
> - host->retune_now = 1;
> - host->hold_retune += 1;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> + __mmc_retune_hold(host);
> + spin_unlock_irqrestore(&host->lock, flags);
> }
>
> void mmc_retune_release(struct mmc_host *host)
> {
> - if (host->hold_retune)
> - host->hold_retune -= 1;
> - else
> + unsigned long flags;
> + bool released;
> +
> + spin_lock_irqsave(&host->lock, flags);
> + released = __mmc_retune_release(host);
> + spin_unlock_irqrestore(&host->lock, flags);
> + if (!released)
> WARN_ON(1);
> }
> EXPORT_SYMBOL(mmc_retune_release);
> @@ -177,18 +204,23 @@ int mmc_retune(struct mmc_host *host)
> {
> bool return_to_hs400 = false;
> int err;
> + unsigned long flags;
>
> - if (host->retune_now)
> - host->retune_now = 0;
> - else
> + spin_lock_irqsave(&host->lock, flags);
> + if (!host->retune_now) {
> + spin_unlock_irqrestore(&host->lock, flags);
> return 0;
> + }
> + host->retune_now = 0;
>
> - if (!host->need_retune || host->doing_retune || !host->card)
> + if (!host->need_retune || host->doing_retune || !host->card) {
> + spin_unlock_irqrestore(&host->lock, flags);
> return 0;
> + }
>
> host->need_retune = 0;
> -
> host->doing_retune = 1;
> + spin_unlock_irqrestore(&host->lock, flags);
>
> if (host->ios.timing == MMC_TIMING_MMC_HS400) {
> err = mmc_hs400_to_hs200(host->card);
> @@ -205,7 +237,9 @@ int mmc_retune(struct mmc_host *host)
> if (return_to_hs400)
> err = mmc_hs200_to_hs400(host->card);
> out:
> + spin_lock_irqsave(&host->lock, flags);
> host->doing_retune = 0;
> + spin_unlock_irqrestore(&host->lock, flags);
>
> return err;
> }
> diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h
> index 5941d68ff989..07e4f427fe15 100644
> --- a/drivers/mmc/core/host.h
> +++ b/drivers/mmc/core/host.h
> @@ -21,22 +21,55 @@ int mmc_retune(struct mmc_host *host);
> void mmc_retune_pause(struct mmc_host *host);
> void mmc_retune_unpause(struct mmc_host *host);
>
> -static inline void mmc_retune_clear(struct mmc_host *host)
> +static inline void __mmc_retune_clear(struct mmc_host *host)
> {
> host->retune_now = 0;
> host->need_retune = 0;
> }
>
> +static inline void mmc_retune_clear(struct mmc_host *host)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> + __mmc_retune_clear(host);
> + spin_unlock_irqrestore(&host->lock, flags);
> +}
> +
> +static inline void __mmc_retune_hold(struct mmc_host *host)
> +{
> + if (!host->hold_retune)
> + host->retune_now = 1;
> + host->hold_retune += 1;
> +}
> +
> +static inline bool __mmc_retune_release(struct mmc_host *host)
> +{
> + if (host->hold_retune) {
> + host->hold_retune -= 1;
> + return true;
> + }
> + return false;
> +}
> +
> static inline void mmc_retune_hold_now(struct mmc_host *host)
> {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> host->retune_now = 0;
> host->hold_retune += 1;
> + spin_unlock_irqrestore(&host->lock, flags);
> }
>
> static inline void mmc_retune_recheck(struct mmc_host *host)
> {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> if (host->hold_retune <= 1)
> host->retune_now = 1;
> + spin_unlock_irqrestore(&host->lock, flags);
> }
>
> static inline int mmc_host_can_cmd23(struct mmc_host *host)
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 7c86efb1044a..114febd15f08 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1820,13 +1820,19 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
> goto free_card;
>
> if (mmc_card_hs200(card)) {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> host->doing_init_tune = 1;
> + spin_unlock_irqrestore(&host->lock, flags);
>
> err = mmc_hs200_tuning(card);
> if (!err)
> err = mmc_select_hs400(card);
>
> + spin_lock_irqsave(&host->lock, flags);
> host->doing_init_tune = 0;
> + spin_unlock_irqrestore(&host->lock, flags);
>
> if (err)
> goto free_card;
> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> index 284856c8f655..5e38759c87f5 100644
> --- a/drivers/mmc/core/queue.c
> +++ b/drivers/mmc/core/queue.c
> @@ -237,6 +237,7 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
> enum mmc_issue_type issue_type;
> enum mmc_issued issued;
> bool get_card, cqe_retune_ok;
> + unsigned long flags;
> blk_status_t ret;
>
> if (mmc_card_removed(mq->card)) {
> @@ -297,8 +298,10 @@ static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
> mmc_get_card(card, &mq->ctx);
>
> if (host->cqe_enabled) {
> + spin_lock_irqsave(&host->lock, flags);
> host->retune_now = host->need_retune && cqe_retune_ok &&
> !host->hold_retune;
> + spin_unlock_irqrestore(&host->lock, flags);
> }
>
> blk_mq_start_request(req);
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index e0e2c265e5d1..e7bddbafd1da 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -713,8 +713,12 @@ void mmc_retune_timer_stop(struct mmc_host *host);
>
> static inline void mmc_retune_needed(struct mmc_host *host)
> {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> if (host->can_retune)
> host->need_retune = 1;
> + spin_unlock_irqrestore(&host->lock, flags);
> }
>
> static inline bool mmc_can_retune(struct mmc_host *host)
Powered by blists - more mailing lists