| 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: Wed, 26 Aug 2015 15:57:00 +0000
From: Bean Huo 霍斌斌 (beanhuo)
<beanhuo@...ron.com>
To: Brian Norris <computersforpeace@...il.com>,
Xander Huff <xander.huff@...com>
CC: "dwmw2@...radead.org" <dwmw2@...radead.org>,
"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"jeff.westfahl@...com" <jeff.westfahl@...com>,
"jaeden.amero@...com" <jaeden.amero@...com>,
"joshc@...com" <joshc@...com>, Ben Shelton <ben.shelton@...com>
Subject: RE: [RESEND RESEND RESEND PATCH v2] mtd: nand_bbt: scan for next
free bbt block if writing bbt fails
> + Bean, who is looking at refactoring this driver. Perhaps he can review
> this.
>
> On Tue, Aug 25, 2015 at 12:49:26PM -0500, Xander Huff wrote:
> > From: Ben Shelton <ben.shelton@...com>
> >
> > If erasing or writing the BBT fails, we should mark the current BBT
> > block as bad and use the BBT descriptor to scan for the next available
> > unused block in the BBT. We should only return a failure if there
> > isn't any space left.
> >
> > Based on original code implemented by Jeff Westfahl
> > <jeff.westfahl@...com>.
> >
> > Signed-off-by: Ben Shelton <ben.shelton@...com>
> > Reviewed-by: Jaeden Amero <jaeden.amero@...com>
> > Suggested-by: Jeff Westfahl <jeff.westfahl@...com>
> > Signed-off-by: Xander Huff <xander.huff@...com>
> > ---
> > This v2 patch is in reply to comments from Brian Norris on 7/22/13.
> > See the following links for context:
> > http://lists.infradead.org/pipermail/linux-mtd/2013-July/047596.html
> > http://patchwork.ozlabs.org/patch/244324/
>
> Wow, a blast from the past...
>
> > ---
> > drivers/mtd/nand/nand_base.c | 4 ++++ drivers/mtd/nand/nand_bbt.c
> > | 34 ++++++++++++++++++++++++++++++++--
> > include/linux/mtd/nand.h | 7 +++++++
> > 3 files changed, 43 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/nand_base.c
> > b/drivers/mtd/nand/nand_base.c index ceb68ca..48299dc 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -2761,6 +2761,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct
> erase_info *instr,
> > pr_debug("%s: device is write protected!\n",
> > __func__);
> > instr->state = MTD_ERASE_FAILED;
> > + instr->priv = NAND_ERASE_WRITE_PROTECTED;
>
Here will overload the 'priv' field of mtdchar.c
Please go thought mtdchar_ioctl() function.
> I'm pretty sure this is an illegal overloading of the 'priv' field.
> Notice how ioctl(MEMERASE64) uses this field in mtdchard.c. So I suspect
> you've broken char access to /dev/mtdX. Try 'flash_erase' from mtd-utils, for
> instance.
>
> > goto erase_exit;
> > }
> >
> > @@ -2776,6 +2777,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct
> erase_info *instr,
> > pr_warn("%s: attempt to erase a bad block at page
> 0x%08x\n",
> > __func__, page);
> > instr->state = MTD_ERASE_FAILED;
> > + instr->priv = NAND_ERASE_BAD_BLOCK;
> > goto erase_exit;
> > }
> >
> > @@ -2802,6 +2804,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct
> erase_info *instr,
> > pr_debug("%s: failed erase, page 0x%08x\n",
> > __func__, page);
> > instr->state = MTD_ERASE_FAILED;
> > + instr->priv = NAND_ERASE_BLOCK_ERASE_FAILED;
> > instr->fail_addr =
> > ((loff_t)page << chip->page_shift);
> > goto erase_exit;
> > @@ -2819,6 +2822,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct
> erase_info *instr,
> > }
> > }
> > instr->state = MTD_ERASE_DONE;
> > + instr->priv = NAND_ERASE_OK;
> >
> > erase_exit:
> >
> > diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
> > index 63a1a36..09f9e62 100644
> > --- a/drivers/mtd/nand/nand_bbt.c
> > +++ b/drivers/mtd/nand/nand_bbt.c
> > @@ -662,6 +662,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
> > page = td->pages[chip];
> > goto write;
> > }
> > +next:
> >
> > /*
> > * Automatic placement of the bad block table. Search direction
> @@
> > -787,13 +788,42 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
> > einfo.addr = to;
> > einfo.len = 1 << this->bbt_erase_shift;
> > res = nand_erase_nand(mtd, &einfo, 1);
> > - if (res < 0)
> > + if (res == -EIO && einfo.state == MTD_ERASE_FAILED
> > + && einfo.priv == NAND_ERASE_BLOCK_ERASE_FAILED) {
>
> Do you actually need that last condition? What's wrong with the first two?
>
> > + /* This block is bad. Mark it as such and see if
> > + * there's another block available in the BBT area. */
> > + int block = page >>
> > + (this->bbt_erase_shift - this->page_shift);
> > + pr_info("nand_bbt: failed to erase block %d when writing
> BBT\n",
> > + block);
> > + bbt_mark_entry(this, block, BBT_BLOCK_WORN);
> > +
> > + res = this->block_markbad(mtd, block);
> > + if (res)
> > + pr_warn("nand_bbt: error %d while marking block %d
> bad\n",
> > + res, block);
> > + goto next;
> > + } else if (res < 0)
> > goto outerr;
For my knowledge , we don't directly mark this block be a bad block,
Just like ubi layer,this block also need to further testing and verify if
It is real bad block.right?
> >
> > res = scan_write_bbt(mtd, to, len, buf,
> > td->options & NAND_BBT_NO_OOB ? NULL :
> > &buf[len]);
> > - if (res < 0)
> > + if (res == -EIO) {
> > + /* This block is bad. Mark it as such and see if
> > + * there's another block available in the BBT area. */
> > + int block = page >>
> > + (this->bbt_erase_shift - this->page_shift);
> > + pr_info("nand_bbt: failed to erase block %d when writing
> BBT\n",
> > + block);
> > + bbt_mark_entry(this, block, BBT_BLOCK_WORN);
> > +
> > + res = this->block_markbad(mtd, block);
> > + if (res)
> > + pr_warn("nand_bbt: error %d while marking block %d
> bad\n",
> > + res, block);
> > + goto next;
> > + } else if (res < 0)
> > goto outerr;
> >
> > pr_info("Bad block table written to 0x%012llx, version 0x%02X\n",
> > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h index
> > 272f429..86e11f6 100644
> > --- a/include/linux/mtd/nand.h
> > +++ b/include/linux/mtd/nand.h
> > @@ -1030,4 +1030,11 @@ struct nand_sdr_timings {
> >
> > /* get timing characteristics from ONFI timing mode. */ const struct
> > nand_sdr_timings *onfi_async_timing_mode_to_sdr_timings(int mode);
> > +
> > +/* reasons for erase failures */
> > +#define NAND_ERASE_OK 0
> > +#define NAND_ERASE_WRITE_PROTECTED 1
> > +#define NAND_ERASE_BAD_BLOCK 2
> > +#define NAND_ERASE_BLOCK_ERASE_FAILED 3
>
> Why exactly do you need these statuses? I thought the existing error codes
> were sufficient..
>
> > +
> > #endif /* __LINUX_MTD_NAND_H */
>
> Brian
--
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