Re: mmotm 2008-12-17-16-41 uploaded

Re: mmotm 2008-12-17-16-41 uploaded

[KAMEZAWA Hiroyuki]

On Wed, 17 Dec 2008 16:41:54 -0800
akpm@linux-foundation.org wrote:

> The mm-of-the-moment snapshot 2008-12-17-16-41 has been uploaded to
> 
>    http://userweb.kernel.org/~akpm/mmotm/
> 
> and will soon be available at
> 
>    git://git.zen-sources.org/zen/mmotm.git
> 

This was necessary. I'm not sure my patch description is corrcect...
but this helps me ;)
This change kfree(dev->p) comes from linux-next.

==
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

dev->release() may free struct dev itself. To free dev->p
in safe way, remember dev->p.

Signed-off-by:KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 drivers/base/core.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Index: mmotm-2.6.28-Dec17/drivers/base/core.c
===================================================================
--- mmotm-2.6.28-Dec17.orig/drivers/base/core.c
+++ mmotm-2.6.28-Dec17/drivers/base/core.c
@@ -109,6 +109,7 @@ static struct sysfs_ops dev_sysfs_ops = 
 static void device_release(struct kobject *kobj)
 {
 	struct device *dev = to_dev(kobj);
+	struct device_private *p = dev->p;
 
 	if (dev->release)
 		dev->release(dev);
@@ -116,11 +117,13 @@ static void device_release(struct kobjec
 		dev->type->release(dev);
 	else if (dev->class && dev->class->dev_release)
 		dev->class->dev_release(dev);
-	else
+	else {
 		WARN(1, KERN_ERR "Device '%s' does not have a release() "
 			"function, it is broken and must be fixed.\n",
 			dev_name(dev));
-	kfree(dev->p);
+		return;
+	}
+	kfree(p);
 }
 
 static struct kobj_type device_ktype = {

Re: mmotm 2008-12-17-16-41 uploaded

[Greg KH]

On Thu, Dec 18, 2008 at 01:10:15PM +0900, KAMEZAWA Hiroyuki wrote:
> On Wed, 17 Dec 2008 16:41:54 -0800
> akpm@linux-foundation.org wrote:
> 
> > The mm-of-the-moment snapshot 2008-12-17-16-41 has been uploaded to
> > 
> >    http://userweb.kernel.org/~akpm/mmotm/
> > 
> > and will soon be available at
> > 
> >    git://git.zen-sources.org/zen/mmotm.git
> > 
> 
> This was necessary. I'm not sure my patch description is corrcect...
> but this helps me ;)
> This change kfree(dev->p) comes from linux-next.
> 
> ==
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> dev->release() may free struct dev itself. To free dev->p
> in safe way, remember dev->p.
> 
> Signed-off-by:KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  drivers/base/core.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> Index: mmotm-2.6.28-Dec17/drivers/base/core.c
> ===================================================================
> --- mmotm-2.6.28-Dec17.orig/drivers/base/core.c
> +++ mmotm-2.6.28-Dec17/drivers/base/core.c
> @@ -109,6 +109,7 @@ static struct sysfs_ops dev_sysfs_ops = 
>  static void device_release(struct kobject *kobj)
>  {
>  	struct device *dev = to_dev(kobj);
> +	struct device_private *p = dev->p;
>  
>  	if (dev->release)
>  		dev->release(dev);
> @@ -116,11 +117,13 @@ static void device_release(struct kobjec
>  		dev->type->release(dev);
>  	else if (dev->class && dev->class->dev_release)
>  		dev->class->dev_release(dev);
> -	else
> +	else {
>  		WARN(1, KERN_ERR "Device '%s' does not have a release() "
>  			"function, it is broken and must be fixed.\n",
>  			dev_name(dev));
> -	kfree(dev->p);
> +		return;
> +	}
> +	kfree(p);

You have a memory leak for when there is no release function for a
device, don't return if that happens.

I've now fixed this up in my tree, thanks for pointing it out.

greg k-h

Re: mmotm 2008-12-17-16-41 uploaded

[KAMEZAWA Hiroyuki]

On Thu, 18 Dec 2008 14:23:29 -0800
Greg KH <greg@kroah.com> wrote:

> On Thu, Dec 18, 2008 at 01:10:15PM +0900, KAMEZAWA Hiroyuki wrote:
> > On Wed, 17 Dec 2008 16:41:54 -0800
> > akpm@linux-foundation.org wrote:
> > 
> > > The mm-of-the-moment snapshot 2008-12-17-16-41 has been uploaded to
> > > 
> > >    http://userweb.kernel.org/~akpm/mmotm/
> > > 
> > > and will soon be available at
> > > 
> > >    git://git.zen-sources.org/zen/mmotm.git
> > > 
> > 
> > This was necessary. I'm not sure my patch description is corrcect...
> > but this helps me ;)
> > This change kfree(dev->p) comes from linux-next.
> > 
> > ==
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > 
> > dev->release() may free struct dev itself. To free dev->p
> > in safe way, remember dev->p.
> > 
> > Signed-off-by:KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > ---
> >  drivers/base/core.c |    7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > Index: mmotm-2.6.28-Dec17/drivers/base/core.c
> > ===================================================================
> > --- mmotm-2.6.28-Dec17.orig/drivers/base/core.c
> > +++ mmotm-2.6.28-Dec17/drivers/base/core.c
> > @@ -109,6 +109,7 @@ static struct sysfs_ops dev_sysfs_ops = 
> >  static void device_release(struct kobject *kobj)
> >  {
> >  	struct device *dev = to_dev(kobj);
> > +	struct device_private *p = dev->p;
> >  
> >  	if (dev->release)
> >  		dev->release(dev);
> > @@ -116,11 +117,13 @@ static void device_release(struct kobjec
> >  		dev->type->release(dev);
> >  	else if (dev->class && dev->class->dev_release)
> >  		dev->class->dev_release(dev);
> > -	else
> > +	else {
> >  		WARN(1, KERN_ERR "Device '%s' does not have a release() "
> >  			"function, it is broken and must be fixed.\n",
> >  			dev_name(dev));
> > -	kfree(dev->p);
> > +		return;
> > +	}
> > +	kfree(p);
> 
> You have a memory leak for when there is no release function for a
> device, don't return if that happens.
> 
I thought "not released" dev may touch dev->p later..

> I've now fixed this up in my tree, thanks for pointing it out.
> 
ok, thanks.

-Kame

> greg k-h
> 

Re: mmotm 2008-12-17-16-41 uploaded

[Greg KH]

On Fri, Dec 19, 2008 at 09:11:14AM +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 18 Dec 2008 14:23:29 -0800
> Greg KH <greg@kroah.com> wrote:
> 
> > On Thu, Dec 18, 2008 at 01:10:15PM +0900, KAMEZAWA Hiroyuki wrote:
> > > On Wed, 17 Dec 2008 16:41:54 -0800
> > > akpm@linux-foundation.org wrote:
> > > 
> > > > The mm-of-the-moment snapshot 2008-12-17-16-41 has been uploaded to
> > > > 
> > > >    http://userweb.kernel.org/~akpm/mmotm/
> > > > 
> > > > and will soon be available at
> > > > 
> > > >    git://git.zen-sources.org/zen/mmotm.git
> > > > 
> > > 
> > > This was necessary. I'm not sure my patch description is corrcect...
> > > but this helps me ;)
> > > This change kfree(dev->p) comes from linux-next.
> > > 
> > > ==
> > > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > > 
> > > dev->release() may free struct dev itself. To free dev->p
> > > in safe way, remember dev->p.
> > > 
> > > Signed-off-by:KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > > ---
> > >  drivers/base/core.c |    7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > Index: mmotm-2.6.28-Dec17/drivers/base/core.c
> > > ===================================================================
> > > --- mmotm-2.6.28-Dec17.orig/drivers/base/core.c
> > > +++ mmotm-2.6.28-Dec17/drivers/base/core.c
> > > @@ -109,6 +109,7 @@ static struct sysfs_ops dev_sysfs_ops = 
> > >  static void device_release(struct kobject *kobj)
> > >  {
> > >  	struct device *dev = to_dev(kobj);
> > > +	struct device_private *p = dev->p;
> > >  
> > >  	if (dev->release)
> > >  		dev->release(dev);
> > > @@ -116,11 +117,13 @@ static void device_release(struct kobjec
> > >  		dev->type->release(dev);
> > >  	else if (dev->class && dev->class->dev_release)
> > >  		dev->class->dev_release(dev);
> > > -	else
> > > +	else {
> > >  		WARN(1, KERN_ERR "Device '%s' does not have a release() "
> > >  			"function, it is broken and must be fixed.\n",
> > >  			dev_name(dev));
> > > -	kfree(dev->p);
> > > +		return;
> > > +	}
> > > +	kfree(p);
> > 
> > You have a memory leak for when there is no release function for a
> > device, don't return if that happens.
> > 
> I thought "not released" dev may touch dev->p later..

Nope, it's a bug in the caller code.  They better not be touching dev->p
later, or bad things would be happening to their code :)

thanks,

greg k-h