On Wed, 11 Apr 2012, Colin Cross wrote:
> On Wed, Apr 11, 2012 at 1:10 PM, Jesper Juhl <jj@xxxxxxxxxxxxx> wrote:
> > If, in __persistent_ram_init(), the call to
> > persistent_ram_buffer_init() fails or the call to
> > persistent_ram_init_ecc() fails then we fail to free the memory we
> > allocated to 'prz' with kzalloc() - thus leaking it.
> >
> > To prevent the leaks I consolidated all error exits from the function
> > at a 'err:' label at the end and made all error cases jump to that
> > label where we can then make sure we always free 'prz'. This is safe
> > since all the situations where the code bails out happen before 'prz'
> > has been stored anywhere and although we'll do a redundant kfree(NULL)
> > call in the case of kzalloc() itself failing that's OK since kfree()
> > deals gracefully with NULL pointers and I felt it was more important
> > to keep all error exits at a single location than to avoid that one
> > harmless/redundant kfree() on a error path.
> >
> > Signed-off-by: Jesper Juhl <jj@xxxxxxxxxxxxx>
> > ---
> > drivers/staging/android/persistent_ram.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/android/persistent_ram.c b/drivers/staging/android/persistent_ram.c
> > index e08f257..8d8c1e3 100644
> > --- a/drivers/staging/android/persistent_ram.c
> > +++ b/drivers/staging/android/persistent_ram.c
> > @@ -399,12 +399,12 @@ static __init
> > struct persistent_ram_zone *__persistent_ram_init(struct device *dev, bool ecc)
> > {
> > struct persistent_ram_zone *prz;
> > - int ret;
> > + int ret = -ENOMEM;
> >
> > prz = kzalloc(sizeof(struct persistent_ram_zone), GFP_KERNEL);
> > if (!prz) {
> > pr_err("persistent_ram: failed to allocate persistent ram zone\n");
> > - return ERR_PTR(-ENOMEM);
> > + goto err;
> > }
> >
> > INIT_LIST_HEAD(&prz->node);
> > @@ -412,13 +412,13 @@ struct persistent_ram_zone *__persistent_ram_init(struct device *dev, bool ecc)
> > ret = persistent_ram_buffer_init(dev_name(dev), prz);
> This adds virtual mappings, which should get unmapped on error.
>
Ok, thank you for the info.
I currently have no idea how to undo those mappings, but I'll research
that.
Would it be OK if we just merge this patch as-is since it doesn't actually
make this issue any worse than it already is/was and then I'll send a
follow-on patch later when I've worked out how to undo those mappings
(unless someone else beats me to it)?
> > if (ret) {
> > pr_err("persistent_ram: failed to initialize buffer\n");
> > - return ERR_PTR(ret);
> > + goto err;
> > }
> >
> > prz->ecc = ecc;
> > ret = persistent_ram_init_ecc(prz, prz->buffer_size);
> > if (ret)
> > - return ERR_PTR(ret);
> > + goto err;
> >
> > if (prz->buffer->sig == PERSISTENT_RAM_SIG) {
> > if (buffer_size(prz) > prz->buffer_size ||
> > @@ -442,6 +442,9 @@ struct persistent_ram_zone *__persistent_ram_init(struct device *dev, bool ecc)
> > atomic_set(&prz->buffer->size, 0);
> >
> > return prz;
> > +err:
> > + kfree(prz);
> > + return ERR_PTR(ret);
> > }
> >
> > struct persistent_ram_zone * __init
> > --
> > 1.7.10
> >
> >
> > --
> > Jesper Juhl <jj@xxxxxxxxxxxxx> http://www.chaosbits.net/
> > Don't top-post http://www.catb.org/jargon/html/T/top-post.html
> > Plain text mails only, please.
> >
>
> Acked-by: Colin Cross <ccross@xxxxxxxxxxx>
>
Thank you :-)
--
Jesper Juhl <jj@xxxxxxxxxxxxx> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
