[PATCH] zfcp: kfree patch

[PATCH] zfcp: kfree patch

[Heiko Carstens]

Hi James,

this is my next attempt to try to get the patch below merged. Last
time Greg KH objected and it didn't get merged.

This patch addresses the following problem: the zfcp device driver
creates own sysfs objects and therefore has its own release function
within the zfcp device driver itself. Upon module unloading there
exists a problem: the module count reaches zero and the module gets
unloaded. _But_ there might still be references to sysfs objects
created by the zfcp device driver (a device driver can't wait until
there are no more references to its created sysfs objects as this
might lead to a deadlock situation, which was already discussed).
So we finally have no more zfcp module code in the kernel but zfcp
generated sysfs objects. If the reference count of these objects
finally drops to zero zfcp's release function will be called...
Which will lead to unpredictable results since the module was already
unloaded.
One solution to this problem is to simply use kfree as release
function since its always there. This doesn't look nice, but it works
and it's safe. And finally would allow module unloading of the zfcp
device driver again.

That's the only solution to this problem that I know of. If anybody
objects to this patch it would be nice if the one comes up with a
better solution.

Thanks,
Heiko

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>

===== zfcp_aux.c 1.17 vs edited =====
--- 1.17/drivers/s390/scsi/zfcp_aux.c	2004-11-19 08:03:14 +01:00
+++ edited/zfcp_aux.c	2005-01-08 10:50:32 +01:00
@@ -40,6 +40,7 @@
 
 /* written against the module interface */
 static int __init  zfcp_module_init(void);
+static void __exit zfcp_module_exit(void);
 
 /* FCP related */
 static void zfcp_ns_gid_pn_handler(unsigned long);
@@ -77,6 +78,7 @@
 
 /* declare driver module init/cleanup functions */
 module_init(zfcp_module_init);
+module_exit(zfcp_module_exit);
 
 MODULE_AUTHOR("Heiko Carstens <heiko.carstens@de.ibm.com>, "
 	      "Andreas Herrman <aherrman@de.ibm.com>, "
@@ -354,6 +356,15 @@
 	return retval;
 }
 
+static void __exit
+zfcp_module_exit(void)
+{
+	zfcp_ccw_unregister();
+	misc_deregister(&zfcp_cfdc_misc);
+	unregister_ioctl32_conversion(zfcp_ioctl_trans.cmd);
+  	fc_release_transport(zfcp_transport_template);
+}
+
 /*
  * function:    zfcp_cfdc_dev_ioctl
  *
@@ -871,7 +882,7 @@
 	/* setup for sysfs registration */
 	snprintf(unit->sysfs_device.bus_id, BUS_ID_SIZE, "0x%016llx", fcp_lun);
 	unit->sysfs_device.parent = &port->sysfs_device;
-	unit->sysfs_device.release = zfcp_sysfs_unit_release;
+	unit->sysfs_device.release = (void (*)(struct device *))kfree;
 	dev_set_drvdata(&unit->sysfs_device, unit);
 
 	/* mark unit unusable as long as sysfs registration is not complete */
@@ -1098,12 +1109,6 @@
 	adapter->in_els_dbf = NULL;
 }
 
-void
-zfcp_dummy_release(struct device *dev)
-{
-	return;
-}
-
 /*
  * Enqueues an adapter at the end of the adapter list in the driver data.
  * All adapter internal structures are set up.
@@ -1119,6 +1124,7 @@
 {
 	int retval = 0;
 	struct zfcp_adapter *adapter;
+	struct device *gs;
 
 	/*
 	 * Note: It is safe to release the list_lock, as any list changes 
@@ -1195,13 +1201,19 @@
 	if (zfcp_sysfs_adapter_create_files(&ccw_device->dev))
 		goto sysfs_failed;
 
-	adapter->generic_services.parent = &adapter->ccw_device->dev;
-	adapter->generic_services.release = zfcp_dummy_release;
-	snprintf(adapter->generic_services.bus_id, BUS_ID_SIZE,
-		 "generic_services");
+	gs = kmalloc(sizeof(struct device), GFP_KERNEL);
+	if (!gs)
+		goto gs_failed;
+	memset(gs, 0, sizeof(struct device));
+
+	gs->parent = &adapter->ccw_device->dev;
+	gs->release = (void (*)(struct device *))kfree;
+	snprintf(gs->bus_id, BUS_ID_SIZE, "generic_services");
+
+	if (device_register(gs))
+		goto gs_free;
 
-	if (device_register(&adapter->generic_services))
-		goto generic_services_failed;
+	adapter->generic_services = gs;
 
 	/* put allocated adapter at list tail */
 	write_lock_irq(&zfcp_data.config_lock);
