Greg KH <greg@xxxxxxxxx> writes:
> On Fri, Jan 13, 2012 at 07:49:07PM +0100, Wolfgang Frisch wrote:
>> Hi,
>>
>> my USB device is causing a kernel oops upon disconnection.
>>
>> It's a "Inside Out Networks Watchport/H" sensor,
>> vendor: 0x1608, product: 0x305, Linux driver: io_ti.
>> The problem appeared after v3.1.
>>
>> A git bisect from v3.1 to v3.2 yielded this commit:
>> ---------------------------------------------------------------
>> 487505c257021fc06a7d05753cf27b011487f1dc is the first bad commit
>> commit 487505c257021fc06a7d05753cf27b011487f1dc
>> Author: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
>> Date: Wed Oct 12 21:53:38 2011 +0000
>>
>> sysfs: Implement support for tagged files in sysfs.
>> ---------------------------------------------------------------
>>
>> This commit introduces a new function sysfs_attr_ns()
>> It assumes that kobj->sd is a valid directory entry and dereferences it
>> unconditionally. This missing check causes a NULL pointer exception when
>> the io_ti device is disconnected. My attached patch fixes the symptom by
>> adding a check to sysfs_attr_ns().
>>
>> The driver works as expected with this patch. However it still leaves a
>> kernel warning message upon disconnection. Unfortunately my
>> understanding of sysfs and the io_ti driver is not good enough to
>> recognize any further, underlying problems.
>
> Do you have network namespaces enabled?
>
> Eric, any thoughts about this?
At a practical level I goofed and overlooked a case where dir can be
NULL in the remove case. Certainly sysfs_hash_and_remove which is the
guts of sysfs_remove has had a check for dir being NULL for a long time,
and it appears to have been introduced by commit
641e6f30a095f3752ed84fd9d279382f5d3ef4c1.
>From long term maintenance perspective we need to stop allowing this
case. sysfs_remove_dir really needs to have it's files and directories
removed before we remove the directory itself. It is totally insane
to ask people to remove attribute that are already gone.
We have a groups member in struct device that can be used to hold
groups of attributes that the device layer will add and remove
when the a device is added and removed. Perhaps we should use
that.
Looking a little farther we do have a driver ->remove call that
is called before the usb_serial device is removed.
Interesting the sysfs_attributes are created in .port_probe
but are removed in .disconnect instead of in .port_remove.
So the fix for this specific case is to make edge_remove_sysfs_attrs the
the .port_remove method.
It isn't nice to have a NULL pointer deference for a case that we used
to allow (however ill advised). I will cook up a patch that handles
this case in device_remove_file but prints a very nasty warning with a
stack back trace when it does.
Given that device remove methods are called before a device is deleted
I don't see any excuse to continue to support the nonsense of a deleting
an attribute after the device has been deleted.
My proposed io_ti.c fix (untested).
diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index 0aac00a..fe49c33 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -2681,11 +2681,6 @@ static void edge_disconnect(struct usb_serial *serial)
struct edgeport_port *edge_port;
dbg("%s", __func__);
-
- for (i = 0; i < serial->num_ports; ++i) {
- edge_port = usb_get_serial_port_data(serial->port[i]);
- edge_remove_sysfs_attrs(edge_port->port);
- }
}
static void edge_release(struct usb_serial *serial)
@@ -2764,6 +2759,7 @@ static struct usb_serial_driver edgeport_1port_device = {
.disconnect = edge_disconnect,
.release = edge_release,
.port_probe = edge_create_sysfs_attrs,
+ .port_remove = edge_remove_sysfs_attrs,
.ioctl = edge_ioctl,
.set_termios = edge_set_termios,
.tiocmget = edge_tiocmget,
The commit where we appear to have started allowing this nonsense.
> commit 641e6f30a095f3752ed84fd9d279382f5d3ef4c1
> Author: Greg Kroah-Hartman <gregkh@xxxxxxx>
> Date: Thu Mar 16 15:44:26 2006 -0800
>
> [PATCH] sysfs: sysfs_remove_dir() needs to invalidate the dentry
>
> When calling sysfs_remove_dir() don't allow any further sysfs functions
> to work for this kobject anymore. This fixes a nasty USB cdc-acm oops
> on disconnect.
>
> Many thanks to Bob Copeland and Paul Fulghum for taking the time to
> track this down.
>
> Cc: Bob Copeland <email@xxxxxxxxxxxxxxx>
> Cc: Paul Fulghum <paulkf@xxxxxxxxxxxxx>
> Cc: Maneesh Soni <maneesh@xxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxx>
>
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index 49bd219..cfd290d 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -302,6 +302,7 @@ void sysfs_remove_dir(struct kobject * kobj)
> * Drop reference from dget() on entrance.
> */
> dput(dentry);
> + kobj->dentry = NULL;
> }
>
> int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
> diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
> index 689f7bc..6beee6f 100644
> --- a/fs/sysfs/inode.c
> +++ b/fs/sysfs/inode.c
> @@ -227,12 +227,16 @@ void sysfs_drop_dentry(struct sysfs_dirent * sd, struct dentry * parent)
> void sysfs_hash_and_remove(struct dentry * dir, const char * name)
> {
> struct sysfs_dirent * sd;
> - struct sysfs_dirent * parent_sd = dir->d_fsdata;
> + struct sysfs_dirent * parent_sd;
> +
> + if (!dir)
> + return;
>
> if (dir->d_inode == NULL)
> /* no inode means this hasn't been made visible yet */
> return;
>
> + parent_sd = dir->d_fsdata;
> mutex_lock(&dir->d_inode->i_mutex);
> list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
> if (!sd->s_element)
>
>
Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
