diff mbox

drivers: mtd: mtdswap: fix possible null dereference

Message ID 4DD65673.8080801@nokia.com
State New, archived
Headers show

Commit Message

john.maxin@nokia.com May 20, 2011, 11:54 a.m. UTC 
Hi Artem,

On 11-05-20 06:46 AM, ext Artem Bityutskiy wrote:
> On Wed, 2011-05-18 at 13:00 +0100, maxin wrote:
>> This patch fixes the possible null dereference of the variable "oinfo"
> 
> Does this print makes sense? Which ECC? I think it should me something
> like "mtd% does not have OOB", mtd->index.
> 

Thank you very much for reviewing the patch.I have updated the print message 
as per your suggestion as it makes more sense for this case.

> Moreover, you should make the print consistent with the other prints
> which start with MTDSWAP_PREFIX.
> 

I have updated the patch as per your comments.

Signed-off-by: Maxin B. John <john.maxin@nokia.com>
---

Comments

Artem Bityutskiy May 20, 2011, 10:20 a.m. UTC | #1 
On Fri, 2011-05-20 at 12:54 +0100, maxin wrote:
> > Moreover, you should make the print consistent with the other prints
> > which start with MTDSWAP_PREFIX.
> > 
> 
> I have updated the patch as per your comments.
> 
> Signed-off-by: Maxin B. John <john.maxin@nokia.com>
> ---
> diff --git a/drivers/mtd/mtdswap.c b/drivers/mtd/mtdswap.c
> index fed215c..62ddb3a 100644
> --- a/drivers/mtd/mtdswap.c
> +++ b/drivers/mtd/mtdswap.c
> @@ -1450,7 +1450,13 @@ static void mtdswap_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
>  	}
>  
>  	oinfo = mtd->ecclayout;
> -	if (!mtd->oobsize || !oinfo || oinfo->oobavail < MTDSWAP_OOBSIZE) {
> +	if (!oinfo) {
> +		printk(KERN_ERR "%s: mtd%d does not have OOB"

Sorry, but if I save this e-mail and feed it to "git am", the commit
message will contain the discussion (with quotes, etc), not the nice
original commit message. Would you please, send a patch which I can just
save and "git am"?
Artem Bityutskiy May 20, 2011, 12:08 p.m. UTC | #2 
On Fri, 2011-05-20 at 14:22 +0100, maxin wrote:
> This patch fixes the possible null dereference of the variable "oinfo"
> Thanks to Coverity for spotting this.
> 
> Signed-off-by: Maxin B. John <john.maxin@nokia.com>
> ---
> diff --git a/drivers/mtd/mtdswap.c b/drivers/mtd/mtdswap.c
> index fed215c..62ddb3a 100644
> --- a/drivers/mtd/mtdswap.c
> +++ b/drivers/mtd/mtdswap.c
> @@ -1450,7 +1450,13 @@ static void mtdswap_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
>  	}
>  
>  	oinfo = mtd->ecclayout;
> -	if (!mtd->oobsize || !oinfo || oinfo->oobavail < MTDSWAP_OOBSIZE) {
> +	if (!oinfo) {
> +		printk(KERN_ERR "%s: mtd%d does not have OOB"
> +			"\n", MTDSWAP_PREFIX, mtd->index);

Sorry for not noticing this immediately, but we never split the strings
if it is not really necessary. What is the fundamental reason to put
"\n" separately?
Artem Bityutskiy May 20, 2011, 12:10 p.m. UTC | #3 
On Fri, 2011-05-20 at 14:22 +0100, maxin wrote:
> This patch fixes the possible null dereference of the variable "oinfo"
> Thanks to Coverity for spotting this.
> 
> Signed-off-by: Maxin B. John <john.maxin@nokia.com>

Also, when I "git am" your patch, the Author's name will be "maxin", is
this what you want?
john.maxin@nokia.com May 20, 2011, 2:34 p.m. UTC | #4 
Hi Artem,

On 11-05-20 01:08 PM, ext Artem Bityutskiy wrote:
> On Fri, 2011-05-20 at 14:22 +0100, maxin wrote:

> Sorry for not noticing this immediately, but we never split the strings
> if it is not really necessary. What is the fundamental reason to put
> "\n" separately?
>

Thanks for a thorough review. I will modify it accordingly.

>Also, when I "git am" your patch, the Author's name will be "maxin", is
>this what you want?

Hmm.. I haven't noticed this before. Thanks... I have updated the MUA
settings.

Best Regards,
Maxin
diff mbox

Patch

diff --git a/drivers/mtd/mtdswap.c b/drivers/mtd/mtdswap.c
index fed215c..62ddb3a 100644
--- a/drivers/mtd/mtdswap.c
+++ b/drivers/mtd/mtdswap.c
@@ -1450,7 +1450,13 @@  static void mtdswap_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 	}
 
 	oinfo = mtd->ecclayout;
-	if (!mtd->oobsize || !oinfo || oinfo->oobavail < MTDSWAP_OOBSIZE) {
+	if (!oinfo) {
+		printk(KERN_ERR "%s: mtd%d does not have OOB"
+			"\n", MTDSWAP_PREFIX, mtd->index);
+		return;
+	}
+
+	if (!mtd->oobsize || oinfo->oobavail < MTDSWAP_OOBSIZE) {
 		printk(KERN_ERR "%s: Not enough free bytes in OOB, "
 			"%d available, %zu needed.\n",
 			MTDSWAP_PREFIX, oinfo->oobavail, MTDSWAP_OOBSIZE);