On Thu, May 15, 2008 at 09:27:50PM +0200, Miklos Szeredi wrote:
quoted text
> > On Thu, 15 May 2008, Miklos Szeredi wrote:
> > > 
> > > This is not meant as a final solution, I'm sure Greg or Kay can help
> > > find a better solution.
> > 
> > Yeah, don't do this:
> > 
> > > +static struct backing_dev_info *dev_get_bdi(struct device *dev)
> > > +{
> > > +	mutex_lock(&bdi_dev_mutex);
> > > +	mutex_unlock(&bdi_dev_mutex);
> > > +
> > > +	return dev_get_drvdata(dev);
> > > +}
> > 
> > This kind of serialization can often hide bugs, and in some cases even 
> > make them go away (if the caller of the function means that the device is 
> > pinned and the tear-down cannot happen, for example), but it's really 
> > really bad form.
> 
> Yeah, I know.
> 
> > In order to use locking in a repeatable manner that is easy to think 
> > about, you really need to *keep* the lock until you've stopped using the 
> > data (or have dereferenced it into a stable form - eg maybe accessing the 
> > pointer itself needs locking, but some individual data read _off_ the 
> > pointer does not).
> > 
> > So the above kind of "get and release the lock" does obviously serialize 
> > wrt others that hold the lock, but it's still wrong.
> > 
> > >  static ssize_t read_ahead_kb_store(struct device *dev,
> > >  				  struct device_attribute *attr,
> > >  				  const char *buf, size_t count)
> > >  {
> > > -	struct backing_dev_info *bdi = dev_get_drvdata(dev);
> > > +	struct backing_dev_info *bdi = dev_get_bdi(dev);
> > >  	char *end;
> > >  	unsigned long read_ahead_kb;
> > >  	ssize_t ret = -EINVAL;
> > 
> > You should just get the lock in the routines that acually use this thing.
> > 
> > Or, if the "struct backing_dev_info *" pointer itself is stable, and it 
> > really is just the access from "struct device" that needs protection, then 
> > at the very least it should have been
> 
> Actually nothing should need protection.  The only problem AFAICS is
> that the device_create()/dev_set_drvdata() interface is racy: somebody
> can come in after the device has been created but before drvdata has
> been set, and then we are in trouble.

Then that needs to be fixed in the code that registered the device
itself.  The driver core knows nothing about this at all.  Is this
something in the block layer?

quoted text
> I'm quite sure this is not the only place in the kernel where this
> would be an issue, that's why I expect the sysfs guys to have some
> sort of alternative solution, that doesn't necessarily involve adding
> a new mutex.

It should be fixed in the bus/subsystem that is creating the device, the
pointer must be set up before device_register() is called (or
device_add()).

thanks,

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