[1/2] mtd/nand : don't free the global data fsl_lbc_ctrl_dev->nand in fsl_elbc_chip_remove()

From: Liu Shuo <b35362@freescale.com>

The global data fsl_lbc_ctrl_dev->nand don't have to be freed in
fsl_elbc_chip_remove(). The right place to do that is in fsl_elbc_nand_remove()
if elbc_fcm_ctrl->counter is zero.

Signed-off-by: Liu Shuo <b35362@freescale.com>
---
 drivers/mtd/nand/fsl_elbc_nand.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

On Tue, 2011-06-28 at 09:50 +0800, b35362@freescale.com wrote:
> From: Liu Shuo <b35362@freescale.com>
> 
> The global data fsl_lbc_ctrl_dev->nand don't have to be freed in
> fsl_elbc_chip_remove(). The right place to do that is in fsl_elbc_nand_remove()
> if elbc_fcm_ctrl->counter is zero.
> 
> Signed-off-by: Liu Shuo <b35362@freescale.com>
> ---
>  drivers/mtd/nand/fsl_elbc_nand.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
> index 0bb254c..a212116 100644
> --- a/drivers/mtd/nand/fsl_elbc_nand.c
> +++ b/drivers/mtd/nand/fsl_elbc_nand.c
> @@ -829,7 +829,6 @@ static int fsl_elbc_chip_remove(struct fsl_elbc_mtd *priv)
>  
>  	elbc_fcm_ctrl->chips[priv->bank] = NULL;
>  	kfree(priv);
> -	kfree(elbc_fcm_ctrl);
>  	return 0;
>  }

Do we have to assign fsl_lbc_ctrl_dev->nand to NULL in
fsl_elbc_nand_remove() then? I think that assignment can be killed then.

        if (!elbc_fcm_ctrl->counter) {
                fsl_lbc_ctrl_dev->nand = NULL;
                kfree(elbc_fcm_ctrl);
        }

On Wed, 29 Jun 2011 09:20:25 +0300
Artem Bityutskiy <dedekind1@gmail.com> wrote:

> On Tue, 2011-06-28 at 09:50 +0800, b35362@freescale.com wrote:
> > From: Liu Shuo <b35362@freescale.com>
> > 
> > The global data fsl_lbc_ctrl_dev->nand don't have to be freed in
> > fsl_elbc_chip_remove(). The right place to do that is in fsl_elbc_nand_remove()
> > if elbc_fcm_ctrl->counter is zero.
> > 
> > Signed-off-by: Liu Shuo <b35362@freescale.com>
> > ---
> >  drivers/mtd/nand/fsl_elbc_nand.c |    1 -
> >  1 files changed, 0 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
> > index 0bb254c..a212116 100644
> > --- a/drivers/mtd/nand/fsl_elbc_nand.c
> > +++ b/drivers/mtd/nand/fsl_elbc_nand.c
> > @@ -829,7 +829,6 @@ static int fsl_elbc_chip_remove(struct fsl_elbc_mtd *priv)
> >  
> >  	elbc_fcm_ctrl->chips[priv->bank] = NULL;
> >  	kfree(priv);
> > -	kfree(elbc_fcm_ctrl);
> >  	return 0;
> >  }
> 
> Do we have to assign fsl_lbc_ctrl_dev->nand to NULL in
> fsl_elbc_nand_remove() then? I think that assignment can be killed then.
> 
>         if (!elbc_fcm_ctrl->counter) {
>                 fsl_lbc_ctrl_dev->nand = NULL;
>                 kfree(elbc_fcm_ctrl);
>         }
> 

If we're freeing fsl_lbc_ctrl, we'd better get rid of references to it...

-Scott

On Wed, 2011-06-29 at 11:45 -0500, Scott Wood wrote:
> On Wed, 29 Jun 2011 09:20:25 +0300
> Artem Bityutskiy <dedekind1@gmail.com> wrote:
> 
> > On Tue, 2011-06-28 at 09:50 +0800, b35362@freescale.com wrote:
> > > From: Liu Shuo <b35362@freescale.com>
> > > 
> > > The global data fsl_lbc_ctrl_dev->nand don't have to be freed in
> > > fsl_elbc_chip_remove(). The right place to do that is in fsl_elbc_nand_remove()
> > > if elbc_fcm_ctrl->counter is zero.
> > > 
> > > Signed-off-by: Liu Shuo <b35362@freescale.com>
> > > ---
> > >  drivers/mtd/nand/fsl_elbc_nand.c |    1 -
> > >  1 files changed, 0 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
> > > index 0bb254c..a212116 100644
> > > --- a/drivers/mtd/nand/fsl_elbc_nand.c
> > > +++ b/drivers/mtd/nand/fsl_elbc_nand.c
> > > @@ -829,7 +829,6 @@ static int fsl_elbc_chip_remove(struct fsl_elbc_mtd *priv)
> > >  
> > >  	elbc_fcm_ctrl->chips[priv->bank] = NULL;
> > >  	kfree(priv);
> > > -	kfree(elbc_fcm_ctrl);
> > >  	return 0;
> > >  }
> > 
> > Do we have to assign fsl_lbc_ctrl_dev->nand to NULL in
> > fsl_elbc_nand_remove() then? I think that assignment can be killed then.
> > 
> >         if (!elbc_fcm_ctrl->counter) {
> >                 fsl_lbc_ctrl_dev->nand = NULL;
> >                 kfree(elbc_fcm_ctrl);
> >         }
> > 
> 
> If we're freeing fsl_lbc_ctrl, we'd better get rid of references to it...

