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

[Dave Young]

On Wed, Jan 09, 2008 at 06:16:02PM +0900, Tejun Heo wrote:
> Hello,
> 
> My laptop and cell finally decided to talk to each other and I could
> reproduce the bug here.  The attached patch should remove the oops.
> 
> The bug is two folded.  I just skimmed through the bluetooth code and am
> very likely to wrong at places.  Please correct me where I'm wrong.
> 
> 1. It's introduced by class device migration and the bug will go away
> with CONFIG_SYSFS_DEPRECATED turned on.  With CONFIG_SYSFS_DEPRECATED
> turned off, what used to be class devices live under actual devices.
> For rfcomm, this means the rfcommN tty device now lives under the
> aclXXXX node (probably represents an active connection?) instead of the
> class directory.
> 
> It seems rfcommN devices are supposed to survive over disconnects so the
> rfcommN device moves under the live connection while connection is alive
> and retreats back to a default directory when the connection is lost.
> This is all fine and dandy as long as the rfcommN device lives under
> class directory as class directory never goes away.
> 
> However, with recent sysfs updates, rfcommN now lives directly under the
> aclXXXX node.  If the connection goes away while rfcommN device is under
> it, it gets deleted but rfcommN is still treated as alive.
> 
> This isn't supported.  sysfs doesn't allow parents to die first and the
> dangling children to be salvaged using sysfs_move().
> 
> 2. Which in turn exposes three bugs in sysfs
> 	- sysfs_lookup() returning NULL on negative lookup.  sysfs
> 	  code requires that all negative dentries are shot down.
> 	  lookup should return -ENOENT instead of NULL.
> 	- in move and rename, error handling is wrong.  It ends up
> 	  passing ERR_PTR() values to dput().
> 	- there was an extra dput() in sysfs_move_dir().
> 
> The attached patch fixes all sysfs bugs and removes the oops.  However,
> rfcommX moving is still broken.  The rfcommX device won't be visible
> from sysfs tree after the initial move failure and all following moves
> will fail.
> 
> Please confirm the attached patch fixes the oops.  I'll separate it into
> two patches and forward them to Greg.  But bluetooth code also needs to
> be updated such that it moves the refcommX device before killing the
> connection node.
For bluetooth device_move, the only child device of hci_conn dev is the rfcomm tty_dev. How about the following patch, please verify :

---
net/bluetooth/hci_sysfs.c |    7 +++++++
1 file changed, 7 insertions(+)

diff -upr linux/net/bluetooth/hci_sysfs.c linux.new/net/bluetooth/hci_sysfs.c
--- linux/net/bluetooth/hci_sysfs.c	2008-01-10 09:01:51.000000000 +0800
+++ linux.new/net/bluetooth/hci_sysfs.c	2008-01-10 09:01:22.000000000 +0800
@@ -316,9 +316,16 @@ void hci_conn_add_sysfs(struct hci_conn 
 	schedule_work(&conn->work);
 }
 
+static int hci_conn_move_child(struct device *dev, void *data)
+{
+	device_move(dev, NULL);
+	return 0;
+}
+
 static void del_conn(struct work_struct *work)
 {
 	struct hci_conn *conn = container_of(work, struct hci_conn, work);
+	device_for_each_child(&conn->dev, NULL, hci_conn_move_child);
 	device_del(&conn->dev);
 	put_device(&conn->dev);
 }

> 
> Thanks.
> 
> -- 
> tejun

> diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
> diff --git a/fs/dcache.c b/fs/dcache.c
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index 3371629..f281cc6 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -678,8 +678,10 @@ static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry,
>  	sd = sysfs_find_dirent(parent_sd, dentry->d_name.name);
>  
>  	/* no such entry */
> -	if (!sd)
> +	if (!sd) {
> +		ret = ERR_PTR(-ENOENT);
>  		goto out_unlock;
> +	}
>  
>  	/* attach dentry and inode */
>  	inode = sysfs_get_inode(sd);
> @@ -781,6 +783,7 @@ int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
>  	old_dentry = sysfs_get_dentry(sd);
>  	if (IS_ERR(old_dentry)) {
>  		error = PTR_ERR(old_dentry);
> +		old_dentry = NULL;
>  		goto out;
>  	}
>  
> @@ -848,6 +851,7 @@ int sysfs_move_dir(struct kobject *kobj, struct kobject *new_parent_kobj)
>  	old_dentry = sysfs_get_dentry(sd);
>  	if (IS_ERR(old_dentry)) {
>  		error = PTR_ERR(old_dentry);
> +		old_dentry = NULL;
>  		goto out;
>  	}
>  	old_parent = old_dentry->d_parent;
> @@ -855,6 +859,7 @@ int sysfs_move_dir(struct kobject *kobj, struct kobject *new_parent_kobj)
>  	new_parent = sysfs_get_dentry(new_parent_sd);
>  	if (IS_ERR(new_parent)) {
>  		error = PTR_ERR(new_parent);
> +		new_parent = NULL;
>  		goto out;
>  	}
>  
> @@ -878,7 +883,6 @@ again:
>  	error = 0;
>  	d_add(new_dentry, NULL);
>  	d_move(old_dentry, new_dentry);
> -	dput(new_dentry);
>  
>  	/* Remove from old parent's list and insert into new parent's list. */
>  	sysfs_unlink_sibling(sd);
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h

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