linux-kernel - Re: [PATCHv2] mmc: block: fix read single on recovery logic

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] [day] [month] [year] [list]

Date:   Tue, 8 Feb 2022 16:12:32 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Adrian Hunter <adrian.hunter@...el.com>,
        Christian Löhle <CLoehle@...erstone.com>
Cc:     "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Avri Altman <avri.altman@....com>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCHv2] mmc: block: fix read single on recovery logic

On Tue, 8 Feb 2022 at 07:37, Adrian Hunter <adrian.hunter@...el.com> wrote:
>
> On 04/02/2022 17:11, Christian Löhle wrote:
> > On reads with MMC_READ_MULTIPLE_BLOCK that fail,
> > the recovery handler will use MMC_READ_SINGLE_BLOCK for
> > each of the blocks, up to MMC_READ_SINGLE_RETRIES times each.
> > The logic for this is fixed to never report unsuccessful reads
> > as success to the block layer.
> >
> > On command error with retries remaining, blk_update_request was
> > called with whatever value error was set last to.
> > In case it was last set to BLK_STS_OK (default), the read will be
> > reported as success, even though there was no data read from the device.
> > This could happen on a CRC mismatch for the response,
> > a card rejecting the command (e.g. again due to a CRC mismatch).
> > In case it was last set to BLK_STS_IOERR, the error is reported correctly,
> > but no retries will be attempted.
> >
> > Fixes: 81196976ed946c ("mmc: block: Add blk-mq support")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Christian Loehle <cloehle@...erstone.com>
>
> Reviewed-by: Adrian Hunter <adrian.hunter@...el.com>

Applied for fixes, thanks to both of you!

Kind regards
Uffe


>
> > ---
> > v2:
> >   - Do not allow data error retries
> >   - Actually retry MMC_READ_SINGLE_RETRIES times instead of
> >   MMC_READ_SINGLE_RETRIES-1
> >
> >
> >  drivers/mmc/core/block.c | 28 ++++++++++++++--------------
> >  1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> > index 4e61b28a002f..8d718aa56d33 100644
> > --- a/drivers/mmc/core/block.c
> > +++ b/drivers/mmc/core/block.c
> > @@ -1682,31 +1682,31 @@ static void mmc_blk_read_single(struct mmc_queue *mq, struct request *req)
> >       struct mmc_card *card = mq->card;
> >       struct mmc_host *host = card->host;
> >       blk_status_t error = BLK_STS_OK;
> > -     int retries = 0;
> >
> >       do {
> >               u32 status;
> >               int err;
> > +             int retries = 0;
> >
> > -             mmc_blk_rw_rq_prep(mqrq, card, 1, mq);
> > +             while (retries++ <= MMC_READ_SINGLE_RETRIES) {
> > +                     mmc_blk_rw_rq_prep(mqrq, card, 1, mq);
> >
> > -             mmc_wait_for_req(host, mrq);
> > +                     mmc_wait_for_req(host, mrq);
> >
> > -             err = mmc_send_status(card, &status);
> > -             if (err)
> > -                     goto error_exit;
> > -
> > -             if (!mmc_host_is_spi(host) &&
> > -                 !mmc_ready_for_data(status)) {
> > -                     err = mmc_blk_fix_state(card, req);
> > +                     err = mmc_send_status(card, &status);
> >                       if (err)
> >                               goto error_exit;
> > -             }
> >
> > -             if (mrq->cmd->error && retries++ < MMC_READ_SINGLE_RETRIES)
> > -                     continue;
> > +                     if (!mmc_host_is_spi(host) &&
> > +                         !mmc_ready_for_data(status)) {
> > +                             err = mmc_blk_fix_state(card, req);
> > +                             if (err)
> > +                                     goto error_exit;
> > +                     }
> >
> > -             retries = 0;
> > +                     if (!mrq->cmd->error)
> > +                             break;
> > +             }
> >
> >               if (mrq->cmd->error ||
> >                   mrq->data->error ||
> >
>