[PATCH 0/2] mtd: nandsim: fix error handling

[PATCH 0/2] mtd: nandsim: fix error handling

[Sheng Yong]

These 2 patches fix error handling when nandsim initialization fails.

In alloc_device(), if creating slab memory fails, free_device() will try
to destroy the slab memory without checking if it exists.  PATCH 1 fixes
it.

If something goes wrong in init_nandsim(), it calls free_device() before
returning. However, the caller of init_nandsim() - ns_init_module() - also
does the cleanup by calling free_nandsim(). This causes double free. PATCH
2 fixes it.

Thanks,
Sheng

Sheng Yong (2):
  mtd: nandsim: fix free of NULL pointer
  mtd: nandsim: fix double free

 drivers/mtd/nand/nandsim.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

-- 
1.8.3.4

[PATCH 1/2] mtd: nandsim: fix free of NULL pointer

[Sheng Yong]

If allocating ns->nand_pages_slab fails, do not try to destroy it when
cleaning up nandsim resources.

Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 drivers/mtd/nand/nandsim.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index cb38f3d..33e4064 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -646,7 +646,8 @@ static void free_device(struct nandsim *ns)
 				kmem_cache_free(ns->nand_pages_slab,
 						ns->pages[i].byte);
 		}
-		kmem_cache_destroy(ns->nand_pages_slab);
+		if (ns->nand_pages_slab)
+			kmem_cache_destroy(ns->nand_pages_slab);
 		vfree(ns->pages);
 	}
 }
-- 
1.8.3.4

Re: [PATCH 1/2] mtd: nandsim: fix free of NULL pointer

[Richard Weinberger]

Am 17.06.2015 um 10:45 schrieb Sheng Yong:
> If allocating ns->nand_pages_slab fails, do not try to destroy it when
> cleaning up nandsim resources.
> 
> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
> ---
>  drivers/mtd/nand/nandsim.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
> index cb38f3d..33e4064 100644
> --- a/drivers/mtd/nand/nandsim.c
> +++ b/drivers/mtd/nand/nandsim.c
> @@ -646,7 +646,8 @@ static void free_device(struct nandsim *ns)
>  				kmem_cache_free(ns->nand_pages_slab,
>  						ns->pages[i].byte);
>  		}
> -		kmem_cache_destroy(ns->nand_pages_slab);
> +		if (ns->nand_pages_slab)
> +			kmem_cache_destroy(ns->nand_pages_slab);

It is perfectly fine to free a NULL pointer.

Thanks,
//richard

Re: [PATCH 1/2] mtd: nandsim: fix free of NULL pointer

[Richard Weinberger]

Am 17.06.2015 um 10:48 schrieb Richard Weinberger:
> Am 17.06.2015 um 10:45 schrieb Sheng Yong:
>> If allocating ns->nand_pages_slab fails, do not try to destroy it when
>> cleaning up nandsim resources.
>>
>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
>> ---
>>  drivers/mtd/nand/nandsim.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
>> index cb38f3d..33e4064 100644
>> --- a/drivers/mtd/nand/nandsim.c
>> +++ b/drivers/mtd/nand/nandsim.c
>> @@ -646,7 +646,8 @@ static void free_device(struct nandsim *ns)
>>  				kmem_cache_free(ns->nand_pages_slab,
>>  						ns->pages[i].byte);
>>  		}
>> -		kmem_cache_destroy(ns->nand_pages_slab);
>> +		if (ns->nand_pages_slab)
>> +			kmem_cache_destroy(ns->nand_pages_slab);
> 
> It is perfectly fine to free a NULL pointer.

Ignore that. /me needs more coffee. ;)

Thanks,
//richard

Re: [PATCH 1/2] mtd: nandsim: fix free of NULL pointer

[Sheng Yong]

On 6/17/2015 4:48 PM, Richard Weinberger wrote:
> Am 17.06.2015 um 10:45 schrieb Sheng Yong:
>> If allocating ns->nand_pages_slab fails, do not try to destroy it when
>> cleaning up nandsim resources.
>>
>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
>> ---
>>  drivers/mtd/nand/nandsim.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
>> index cb38f3d..33e4064 100644
>> --- a/drivers/mtd/nand/nandsim.c
>> +++ b/drivers/mtd/nand/nandsim.c
>> @@ -646,7 +646,8 @@ static void free_device(struct nandsim *ns)
>>  				kmem_cache_free(ns->nand_pages_slab,
>>  						ns->pages[i].byte);
>>  		}
>> -		kmem_cache_destroy(ns->nand_pages_slab);
>> +		if (ns->nand_pages_slab)
>> +			kmem_cache_destroy(ns->nand_pages_slab);
> 
> It is perfectly fine to free a NULL pointer.
OK, then maybe the double free is not a serious problem, besides we just
get a message "Trying to vfree() nonexistent vm area" or the like. But
kmem_cache_destroy() will access ns->nand_pages_slab, and ns->nand_pages_slab
is NULL. This will crash the kernel. :)

