Re: [PATCH 2/2] pstore: correct the max_dump_cnt clearing of ramoops

[Liu ShuoX]

On Tue 11.Mar'14 at 13:37:23 -0700, Kees Cook wrote:
> On Mon, Mar 10, 2014 at 11:17 PM, Liu ShuoX <shuox.liu@xxxxxxxxx> wrote:
> > From: Liu ShuoX <shuox.liu@xxxxxxxxx>
> >
> > In case that ramoops_init_przs failed, max_dump_cnt won't be reset to
> > zero in error handle path.
> >
> > Signed-off-by: Liu ShuoX <shuox.liu@xxxxxxxxx>
> > ---
> >  fs/pstore/ram.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index 6f96d8c..522e530 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -326,6 +326,7 @@ static void ramoops_free_przs(struct ramoops_context
> > *cxt)
> >         for (i = 0; !IS_ERR_OR_NULL(cxt->przs[i]); i++)
> >                 persistent_ram_free(cxt->przs[i]);
> >         kfree(cxt->przs);
> > +       cxt->max_dump_cnt = 0;
> >  }
> >   static int ramoops_init_przs(struct device *dev, struct ramoops_context
> > *cxt,
> > @@ -350,7 +351,7 @@ static int ramoops_init_przs(struct device *dev, struct
> > ramoops_context *cxt,
> >                              GFP_KERNEL);
> >         if (!cxt->przs) {
> >                 dev_err(dev, "failed to initialize a prz array for
> > dumps\n");
> > -               return -ENOMEM;
> > +               goto fail_prz;
>
> This will have no effect. If cxt->przs == NULL, ramoops_free_przs will
> immediately exit too, not hitting your max_dump_cnt = 0 change.
> Perhaps move the =0 in that function to the top before the check and
> return?
Yes, you are right. Below has the latest patch which move the =0 to the
top of that function, just as you mentioned. Thanks.
> >         }
> >         for (i = 0; i < cxt->max_dump_cnt; i++) {
> > @@ -508,7 +509,6 @@ fail_buf:
> >         kfree(cxt->pstore.buf);
> >  fail_clear:
> >         cxt->pstore.bufsize = 0;
> > -       cxt->max_dump_cnt = 0;
> >  fail_cnt:
> >         kfree(cxt->fprz);
> >  fail_init_fprz:
> > --
> > 1.8.3.2
>
> Otherwise,  yes, once fixed, this clean-up looks good -- it keeps the
> variable initialization and cleanup all in ramoops_init_przs() which
> is how it should be.
>
> Thanks!
>
> -Kees
-----

From: Liu ShuoX <shuox.liu@xxxxxxxxx>

In case that ramoops_init_przs failed, max_dump_cnt won't be reset to
zero in error handle path.

Signed-off-by: Liu ShuoX <shuox.liu@xxxxxxxxx>
---
 fs/pstore/ram.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 6f96d8c..3b57443 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -320,6 +320,7 @@ static void ramoops_free_przs(struct ramoops_context *cxt)
 {
 	int i;
 
+	cxt->max_dump_cnt = 0;
 	if (!cxt->przs)
 		return;
 
@@ -350,7 +351,7 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
 			     GFP_KERNEL);
 	if (!cxt->przs) {
 		dev_err(dev, "failed to initialize a prz array for dumps\n");
-		return -ENOMEM;
+		goto fail_prz;
 	}
 
 	for (i = 0; i < cxt->max_dump_cnt; i++) {
@@ -508,7 +509,6 @@ fail_buf:
 	kfree(cxt->pstore.buf);
 fail_clear:
 	cxt->pstore.bufsize = 0;
-	cxt->max_dump_cnt = 0;
 fail_cnt:
 	kfree(cxt->fprz);
 fail_init_fprz:
--
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/