>> + dev_set_drvdata(&adapter->stat_services, adapter); >> + >> + if (device_register(&adapter->stat_services)) >> + goto services_failed; >> + >> + if (zfcp_sysfs_statistic_services_create_files(&adapter->stat_services)) >> + { >> + device_unregister(&adapter->stat_services); Sorry, but this change won't solve the lifetime problems of objects we have in here. device_unregister(&adapter->stat_services) does not mean you can free the memory used for the struct adapter immediatly after the function returns. What might happen here: device_unregister(&adapter->stat_services); ... kfree(adapter); ... [somewhere else somebody else feels the need to allocate memory] kmalloc(whatever size); [this kmalloc could return the memory you freed above, and the newly allocated memory gets written to and therefore invalidated] ... [now finally the driver core feels like it may call the release function of your struct device, but... the struct device got overwritten already] -> jump to random spot. So we have a use-after-free bug here with potential of memory corruption and random jumps. This is already broken in the current code without this patch and the problem was introduced with the "generic_services" device. Just have a look at zfcp_adapter_dequeue(). Same problem as described above. Needs fixing :) Having a device release function that does nothing (zfcp_dummy_release) is a problem. How can the rest of the code know if the release function was already called and therefore if somebody still needs the struct device if the release function does nothing? Adding more stuff to the zfcp sysfs tree doesn't look like the right way to go for me. It just adds more complexity because of things that you don't even should care about. IMHO you should go the way that Matthew Wilcox proposed and add your new attributes somewhere in the scsi midlayer or transport class code. >> @@ -1264,6 +1285,7 @@ zfcp_port_enqueue(struct zfcp_adapter *a >> if (zfcp_sysfs_port_create_files(&port->sysfs_device, status)) { >> device_unregister(&port->sysfs_device); >> + kfree(port); > > Scratching my head... another unrelated change (looks like a fix for a real > memory leak)? Adding a kfree() in here is wrong. The struct port gets freed when the release function of the sysfs_device gets called. So you would end up freeing the struct port twice and one of them is too early (see above). Btw. this kfree() line wasn't part of your first patch. It would be helpful if you outline what and why you changed something, even if that would be just to reply on my first review. That way whoever commented on your patch would know if you agree or why you don't. E.g. you still have the same four nearly identical functions. So it looks to me like you disagree with what I suggested, but why? - To unsubscribe from this list: send the line "unsubscribe linux-s390" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html