thanks,
Sheng
> 
> Thanks,
> //richard
> 
> .
> 

Re: [PATCH 1/2] mtd: nandsim: fix free of NULL pointer

[Richard Weinberger]

Am 17.06.2015 um 11:03 schrieb Sheng Yong:
> 
> 
> On 6/17/2015 4:48 PM, Richard Weinberger wrote:
>> Am 17.06.2015 um 10:45 schrieb Sheng Yong:
>>> If allocating ns->nand_pages_slab fails, do not try to destroy it when
>>> cleaning up nandsim resources.
>>>
>>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
>>> ---
>>>  drivers/mtd/nand/nandsim.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
>>> index cb38f3d..33e4064 100644
>>> --- a/drivers/mtd/nand/nandsim.c
>>> +++ b/drivers/mtd/nand/nandsim.c
>>> @@ -646,7 +646,8 @@ static void free_device(struct nandsim *ns)
>>>  				kmem_cache_free(ns->nand_pages_slab,
>>>  						ns->pages[i].byte);
>>>  		}
>>> -		kmem_cache_destroy(ns->nand_pages_slab);
>>> +		if (ns->nand_pages_slab)
>>> +			kmem_cache_destroy(ns->nand_pages_slab);
>>
>> It is perfectly fine to free a NULL pointer.
> OK, then maybe the double free is not a serious problem, besides we just
> get a message "Trying to vfree() nonexistent vm area" or the like. But
> kmem_cache_destroy() will access ns->nand_pages_slab, and ns->nand_pages_slab
> is NULL. This will crash the kernel. :)

Please see my other may, I was wrong. :)

Thanks,
//richard

[PATCH 2/2] mtd: nandsim: fix double free

[Sheng Yong]

Do not call free_device() in init_nandsim, the caller - ns_init_module -
will take care of that if something goes wrong.

Signed-off-by: Sheng Yong <shengyong1@huawei.com>
---
 drivers/mtd/nand/nandsim.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index 33e4064..9c655f1 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -730,8 +730,7 @@ static int init_nandsim(struct mtd_info *mtd)
 	/* Fill the partition_info structure */
 	if (parts_num > ARRAY_SIZE(ns->partitions)) {
 		NS_ERR("too many partitions.\n");
-		ret = -EINVAL;
-		goto error;
+		return -EINVAL;
 	}
 	remains = ns->geom.totsz;
 	next_offset = 0;
@@ -740,8 +739,7 @@ static int init_nandsim(struct mtd_info *mtd)
 
 		if (!part_sz || part_sz > remains) {
 			NS_ERR("bad partition size.\n");
-			ret = -EINVAL;
-			goto error;
+			return -EINVAL;
 		}
 		ns->partitions[i].name   = get_partition_name(i);
 		ns->partitions[i].offset = next_offset;