@@ -1213,7 +1225,9 @@
 
 	goto out;
 
- generic_services_failed:
+ gs_free:
+	kfree(gs);
+ gs_failed:
 	zfcp_sysfs_adapter_remove_files(&adapter->ccw_device->dev);
  sysfs_failed:
 	dev_set_drvdata(&ccw_device->dev, NULL);
@@ -1245,7 +1259,7 @@
 	int retval = 0;
 	unsigned long flags;
 
-	device_unregister(&adapter->generic_services);
+	device_unregister(adapter->generic_services);
 	zfcp_sysfs_adapter_remove_files(&adapter->ccw_device->dev);
 	dev_set_drvdata(&adapter->ccw_device->dev, NULL);
 	/* sanity check: no pending FSF requests */
@@ -1370,13 +1384,13 @@
 			return NULL;
 		}
 		port->d_id = d_id;
-		port->sysfs_device.parent = &adapter->generic_services;
+		port->sysfs_device.parent = adapter->generic_services;
 	} else {
 		snprintf(port->sysfs_device.bus_id,
 			 BUS_ID_SIZE, "0x%016llx", wwpn);
-	port->sysfs_device.parent = &adapter->ccw_device->dev;
+		port->sysfs_device.parent = &adapter->ccw_device->dev;
 	}
-	port->sysfs_device.release = zfcp_sysfs_port_release;
+	port->sysfs_device.release = (void (*)(struct device *))kfree;
 	dev_set_drvdata(&port->sysfs_device, port);
 
 	/* mark port unusable as long as sysfs registration is not complete */
===== zfcp_def.h 1.20 vs edited =====
--- 1.20/drivers/s390/scsi/zfcp_def.h	2004-12-07 11:19:06 +01:00
+++ edited/zfcp_def.h	2005-01-08 10:50:33 +01:00
@@ -903,7 +903,7 @@
 	spinlock_t              dbf_lock;
 	struct zfcp_adapter_mempool	pool;      /* Adapter memory pools */
 	struct qdio_initialize  qdio_init_data;    /* for qdio_establish */
-	struct device           generic_services;  /* directory for WKA ports */
+	struct device           *generic_services; /* directory for WKA ports */
 };
 
 /*
===== zfcp_ext.h 1.13 vs edited =====
--- 1.13/drivers/s390/scsi/zfcp_ext.h	2004-12-07 11:19:06 +01:00
+++ edited/zfcp_ext.h	2005-01-08 10:50:34 +01:00
@@ -47,8 +47,6 @@
 extern void zfcp_sysfs_port_remove_files(struct device *, u32);
 extern int  zfcp_sysfs_unit_create_files(struct device *);
 extern void zfcp_sysfs_unit_remove_files(struct device *);
-extern void zfcp_sysfs_port_release(struct device *);
-extern void zfcp_sysfs_unit_release(struct device *);
 
 /**************************** CONFIGURATION  *********************************/
 extern struct zfcp_unit *zfcp_get_unit_by_lun(struct zfcp_port *, fcp_lun_t);
