Memory leak on UBI volume truncating

Memory leak on UBI volume truncating

[Marek Skuczynski]

Hello,
  I have prepare a simple volume update stress test (see test code below).
  This test has been run for kernel 2.6.23 with some updates from 2.6.28,
  and always after a few minutes the OOM killer was launched.

  What i found it that each an UBI volume truncate operation with
ubiupdatevol tool
  causes memory leak. I think this happens because:

  - ubi_start_update()  param "bytes" is equal 0

  - vol->updating flag is re-set to 0

  - vol->upd_buf is allocated regardless of vol->updating flag,
     but not released on device close by vol_cdev_release()

  I never run the test on a newer kernel version, so I cannot confirm
that this problem still exists.

  Please confirm, whether my findings are correct or not, thanks.

Regards,
  Marek


int ubi_start_update(struct ubi_device *ubi, struct ubi_volume *vol,
    long long bytes)
   /* ... */
   vol->updating = 1;

   if (bytes == 0) {
        /* ... */
        vol->updating = 0;
   }

   vol->upd_buf = vmalloc(ubi->leb_size);
   /* ... */
}

static int vol_cdev_release(struct inode *inode, struct file *file)
{
  /* ... */
   if (vol->updating) {
      /* ... */
      vol->updating = 0;
      vfree(vol->upd_buf);
   }
   /* ... */
}

[--- TEST BODY ----]
while [ true ]
do
   echo "Truncating #1"
   /tmp/ubiupdatevol /dev/ubi0_3 -t
   echo "Writing #1"
   cat /dev/zero  | /tmp/ubiupdatevol /dev/ubi0_3 -s 4206 -
   echo "Truncating #2"
   /tmp/ubiupdatevol /dev/ubi0_3 -t
   echo "Writing #2"
   cat /dev/urandom  | /tmp/ubiupdatevol /dev/ubi0_3 -s 4206 -
done

Re: Memory leak on UBI volume truncating

[Artem Bityutskiy]

Hi,

On Wed, 2010-01-13 at 15:28 +0100, Marek Skuczynski wrote:
> Hello,
>   I have prepare a simple volume update stress test (see test code below).
>   This test has been run for kernel 2.6.23 with some updates from 2.6.28,
>   and always after a few minutes the OOM killer was launched.
> 
>   What i found it that each an UBI volume truncate operation with
> ubiupdatevol tool
>   causes memory leak. I think this happens because:
> 
>   - ubi_start_update()  param "bytes" is equal 0
> 
>   - vol->updating flag is re-set to 0
> 
>   - vol->upd_buf is allocated regardless of vol->updating flag,
>      but not released on device close by vol_cdev_release()
> 
>   I never run the test on a newer kernel version, so I cannot confirm
> that this problem still exists.
> 
>   Please confirm, whether my findings are correct or not, thanks.

thanks for this finding. Looks like your analysis is right. Does this
simple patch help?

diff --git a/drivers/mtd/ubi/upd.c b/drivers/mtd/ubi/upd.c
index c1d7b88..9e3cd34 100644
--- a/drivers/mtd/ubi/upd.c
+++ b/drivers/mtd/ubi/upd.c
@@ -155,12 +155,12 @@ int ubi_start_update(struct ubi_device *ubi, struct ubi_volume *vol,
                if (err)
                        return err;
                vol->updating = 0;
+       } else {
+               vol->upd_buf = vmalloc(ubi->leb_size);
+               if (!vol->upd_buf)
+                       return -ENOMEM;
        }

-       vol->upd_buf = vmalloc(ubi->leb_size);
-       if (!vol->upd_buf)
-               return -ENOMEM;
-
        vol->upd_ebs = div_u64(bytes + vol->usable_leb_size - 1,
                               vol->usable_leb_size);
        vol->upd_bytes = bytes;

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: Memory leak on UBI volume truncating

[Artem Bityutskiy]

On Sun, 2010-01-17 at 12:37 +0200, Artem Bityutskiy wrote:
> Hi,
> 
> On Wed, 2010-01-13 at 15:28 +0100, Marek Skuczynski wrote:
> > Hello,
> >   I have prepare a simple volume update stress test (see test code below).
> >   This test has been run for kernel 2.6.23 with some updates from 2.6.28,
> >   and always after a few minutes the OOM killer was launched.
> > 
> >   What i found it that each an UBI volume truncate operation with
> > ubiupdatevol tool
> >   causes memory leak. I think this happens because:
> > 
> >   - ubi_start_update()  param "bytes" is equal 0
> > 
> >   - vol->updating flag is re-set to 0
> > 
> >   - vol->upd_buf is allocated regardless of vol->updating flag,
> >      but not released on device close by vol_cdev_release()
> > 
> >   I never run the test on a newer kernel version, so I cannot confirm
> > that this problem still exists.
> > 
> >   Please confirm, whether my findings are correct or not, thanks.
> 
> thanks for this finding. Looks like your analysis is right. Does this
> simple patch help?

Actually this even simpler patch should fix the issue. Could you please
test it and let me know if it helps.

diff --git a/drivers/mtd/ubi/upd.c b/drivers/mtd/ubi/upd.c
index c1d7b88..425bf5a 100644
--- a/drivers/mtd/ubi/upd.c
+++ b/drivers/mtd/ubi/upd.c
@@ -155,6 +155,7 @@ int ubi_start_update(struct ubi_device *ubi, struct ubi_volume *vol,
                if (err)
                        return err;
                vol->updating = 0;
+               return 0;
        }

        vol->upd_buf = vmalloc(ubi->leb_size);

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: Memory leak on UBI volume truncating

