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

[Cornelia Huck <cornelia.huck@de.ibm.com>]

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.