linux-kernel - Re: [PATCH 1/1] MTD: UBI: try to avoid program data to NOR flash after erasure interrupted

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]

Date:	Fri, 01 Nov 2013 10:58:04 +0200
From:	Artem Bityutskiy <dedekind1@...il.com>
To:	Qi Wang 王起 "(qiwang)" 
	<qiwang@...ron.com>
Cc:	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1]  MTD: UBI:  try to avoid program data to NOR flash
 after erasure interrupted

Hi,

could you please re-send your patch separately, without quoting any
parts of this conversation, so that I could use 'git am'.

Your patch also contains trailing white-spaces, please, get rid of them
in the next submission.

Also, could you please clearly state whether you have tested this patch
on a real NOR flash or not. If yes, then could you share the chip
vendor/type information?

On Thu, 2013-10-31 at 04:07 +0000, Qi Wang 王起 (qiwang) wrote:

> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -499,59 +499,44 @@ static int nor_erase_prepare(struct ubi_device *ubi, int pnum)
>  	size_t written;
>  	loff_t addr;
>  	uint32_t data = 0;
> -	/*
> -	 * Note, we cannot generally define VID header buffers on stack,
> -	 * because of the way we deal with these buffers (see the header
> -	 * comment in this file). But we know this is a NOR-specific piece of
> -	 * code, so we can do this. But yes, this is error-prone and we should
> -	 * (pre-)allocate VID header buffer instead.
> -	 */

Please, do not remove this comment.

>  	struct ubi_vid_hdr vid_hdr;
> +	struct ubi_ec_hdr ec_hdr;

To make it obvious what the above big comment talks about, could you
please define 'struct ubi_ec_hdr ec_hdr' above that big comment.

Otherwise looks good to me, thank you!


> My Comments for above changing:
> 1. 
> 	-	/*
> 	-	 * Note, we cannot generally define VID header buffers on stack,
> 	-	 * because of the way we deal with these buffers (see the header
> 	-	 * comment in this file). But we know this is a NOR-specific piece of
> 	-	 * code, so we can do this. But yes, this is error-prone and we should
> 	-	 * (pre-)allocate VID header buffer instead.
> 	-	 */
> 	I remove above comment, because I pre-allocate VID header and EC header together. 
> 	So I think no need to emphasize VID header buffers cannot be on stack.
> 	(Maybe my understanding about this comment is error, if so, please correct me)

The problem is that some functions in io.c can read or write _beyond_
sizeof(struct ubi_vid_hdr), but this is only relevant to NAND, not for
NOR, and the code you change is NOR-only. This is why that comment is
there, and I'd like to keep it.

> 2.
> 	why use
> 	"if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR && err != UBI_IO_FF)"
> 	but not 
> 	"if (!err)" 
> 	to judge if need to program '0' to invalid this block.
> 
> 	In case err == UBI_IO_FF_BITFLIPS, err == UBI_IO_BITFLIPS or unexpected value return
> 	from read function, I think UBI still need to invalid this block for above mentioned 
> 	condition. So I use
> 	"if (err != UBI_IO_BAD_HDR_EBADMSG && err != UBI_IO_BAD_HDR && err != UBI_IO_FF)"
> 	to judge. 

In case of UBI_IO_FF (all FFs) UBI will erase the eraseblock before
using it anyway, so invalidation is not necessary.

Thanks!

-- 
Best Regards,
Artem Bityutskiy

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