[Marek Skuczynski]

Hello Artem,
> On Sun, Jan 17, 2010 at 11:43 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> On Sun, 2010-01-17 at 12:37 +0200, Artem Bityutskiy wrote:
>>> Hi,
>>>
>>> On Wed, 2010-01-13 at 15:28 +0100, Marek Skuczynski wrote:
>>> > Hello,
>>> >   I have prepare a simple volume update stress test (see test code below).
>>> >   This test has been run for kernel 2.6.23 with some updates from 2.6.28,
>>> >   and always after a few minutes the OOM killer was launched.
>>> >
>>> >   What i found it that each an UBI volume truncate operation with
>>> > ubiupdatevol tool
>>> >   causes memory leak. I think this happens because:
>>> >
>>> >   - ubi_start_update()  param "bytes" is equal 0
>>> >
>>> >   - vol->updating flag is re-set to 0
>>> >
>>> >   - vol->upd_buf is allocated regardless of vol->updating flag,
>>> >      but not released on device close by vol_cdev_release()
>>> >
>>> >   I never run the test on a newer kernel version, so I cannot confirm
>>> > that this problem still exists.
>>> >
>>> >   Please confirm, whether my findings are correct or not, thanks.
>>>
>>> thanks for this finding. Looks like your analysis is right. Does this
>>> simple patch help?
>>
>> Actually this even simpler patch should fix the issue. Could you please
>> test it and let me know if it helps.
>>
>> diff --git a/drivers/mtd/ubi/upd.c b/drivers/mtd/ubi/upd.c
>> index c1d7b88..425bf5a 100644
>> --- a/drivers/mtd/ubi/upd.c
>> +++ b/drivers/mtd/ubi/upd.c
>> @@ -155,6 +155,7 @@ int ubi_start_update(struct ubi_device *ubi, struct ubi_volume *vol,
>>                if (err)
>>                        return err;
>>                vol->updating = 0;
>> +               return 0;
>>        }
>>
>>        vol->upd_buf = vmalloc(ubi->leb_size);
>>
>> --

Thanks for you reply. It seems the fix solve the issue.

Regards,
 Marek

Re: Memory leak on UBI volume truncating

[Artem Bityutskiy]

On Mon, 2010-01-18 at 09:36 +0100, Marek Skuczynski wrote:
> Hello Artem,
> > On Sun, Jan 17, 2010 at 11:43 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> >> On Sun, 2010-01-17 at 12:37 +0200, Artem Bityutskiy wrote:
> >>> Hi,
> >>>
> >>> On Wed, 2010-01-13 at 15:28 +0100, Marek Skuczynski wrote:
> >>> > Hello,
> >>> >   I have prepare a simple volume update stress test (see test code below).
> >>> >   This test has been run for kernel 2.6.23 with some updates from 2.6.28,
> >>> >   and always after a few minutes the OOM killer was launched.
> >>> >
> >>> >   What i found it that each an UBI volume truncate operation with
> >>> > ubiupdatevol tool
> >>> >   causes memory leak. I think this happens because:
> >>> >
> >>> >   - ubi_start_update()  param "bytes" is equal 0
> >>> >
> >>> >   - vol->updating flag is re-set to 0
> >>> >
> >>> >   - vol->upd_buf is allocated regardless of vol->updating flag,
> >>> >      but not released on device close by vol_cdev_release()
> >>> >
> >>> >   I never run the test on a newer kernel version, so I cannot confirm
> >>> > that this problem still exists.
> >>> >
> >>> >   Please confirm, whether my findings are correct or not, thanks.
> >>>
> >>> thanks for this finding. Looks like your analysis is right. Does this
> >>> simple patch help?
> >>
> >> Actually this even simpler patch should fix the issue. Could you please
> >> test it and let me know if it helps.
> >>
> >> diff --git a/drivers/mtd/ubi/upd.c b/drivers/mtd/ubi/upd.c
> >> index c1d7b88..425bf5a 100644
> >> --- a/drivers/mtd/ubi/upd.c
> >> +++ b/drivers/mtd/ubi/upd.c
> >> @@ -155,6 +155,7 @@ int ubi_start_update(struct ubi_device *ubi, struct ubi_volume *vol,
> >>                if (err)
> >>                        return err;
> >>                vol->updating = 0;
> >> +               return 0;
> >>        }
> >>
> >>        vol->upd_buf = vmalloc(ubi->leb_size);
> >>
> >> --
> 
> Thanks for you reply. It seems the fix solve the issue.

Cool. I'll push the fix and also send it to -stable a bit later. Thanks
for catching this.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: Memory leak on UBI volume truncating

[Artem Bityutskiy]

On Mon, 2010-01-18 at 09:36 +0100, Marek Skuczynski wrote:
> >> diff --git a/drivers/mtd/ubi/upd.c b/drivers/mtd/ubi/upd.c
> >> index c1d7b88..425bf5a 100644
> >> --- a/drivers/mtd/ubi/upd.c
> >> +++ b/drivers/mtd/ubi/upd.c
> >> @@ -155,6 +155,7 @@ int ubi_start_update(struct ubi_device *ubi, struct ubi_volume *vol,
> >>                if (err)
> >>                        return err;
> >>                vol->updating = 0;
> >> +               return 0;
> >>        }
> >>
> >>        vol->upd_buf = vmalloc(ubi->leb_size);
> >>
> >> --
> 
> Thanks for you reply. It seems the fix solve the issue.

FYI, the fix is merged to the Linus's tree.

Thanks.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)