Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1535603 > unrolled thread

[PATCH 1/1] mtd: ubi: fix improper return value

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/1] mtd: ubi: fix improper return value Pan Bian <bianpan201604@163.com> - 2016-12-04 07:20 +0100
    Re: [PATCH 1/1] mtd: ubi: fix improper return value Marek Vasut <marek.vasut@gmail.com> - 2016-12-04 16:40 +0100
      Re: [PATCH 1/1] mtd: ubi: fix improper return value Joe Perches <joe@perches.com> - 2016-12-04 21:40 +0100
        Re: [PATCH 1/1] mtd: ubi: fix improper return value Richard Weinberger <richard@nod.at> - 2016-12-04 22:00 +0100
          Re: [PATCH 1/1] mtd: ubi: fix improper return value Artem Bityutskiy <dedekind1@gmail.com> - 2016-12-05 08:10 +0100
            Re: [PATCH 1/1] mtd: ubi: fix improper return value Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-12-05 09:30 +0100
              Re: [PATCH 1/1] mtd: ubi: fix improper return value Artem Bityutskiy <dedekind1@gmail.com> - 2016-12-05 11:00 +0100
        Re: [PATCH 1/1] mtd: ubi: fix improper return value Marek Vasut <marek.vasut@gmail.com> - 2016-12-04 22:40 +0100

#1535603 — [PATCH 1/1] mtd: ubi: fix improper return value

From: Pan Bian <bianpan2016@163.com>

When __vmalloc() returns a NULL pointer, the region is not checked, and
we cannot make sure that only 0xFF bytes are present at offset. Thus,
returning 0 seems improper.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=189081

Signed-off-by: Pan Bian <bianpan2016@163.com>
---
 drivers/mtd/ubi/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index b6fb8f9..b54fe05 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -1413,7 +1413,7 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int pnum, int offset, int len)
 	buf = __vmalloc(len, GFP_NOFS, PAGE_KERNEL);
 	if (!buf) {
 		ubi_err(ubi, "cannot allocate memory to check for 0xFFs");
-		return 0;
+		return -ENOMEM;
 	}
 
 	err = mtd_read(ubi->mtd, addr, len, &read, buf);
-- 
1.9.1

[toc] | [next] | [standalone]

#1535693

On 12/04/2016 07:12 AM, Pan Bian wrote:
> From: Pan Bian <bianpan2016@163.com>
> 
> When __vmalloc() returns a NULL pointer, the region is not checked, and
> we cannot make sure that only 0xFF bytes are present at offset. Thus,
> returning 0 seems improper.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=189081
> 
> Signed-off-by: Pan Bian <bianpan2016@163.com>
> ---
>  drivers/mtd/ubi/io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> index b6fb8f9..b54fe05 100644
> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -1413,7 +1413,7 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int pnum, int offset, int len)
>  	buf = __vmalloc(len, GFP_NOFS, PAGE_KERNEL);
>  	if (!buf) {
>  		ubi_err(ubi, "cannot allocate memory to check for 0xFFs");
> -		return 0;
> +		return -ENOMEM;

I wonder if you shouldn't also nuke the ubi_err() , because when you run
out of memory, printk() will likely also fail.

>  	}
>  
>  	err = mtd_read(ubi->mtd, addr, len, &read, buf);
> 


-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1535708

