Re: [Bluez-devel] Oops involving RFCOMM and sysfs

Cornelia Huck
2008-01-18T09:20:18Z

On Fri, 18 Jan 2008 11:37:21 +0800,
"Dave Young" <hidave.darkstar@gmail.com> wrote:

> Lets see the device_move function, seems there's some problems in it:
>
> 1302 int device_move(struct device *dev, struct device *new_parent)
> 1303 {
> 1304 int error;
> 1305 struct device *old_parent;
> 1306 struct kobject *new_parent_kobj;
> 1307
> 1308 dev = get_device(dev);
> 1309 if (!dev)
> 1310 return -EINVAL;
> 1311
> 1312 new_parent = get_device(new_parent);
> 1313 new_parent_kobj = get_device_parent (dev, new_parent);
>
> Here could get kobject reference

Eww. get_device_parent() may inflate the refcount in one case
for !CONFIG_SYSFS_DEPRECATED, but often won't. (And the function is
named confusingly, since it hints that we always get a reference, which
we don't.)

>
> 1314 if (IS_ERR(new_parent_kobj)) {
> 1315 error = PTR_ERR(new_parent_kobj);
> 1316 put_device(new_parent);
> 1317 goto out;
> 1318 }
> 1319 pr_debug("DEVICE: moving '%s' to '%s'\n", dev->bus_id,
> 1320 new_parent ? new_parent->bus_id : "<NULL>");
> 1321 error = kobject_move(&dev->kobj, new_parent_kobj);
> 1322 if (error) {
> 1323 put_device(new_parent);
>
> imagine new_parent is NULL, then the new_parent_kobj should be put

No, we would need a put_device_parent() (crappy name) which puts the
reference iff get_device_parent() grabbed it.

>
> 1324 goto out;
> 1325 }
> 1326 old_parent = dev->parent;
> 1327 dev->parent = new_parent;
> 1328 if (old_parent)
> 1329 klist_remove(&dev->knode_parent);
> 1330 if (new_parent)
> 1331 klist_add_tail(&dev->knode_parent,
> &new_parent->klist_children);
> 1332 if (!dev->class)
> 1333 goto out_put;
>
> Why not put new_parent | new_parent_kobj?

Because that is the good case :)

>
> 1334 error = device_move_class_links(dev, old_parent, new_parent);
> 1335 if (error) {
> 1336 /* We ignore errors on cleanup since we're hosed
> anyway... */
> 1337 device_move_class_links(dev, new_parent, old_parent);
> 1338 if (!kobject_move(&dev->kobj, &old_parent->kobj)) {
> 1339 if (new_parent)
> 1340 klist_remove(&dev->knode_parent);
> 1341 if (old_parent)
> 1342 klist_add_tail(&dev->knode_parent,
> 1343
> &old_parent->klist_children);
> 1344 }
> 1345 put_device(new_parent);
>
> Same doubt as above

We'd need put_device_parent() or whatever here as well, I guess.

>
> 1346 goto out;
> 1347 }
> 1348 out_put:
> 1349 put_device(old_parent);
> 1350 out:
> 1351 put_device(dev);
> 1352 return error;
> 1353 }
>
> Hope I'm wrong, but if it's indeed bugs, I will send a patch about this.

There are more problems, I'm afraid :( setup_parent() calls
get_device_parent() as well, so device_add() has the same problems on
error cleanup...

I'll take a look at it if I find some time, but I'm afraid I'll not
be able to do so before next week.