===== zfcp_sysfs_port.c 1.11 vs edited =====
--- 1.11/drivers/s390/scsi/zfcp_sysfs_port.c	2004-11-16 04:29:22 +01:00
+++ edited/zfcp_sysfs_port.c	2005-01-08 10:50:35 +01:00
@@ -35,16 +35,6 @@
 #define ZFCP_LOG_AREA                   ZFCP_LOG_AREA_CONFIG
 
 /**
- * zfcp_sysfs_port_release - gets called when a struct device port is released
- * @dev: pointer to belonging device
- */
-void
-zfcp_sysfs_port_release(struct device *dev)
-{
-	kfree(dev);
-}
-
-/**
  * ZFCP_DEFINE_PORT_ATTR
  * @_name:   name of show attribute
  * @_format: format string
===== zfcp_sysfs_unit.c 1.9 vs edited =====
--- 1.9/drivers/s390/scsi/zfcp_sysfs_unit.c	2004-11-16 04:29:22 +01:00
+++ edited/zfcp_sysfs_unit.c	2005-01-08 10:50:36 +01:00
@@ -35,16 +35,6 @@
 #define ZFCP_LOG_AREA                   ZFCP_LOG_AREA_CONFIG
 
 /**
- * zfcp_sysfs_unit_release - gets called when a struct device unit is released
- * @dev: pointer to belonging device
- */
-void
-zfcp_sysfs_unit_release(struct device *dev)
-{
-	kfree(dev);
-}
-
-/**
  * ZFCP_DEFINE_UNIT_ATTR
  * @_name:   name of show attribute
  * @_format: format string

Re: [PATCH] zfcp: kfree patch

[Arjan van de Ven]

On Sat, 2005-01-08 at 11:36 +0100, Heiko Carstens wrote:
> Hi James,
> 
> this is my next attempt to try to get the patch below merged. Last
> time Greg KH objected and it didn't get merged.
> 
> This patch addresses the following problem: the zfcp device driver
> creates own sysfs objects and therefore has its own release function
> within the zfcp device driver itself. 

would this be solved by using the fc transport class instead, so that
the scsi layer takes care of all this ?

Re: [PATCH] zfcp: kfree patch

[Greg KH]

On Sat, Jan 08, 2005 at 11:36:41AM +0100, Heiko Carstens wrote:
> Hi James,
> 
> this is my next attempt to try to get the patch below merged. Last
> time Greg KH objected and it didn't get merged.

As nothing has changed in the patch, I still object to it.  Actually,
krefs will BUG() out if you try to do this, so I might as well add a
patch so that kobjects do the same thing...

sorry,

greg k-h

Re: [PATCH] zfcp: kfree patch

[Heiko Carstens]

> > this is my next attempt to try to get the patch below merged. Last
> > time Greg KH objected and it didn't get merged.
> > 
> > This patch addresses the following problem: the zfcp device driver
> > creates own sysfs objects and therefore has its own release function
> > within the zfcp device driver itself. 
> 
> would this be solved by using the fc transport class instead, so that
> the scsi layer takes care of all this ?

What would keep the two release functions defined in
scsi_transport_fc.c from being called _after_ the fc transport class
module was unloaded? As usual I might be wrong but this looks like
the same problem to me.

Heiko

Re: [PATCH] zfcp: kfree patch

[Heiko Carstens]

> > this is my next attempt to try to get the patch below merged. Last
> > time Greg KH objected and it didn't get merged.
> 
> As nothing has changed in the patch, I still object to it.  Actually,
> krefs will BUG() out if you try to do this, so I might as well add a
> patch so that kobjects do the same thing...

So what's the fix for this generic (non-zfcp specific) problem then?

Heiko

Re: [PATCH] zfcp: kfree patch

[Greg KH]

On Mon, Jan 10, 2005 at 07:53:19AM +0100, Heiko Carstens wrote:
> > > this is my next attempt to try to get the patch below merged. Last
> > > time Greg KH objected and it didn't get merged.
> > 
> > As nothing has changed in the patch, I still object to it.  Actually,
> > krefs will BUG() out if you try to do this, so I might as well add a
> > patch so that kobjects do the same thing...
> 
> So what's the fix for this generic (non-zfcp specific) problem then?

As no other code uses this type of "solution" I don't see how you can
say it is "generic".

Re: [PATCH] zfcp: kfree patch

[Heiko Carstens]

> > > As nothing has changed in the patch, I still object to it. Actually,
> > > krefs will BUG() out if you try to do this, so I might as well add a
> > > patch so that kobjects do the same thing...
> > So what's the fix for this generic (non-zfcp specific) problem then?
> As no other code uses this type of "solution" I don't see how you can
> say it is "generic".

Since you still don't like the patch I would like to know how it should
look like that you would accept it.

Also in my original post I described what the problem is. If that problem
exists then it is generic.
Please see also "Module unloading in a reference counted world"
-> http://lwn.net/Articles/67421/

Heiko

Re: [PATCH] zfcp: kfree patch

[Greg KH]

On Mon, Jan 10, 2005 at 09:10:21AM +0100, Heiko Carstens wrote:
> > > > As nothing has changed in the patch, I still object to it. Actually,
> > > > krefs will BUG() out if you try to do this, so I might as well add a
> > > > patch so that kobjects do the same thing...
> > > So what's the fix for this generic (non-zfcp specific) problem then?
> > As no other code uses this type of "solution" I don't see how you can
> > say it is "generic".
> 
> Since you still don't like the patch I would like to know how it should
> look like that you would accept it.

As I don't know the scsi subsystem, I can only rely on the fact that
kfree should never be used as a release call.  So fix that, and I'm
happy :)

> Also in my original post I described what the problem is. If that problem
> exists then it is generic.
> Please see also "Module unloading in a reference counted world"
> -> http://lwn.net/Articles/67421/

I understand the "generic" issue, however no other subsystem needs to
use kfree as a release call.  Now either everyone else is broken or...

thanks,

greg k-h

Re: [PATCH] zfcp: kfree patch

[Heiko Carstens]

> As I don't know the scsi subsystem, I can only rely on the fact that
> kfree should never be used as a release call.  So fix that, and I'm
> happy :)

The code in question has nothing to do with the scsi subsystem at all.
It's just a module which is doing a device_register() call and which
has the release function in the module code as well. The issue still
is that there might be referenced kobjects created by the module while
the module is already unloaded. When the reference count of these
kobjects drop to zero the release function of the already unloaded
module will be called.

> I understand the "generic" issue, however no other subsystem needs to
> use kfree as a release call.  Now either everyone else is broken or...

Hmm.. then maybe you could explain to me how e.g.
drivers/usb/serial/usb-serial.c does this right and keeps the release
function from being called after usb-serial.ko got unloaded?
Sorry, I don't want to be offensive, just looking for a solution :)

Heiko

Re: [PATCH] zfcp: kfree patch

[James Bottomley]

On Mon, 2005-01-10 at 07:41 +0100, Heiko Carstens wrote:
> What would keep the two release functions defined in
> scsi_transport_fc.c from being called _after_ the fc transport class
> module was unloaded? As usual I might be wrong but this looks like
> the same problem to me.

Object lifetime rules:  devices which are attached to transport classes
must be serviced by HBAs which do the attachment.  This makes the
transport class a required module for the HBA.  So the transport class
can't be removed until all of the HBAs have been removed (which means
that all the objects they service have also been removed).

If you're asking how SCSI ensures that all the scsi_devices are
destroyed before the HBA driver is removed, we have a module reference
count system that means the HBA unload waits until everything
relinquishes the devices.

James

Re: [PATCH] zfcp: kfree patch

[Greg KH]

On Mon, Jan 10, 2005 at 10:27:06AM +0100, Heiko Carstens wrote:
> Hmm.. then maybe you could explain to me how e.g.
> drivers/usb/serial/usb-serial.c does this right and keeps the release
> function from being called after usb-serial.ko got unloaded?

usb-serial can not get unloaded if there is a reference held on a sysfs
file owned by it, or if a user has a port open.  This is due to the
module reference counting logic in sysfs attributes, and in the module
reference count stuff in the usb-serial core itself.

Unless I'm missing something, and if so, please let me know and I'll go
fix it up.

thanks,

greg k-h

Re: [PATCH] zfcp: kfree patch

[Heiko Carstens]

> > drivers/usb/serial/usb-serial.c does this right and keeps the release
> > function from being called after usb-serial.ko got unloaded?
> usb-serial can not get unloaded if there is a reference held on a sysfs
> file owned by it, or if a user has a port open.  This is due to the
> module reference counting logic in sysfs attributes, and in the module
> reference count stuff in the usb-serial core itself.

Makes sense. Just wondering if there isn't a race in fs/sysfs/file.c:
the function check_perm() first gets a reference to a kobject in
question and afterwards does a try_module_get(). Now if
sysfs_get_kobject() succeeds but try_module_get() fails, this means
you end up with a kobject where the corresponding module code is
missing and the kobject_put() at the end of check_perm() could
result in a release() call to already unloaded code.
Or not?

Heiko