On Sun, 2016-12-04 at 13:48 +0100, Marek Vasut wrote:
> On 12/04/2016 07:12 AM, Pan Bian wrote:
> > From: Pan Bian <bianpan2016@163.com>
> > 
> > When __vmalloc() returns a NULL pointer, the region is not checked, and
> > we cannot make sure that only 0xFF bytes are present at offset. Thus,
> > returning 0 seems improper.
> > 
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=189081
> > 
> > Signed-off-by: Pan Bian <bianpan2016@163.com>
> > ---
> >  drivers/mtd/ubi/io.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
[]
> > @@ -1413,7 +1413,7 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int pnum, int offset, int len)
> >  	buf = __vmalloc(len, GFP_NOFS, PAGE_KERNEL);
> >  	if (!buf) {
> >  		ubi_err(ubi, "cannot allocate memory to check for 0xFFs");
> > -		return 0;
> > +		return -ENOMEM;
> 
> I wonder if you shouldn't also nuke the ubi_err() , because when you run
> out of memory, printk() will likely also fail.

No, not really.  printk doesn't allocate memory.

But the ubi_err should be removed because all memory
allocations that fail without a specific GFP_NOWARN
flag already have a dump_stack() call.

[toc] | [prev] | [next] | [standalone]

#1535717

On 04.12.2016 21:33, Joe Perches wrote:
>>> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> []
>>> @@ -1413,7 +1413,7 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int pnum, int offset, int len)
>>>  	buf = __vmalloc(len, GFP_NOFS, PAGE_KERNEL);
>>>  	if (!buf) {
>>>  		ubi_err(ubi, "cannot allocate memory to check for 0xFFs");
>>> -		return 0;
>>> +		return -ENOMEM;
>>
>> I wonder if you shouldn't also nuke the ubi_err() , because when you run
>> out of memory, printk() will likely also fail.
> 
> No, not really.  printk doesn't allocate memory.
> 
> But the ubi_err should be removed because all memory
> allocations that fail without a specific GFP_NOWARN
> flag already have a dump_stack() call.

We should better think about how to get ubi_self_check_all_ff() fixed.
When enabled on a modern NAND, vmalloc() is likely to fail now and then
since len is the erase block size and can be up to a few mega bytes.

Thanks,
//richard

[toc] | [prev] | [next] | [standalone]

#1535854

On Sun, 2016-12-04 at 21:52 +0100, Richard Weinberger wrote:
> We should better think about how to get ubi_self_check_all_ff()
> fixed.
> When enabled on a modern NAND, vmalloc() is likely to fail now and
> then
> since len is the erase block size and can be up to a few mega bytes.

I did an attempt to switch from virtually continuous buffers to an
array of page pointers, but never finished.

[toc] | [prev] | [next] | [standalone]

#1535888

On Mon, 05 Dec 2016 09:09:34 +0200
Artem Bityutskiy <dedekind1@gmail.com> wrote:

> On Sun, 2016-12-04 at 21:52 +0100, Richard Weinberger wrote:
> > We should better think about how to get ubi_self_check_all_ff()
> > fixed.
> > When enabled on a modern NAND, vmalloc() is likely to fail now and
> > then
> > since len is the erase block size and can be up to a few mega bytes.  
> 
> I did an attempt to switch from virtually continuous buffers to an
> array of page pointers, but never finished.

I started to implement that too but unfortunately never had the time to
finish it :-(.
Don't know why you were trying to move to kzalloc-ed buffer, but my
goal was to avoid the extra copy when the controller transfers data
using DMA, and the recent posts regarding vmalloc-ed buffers and DMA
might solve the issue.

This being said, UBI and UBIFS tend to allocate big portions of
memory (usually a full eraseblock), and sometime this is
overkill.
For example, I'm not sure we need to allocate that much memory to do
things like 'check if this portion is all filled with 0xff'. Allocating
a ->max_write_size buffer and iterating over write-units should be
almost as efficient and still consume less memory. But this has nothing
to do with the vmalloc vs kmalloc debate ;-).

[toc] | [prev] | [next] | [standalone]

#1535947

On Mon, 2016-12-05 at 09:23 +0100, Boris Brezillon wrote:
> I started to implement that too but unfortunately never had the time
> to
> finish it :-(.
> Don't know why you were trying to move to kzalloc-ed buffer, but my
> goal was to avoid the extra copy when the controller transfers data
> using DMA, and the recent posts regarding vmalloc-ed buffers and DMA
> might solve the issue.

Yes, I wanted to do that globally for UBI/UBIFS to get rid of vmalloc.

> This being said, UBI and UBIFS tend to allocate big portions of
> memory (usually a full eraseblock), and sometime this is
> overkill.

Those checks were just hacky debugging functions at the beginning, then
they got cleaned up without a re-write.

> For example, I'm not sure we need to allocate that much memory to do
> things like 'check if this portion is all filled with 0xff'. 

Because memcmp() is was very easy to use. Back then the focus was
getting other things work well, and efforts were saved on less
important parts. And 128KiB was not terribly bad. Today, these less
important things are important.

> Allocating
> a ->max_write_size buffer and iterating over write-units should be
> almost as efficient and still consume less memory. But this has
> nothing
> to do with the vmalloc vs kmalloc debate ;-).

Well, this is related. Someone may start small and take care of these
first :-)

Artem.

[toc] | [prev] | [next] | [standalone]

#1535721

On 12/04/2016 09:33 PM, Joe Perches wrote:
> On Sun, 2016-12-04 at 13:48 +0100, Marek Vasut wrote:
>> On 12/04/2016 07:12 AM, Pan Bian wrote:
>>> From: Pan Bian <bianpan2016@163.com>
>>>
>>> When __vmalloc() returns a NULL pointer, the region is not checked, and
>>> we cannot make sure that only 0xFF bytes are present at offset. Thus,
>>> returning 0 seems improper.
>>>
>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=189081
>>>
>>> Signed-off-by: Pan Bian <bianpan2016@163.com>
>>> ---
>>>  drivers/mtd/ubi/io.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> []
>>> @@ -1413,7 +1413,7 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int pnum, int offset, int len)
>>>  	buf = __vmalloc(len, GFP_NOFS, PAGE_KERNEL);
>>>  	if (!buf) {
>>>  		ubi_err(ubi, "cannot allocate memory to check for 0xFFs");
>>> -		return 0;
>>> +		return -ENOMEM;
>>
>> I wonder if you shouldn't also nuke the ubi_err() , because when you run
>> out of memory, printk() will likely also fail.
> 
> No, not really.  printk doesn't allocate memory.
> 
> But the ubi_err should be removed because all memory
> allocations that fail without a specific GFP_NOWARN
> flag already have a dump_stack() call.

Ah, thanks for the correction :-)

-- 
Best regards,
Marek Vasut

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web