@@ -753,8 +751,7 @@ static int init_nandsim(struct mtd_info *mtd)
 	if (remains) {
 		if (parts_num + 1 > ARRAY_SIZE(ns->partitions)) {
 			NS_ERR("too many partitions.\n");
-			ret = -EINVAL;
-			goto error;
+			return -EINVAL;
 		}
 		ns->partitions[i].name   = get_partition_name(i);
 		ns->partitions[i].offset = next_offset;
@@ -789,24 +786,18 @@ static int init_nandsim(struct mtd_info *mtd)
 	printk("options: %#x\n",                ns->options);
 
 	if ((ret = alloc_device(ns)) != 0)
-		goto error;
+		return ret;
 
 	/* Allocate / initialize the internal buffer */
 	ns->buf.byte = kmalloc(ns->geom.pgszoob, GFP_KERNEL);
 	if (!ns->buf.byte) {
 		NS_ERR("init_nandsim: unable to allocate %u bytes for the internal buffer\n",
 			ns->geom.pgszoob);
-		ret = -ENOMEM;
-		goto error;
+		return -ENOMEM;
 	}
 	memset(ns->buf.byte, 0xFF, ns->geom.pgszoob);
 
 	return 0;
-
-error:
-	free_device(ns);
-
-	return ret;
 }
 
 /*
-- 
1.8.3.4

Re: [PATCH 0/2] mtd: nandsim: fix error handling

[Sheng Yong]

Ping. Any comments are appreciated :)

thanks,
Sheng

On 6/17/2015 4:45 PM, Sheng Yong wrote:
> These 2 patches fix error handling when nandsim initialization fails.
> 
> In alloc_device(), if creating slab memory fails, free_device() will try
> to destroy the slab memory without checking if it exists.  PATCH 1 fixes
> it.
> 
> If something goes wrong in init_nandsim(), it calls free_device() before
> returning. However, the caller of init_nandsim() - ns_init_module() - also
> does the cleanup by calling free_nandsim(). This causes double free. PATCH
> 2 fixes it.
> 
> Thanks,
> Sheng
> 
> Sheng Yong (2):
>   mtd: nandsim: fix free of NULL pointer
>   mtd: nandsim: fix double free
> 
>  drivers/mtd/nand/nandsim.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
> 

Re: [PATCH 0/2] mtd: nandsim: fix error handling

[Richard Weinberger]

Am 23.06.2015 um 03:03 schrieb Sheng Yong:
> Ping. Any comments are appreciated :)
> 
> thanks,
> Sheng
> 
> On 6/17/2015 4:45 PM, Sheng Yong wrote:
>> These 2 patches fix error handling when nandsim initialization fails.
>>
>> In alloc_device(), if creating slab memory fails, free_device() will try
>> to destroy the slab memory without checking if it exists.  PATCH 1 fixes
>> it.
>>
>> If something goes wrong in init_nandsim(), it calls free_device() before
>> returning. However, the caller of init_nandsim() - ns_init_module() - also
>> does the cleanup by calling free_nandsim(). This causes double free. PATCH
>> 2 fixes it.
>>
>> Thanks,
>> Sheng
>>
>> Sheng Yong (2):
>>   mtd: nandsim: fix free of NULL pointer
>>   mtd: nandsim: fix double free

Both patches look good to me.

Thanks,
//richard

Re: [PATCH 0/2] mtd: nandsim: fix error handling

[Sheng Yong]

On 6/23/2015 2:26 PM, Richard Weinberger wrote:
> Am 23.06.2015 um 03:03 schrieb Sheng Yong:
>> Ping. Any comments are appreciated :)
>>
>> thanks,
>> Sheng
>>
>> On 6/17/2015 4:45 PM, Sheng Yong wrote:
>>> These 2 patches fix error handling when nandsim initialization fails.
>>>
>>> In alloc_device(), if creating slab memory fails, free_device() will try
>>> to destroy the slab memory without checking if it exists.  PATCH 1 fixes
>>> it.
>>>
>>> If something goes wrong in init_nandsim(), it calls free_device() before
>>> returning. However, the caller of init_nandsim() - ns_init_module() - also
>>> does the cleanup by calling free_nandsim(). This causes double free. PATCH
>>> 2 fixes it.
>>>
>>> Thanks,
>>> Sheng
>>>
>>> Sheng Yong (2):
>>>   mtd: nandsim: fix free of NULL pointer
>>>   mtd: nandsim: fix double free
> 
> Both patches look good to me.
Hi, Richard
Can I add your reviewed-by?

thanks,
Sheng
> 
> Thanks,
> //richard
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 
> 

Re: [PATCH 0/2] mtd: nandsim: fix error handling

[Brian Norris]

On Wed, Jun 17, 2015 at 08:45:11AM +0000, Sheng Yong wrote:
> These 2 patches fix error handling when nandsim initialization fails.
> 
> In alloc_device(), if creating slab memory fails, free_device() will try
> to destroy the slab memory without checking if it exists.  PATCH 1 fixes
> it.
> 
> If something goes wrong in init_nandsim(), it calls free_device() before
> returning. However, the caller of init_nandsim() - ns_init_module() - also
> does the cleanup by calling free_nandsim(). This causes double free. PATCH
> 2 fixes it.
> 
> Thanks,
> Sheng
> 
> Sheng Yong (2):
>   mtd: nandsim: fix free of NULL pointer
>   mtd: nandsim: fix double free
> 
>  drivers/mtd/nand/nandsim.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)

This series doesn't apply cleanly to -next. Please rebase on either
linux-next.git or l2-mtd.git:

http://www.linux-mtd.infradead.org/source.html

Thanks,
Brian