Yes, on the one hand this is a good defensive programming practice, on
the other hand it hides double-free bugs. Like this patch fixes a
double-free bug, and why it was noticed before? I thought may be because
of this NULL assignment?

I do not insist though, that was just a suggestion/question.

On Thu, 30 Jun 2011 14:53:13 +0300
Artem Bityutskiy <dedekind1@gmail.com> wrote:

> On Wed, 2011-06-29 at 11:45 -0500, Scott Wood wrote:
> > If we're freeing fsl_lbc_ctrl, we'd better get rid of references to it...
> 
> Yes, on the one hand this is a good defensive programming practice, on
> the other hand it hides double-free bugs. Like this patch fixes a
> double-free bug, and why it was noticed before? I thought may be because
> of this NULL assignment?

I'm not sure how the NULL assignment was hiding anything here.  It was
probably hidden only because nobody tested it with suitable debug options
enabled since the code was last reorganized.

If the NULL assignment is dropped, consider what happens if the
fsl_elbc_nand module is removed then reinserted.  On reinsertion, it will
see a non-NULL fsl_lbc_ctrl_dev->nand, and will skip allocating a new one.
Then you're referencing freed memory.

Looking more closely, the MAX_BANKS loop should be removed.  Since the
reorganization, the platform device represents one chip, not the
controller, so we should only be removing that one chip.

-Scott

On Thu, 2011-06-30 at 11:26 -0500, Scott Wood wrote:
> If the NULL assignment is dropped, consider what happens if the
> fsl_elbc_nand module is removed then reinserted.  On reinsertion, it
> will
> see a non-NULL fsl_lbc_ctrl_dev->nand, and will skip allocating a new
> one.
> Then you're referencing freed memory.

Oh, then this sounds like a separate bug. Removing the module should
kill everything, and re-inserging the module should have zero
dependencies on the previous states...

Anyway, if you think the original patch is OK, I can put it to my tree.

On Fri, 1 Jul 2011 08:40:21 +0300
Artem Bityutskiy <dedekind1@gmail.com> wrote:

> On Thu, 2011-06-30 at 11:26 -0500, Scott Wood wrote:
> > If the NULL assignment is dropped, consider what happens if the
> > fsl_elbc_nand module is removed then reinserted.  On reinsertion, it
> > will
> > see a non-NULL fsl_lbc_ctrl_dev->nand, and will skip allocating a new
> > one.
> > Then you're referencing freed memory.
> 
> Oh, then this sounds like a separate bug. Removing the module should
> kill everything, and re-inserging the module should have zero
> dependencies on the previous states...

fsl_lbc_ctrl_dev (and thus the fsl_lbc_ctrl_dev->nand pointer) is not part
of the module, it is part of arch/powerpc/sysdev/fsl_lbc.c.  NAND isn't the
only thing that elbc does.  Since there can be multiple NAND chips, which
are separately probed, the first chip (under a lock) creates the NAND state
that is shared among the chips, and the last one removed destroys it.

> Anyway, if you think the original patch is OK, I can put it to my tree.

I think it's OK.  The loop also needs to be removed, so the remove callback
operates only on the particular chip it's called on, but that's a separate
bug.

-Scott

On Tue, 2011-06-28 at 09:50 +0800, b35362@freescale.com wrote:
> From: Liu Shuo <b35362@freescale.com>
> 
> The global data fsl_lbc_ctrl_dev->nand don't have to be freed in
> fsl_elbc_chip_remove(). The right place to do that is in fsl_elbc_nand_remove()
> if elbc_fcm_ctrl->counter is zero.
> 
> Signed-off-by: Liu Shuo <b35362@freescale.com>

Changed the subject to something shorted and pushed to l2-mtd-2.6.git,
thanks.