get_device in device_create_file

get_device in device_create_file

[Pete Zaitcev]

Hi, Greg:

This code makes no sense to me:

> int device_create_file(struct device * dev, struct device_attribute * attr)
> {
> 	int error = 0;
> 	if (get_device(dev)) {
> 		error = sysfs_create_file(&dev->kobj, &attr->attr);
> 		put_device(dev);
> 	}
> 	return error;
> }

If the struct device *dev, and its presumably enclosing structure,
can be freed by a different CPU (or pre-empt), then get_device
does not protect it. It can be freed before get_device is reached.
Buf it not, and the caller has a reference, then the call to
get_device is redundant.

How is this supposed to work?

-- Pete

Re: get_device in device_create_file

[Dmitry Torokhov]

On Tuesday 01 August 2006 16:25, Pete Zaitcev wrote:
> Hi, Greg:
> 
> This code makes no sense to me:
> 
> > int device_create_file(struct device * dev, struct device_attribute * attr)
> > {
> > 	int error = 0;
> > 	if (get_device(dev)) {
> > 		error = sysfs_create_file(&dev->kobj, &attr->attr);
> > 		put_device(dev);
> > 	}
> > 	return error;
> > }
> 
> If the struct device *dev, and its presumably enclosing structure,
> can be freed by a different CPU (or pre-empt), then get_device
> does not protect it. It can be freed before get_device is reached.

*nod*

> Buf it not, and the caller has a reference, then the call to
> get_device is redundant.
>

Yes it is. There are few of redundant gets and puts sprinkled around
in the driver core, but the last time I mentioned that Greg was not
quite ready to get rid of them ;)
 

-- 
Dmitry

Re: get_device in device_create_file

[Pete Zaitcev]

On Tue, 1 Aug 2006 21:13:05 -0400, Dmitry Torokhov <dtor@insightbb.com> wrote:

> > > 	if (get_device(dev)) {
> > > 		error = sysfs_create_file(&dev->kobj, &attr->attr);
> > > 		put_device(dev);
> > > 	}

> > Buf it not, and the caller has a reference, then the call to
> > get_device is redundant.

> Yes it is. There are few of redundant gets and puts sprinkled around
> in the driver core, but the last time I mentioned that Greg was not
> quite ready to get rid of them ;)

I see a small, but nonzero harm from keeping them, for two reasons.
One, I reviewed a patch for RHEL-4 today, where the submitter copied
this code without thinking. So, if we don't flush these, they will
proliferate. Hackers assume that if Greg wrote that, it must be the
right way. Two, these calls shrink the race window and may be
masking something.

-- Pete

Re: get_device in device_create_file

[Greg KH]

On Tue, Aug 01, 2006 at 01:25:09PM -0700, Pete Zaitcev wrote:
> Hi, Greg:
> 
> This code makes no sense to me:
> 
> > int device_create_file(struct device * dev, struct device_attribute * attr)
> > {
> > 	int error = 0;
> > 	if (get_device(dev)) {
> > 		error = sysfs_create_file(&dev->kobj, &attr->attr);
> > 		put_device(dev);
> > 	}
> > 	return error;
> > }
> 
> If the struct device *dev, and its presumably enclosing structure,
> can be freed by a different CPU (or pre-empt), then get_device
> does not protect it. It can be freed before get_device is reached.
> Buf it not, and the caller has a reference, then the call to
> get_device is redundant.

Yes, it is redundant, sorry.  I know there are a few places that we
gratuitously grab references in the core that we don't really need to do
so.

It's interesting that someone would cut-and-paste from a driver core
file into something new.  What kind of code did they do that for?

Anyway, patches to clean this kind of stuff up is gladly accepted.

thanks,

greg k-h