Unregistering interfaces
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 28 lines · Andrew Morton
Quoted 17 lines · Greg KH
Quoted 8 lines · Andrew Morton
Quoted 5 lines · Alan Stern
However, a very noticeable and IMO unacceptable delay occurs when there's a reference to a kobject caused by a negative dentry that won't get recycled until the system decides it's good and ready. That's the real problem I wanted to call to people's attention.
Have you verified that this actually happens?
Yes, I've verified this, and it looks like Maneesh also agrees with this.
Quoted 2 lines
If so, what are its effects? rmmod hangs for half an hour? Cannot reload the module?
Quoted 9 lines
Well, before the patch that I submitted for the USB core, we would hang waiting for the release function to return as there was still a reference to the kobject pending.
For other subsystems (and USB), this might cause nasty oopses when the kobject is finally released and yet the module has been unloaded already (as the owner of the reference did not cause the module reference count to increment.) This is bad.
Quoted 7 lines
The module should remain in memory, "unhashed", until the final kobject reference falls to zero. Destruction of that kobject causes the refcount on the module to fall to zero which causes the entire module to be released.
(hmm, the existence of a kobject doesn't appear to contribute to its module's refcount. Why not?)
It does, if a file for that kobject is opened. In this case, there was no file opened, so the module refcount isn't incremented.
Quoted 4 lines
But the module system is that smart, so what we do instead is to block rmmod until the module refcount falls to zero, and rmmod then does the final destruction. We could have punted the module destruction up to some reaper thread but for some reason did not do so.
Well, as the module refcount was never incremented, it does not do this. I just verified this (accidentally) by removing my lp modules and then doing a bk pull. kswapd forced the stale dentry out, which decremented the kobject reference count, which then tried to call the release function in the (now gone) lp module. My machine then spit up a lovely oops, and was reduced to a unusable sludge as there was no more kswapd running :(
Quoted 2 lines
So as far as I can tell, the only problem we have is that rmmod will hang around until memory pressure, yes?
Nope, oopses will happen. You can verify this yourself by doing much of what I just did.
This needs to get fixed, as it's not just a USB issue (so I've added lkml on the cc list.)
Quoted 2 lines
Maybe a
shrink_dcache_parent(dentry)on entry tosimple_rmdir()would suffice?
Will that get rid of the references properly nwhen we remove the kobject?
thanks,
greg k-h
Quoted 11 lines · Greg KH
Quoted 8 lines
The module should remain in memory, "unhashed", until the final kobject reference falls to zero. Destruction of that kobject causes the refcount on the module to fall to zero which causes the entire module to be released.
(hmm, the existence of a kobject doesn't appear to contribute to its module's refcount. Why not?)
It does, if a file for that kobject is opened. In this case, there was no file opened, so the module refcount isn't incremented.
hm, surprised. Shouldn't the existence of a kobject contribute to its module's refcount?
Quoted 5 lines
Quoted 2 lines
Maybe a
shrink_dcache_parent(dentry)on entry tosimple_rmdir()would suffice?Will that get rid of the references properly nwhen we remove the kobject?
That's one the dcache guys could address better, but I was mainly proposing it as a way of removing any negative dentries. But it appears that we have problems beyond negative dentries?
Quoted 7 lines · Andrew Morton
Quoted 6 lines · Greg KH
Quoted 6 lines
The module should remain in memory, "unhashed", until the final kobject reference falls to zero. Destruction of that kobject causes the refcount on the module to fall to zero which causes the entire module to be released.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
(hmm, the existence of a kobject doesn't appear to contribute to its module's refcount. Why not?)
It does, if a file for that kobject is opened. In this case, there was no file opened, so the module refcount isn't incremented.
hm, surprised. Shouldn't the existence of a kobject contribute to its module's refcount?
No, a kobject by itself knows nothing about a module. Only the attribute files do (and they are the things that contain the struct module *), as they are what user space can grab references to.
I never thought that the kobject would care, as it is only a directory, and I didn't think that anything could grab directory references on their own. But then Maneesh's patch wasn't in the kernel at that time :)
thanks,
greg k-h
Quoted 17 lines · Greg KH
Quoted 7 lines · Andrew Morton
Quoted 6 lines · Greg KH
Quoted 6 lines
The module should remain in memory, "unhashed", until the final kobject reference falls to zero. Destruction of that kobject causes the refcount on the module to fall to zero which causes the entire module to be released.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
(hmm, the existence of a kobject doesn't appear to contribute to its module's refcount. Why not?)
It does, if a file for that kobject is opened. In this case, there was no file opened, so the module refcount isn't incremented.
hm, surprised. Shouldn't the existence of a kobject contribute to its module's refcount?
Quoted 9 lines
No, a kobject by itself knows nothing about a module. Only the attribute files do (and they are the things that contain the struct module *), as they are what user space can grab references to.
I never thought that the kobject would care, as it is only a directory, and I didn't think that anything could grab directory references on their own. But then Maneesh's patch wasn't in the kernel at that time :)
I don't how fisible is it to move the owner field to the kobject and keep the module and kobject both alive as long as there are sysfs directories referring to them.
Maneesh
Signature · 6 lines
Maneesh Soni Linux Technology Center, IBM Software Lab, Bangalore, India email: [email protected] Phone: 91-80-25044999 Fax: 91-80-25268553 T/L : 9243696
Quoted 14 lines · Greg KH
Quoted 13 lines · Andrew Morton
Quoted 11 lines · Greg KH
Quoted 8 lines
The module should remain in memory, "unhashed", until the final kobject reference falls to zero. Destruction of that kobject causes the refcount on the module to fall to zero which causes the entire module to be released.
(hmm, the existence of a kobject doesn't appear to contribute to its module's refcount. Why not?)
It does, if a file for that kobject is opened. In this case, there was no file opened, so the module refcount isn't incremented.
Quoted 6 lines
Quoted 2 lines
hm, surprised. Shouldn't the existence of a kobject contribute to its module's refcount?
No, a kobject by itself knows nothing about a module. Only the attribute files do (and they are the things that contain the struct module *), as they are what user space can grab references to.
There's another reason. Practically every module has kobjects registered all the time; they are not unregistered until the module's unload procedure runs. If these kobjects contributed to the modules refcount then it would be impossible ever to rmmod the module without using the -f flag.
Furthermore, all the excess usage counts from those kobjects would swamp and hide the genuine usage counts (the ones that are displayed now). The user would have no way to know whether rmmod -f would work or would hang.
Alan Stern
Quoted 19 lines · Andrew Morton
Quoted 11 lines · Greg KH
Quoted 8 lines
The module should remain in memory, "unhashed", until the final kobject reference falls to zero. Destruction of that kobject causes the refcount on the module to fall to zero which causes the entire module to be released.
(hmm, the existence of a kobject doesn't appear to contribute to its module's refcount. Why not?)
It does, if a file for that kobject is opened. In this case, there was no file opened, so the module refcount isn't incremented.
hm, surprised. Shouldn't the existence of a kobject contribute to its module's refcount?
Quoted 3 lines
Quoted 2 lines
Maybe a
shrink_dcache_parent(dentry)on entry tosimple_rmdir()would suffice?
Quoted 6 lines
Quoted 2 lines
Will that get rid of the references properly nwhen we remove the kobject?
That's one the dcache guys could address better, but I was mainly proposing it as a way of removing any negative dentries. But it appears that we have problems beyond negative dentries?
shrink_dcache_parent() will only free up the zero ref. counted dentries, positive or negatvie doesnot matter. But we may have some faulty user space app holding a sysfs file, sysfs directory, corresponding kobject, module etc. Refer http://bugme.osdl.org/show_bug.cgi?id=1884
(Greg, I am including the reply for other thread also here)
http://marc.theaimsgroup.com/?l=linux-kernel&m=108059485726012&w=2
Fix for these problems will depend upon the life time rule for modules Vs kobjects Vs dentries. When and what should die or live?
I am not very clear about how the first two behave. Still I can think of a solution within sysfs like this as Alen suggested. But again I am not very sure if this can be done properly without any races. But anyway I am trying.
1) backout my patch sysfs-pin-kobject.patch
2) handle NULL d_fsdata while trying to access the corresponding kobject for an attribute file.
Maneesh
Signature · 6 lines
Maneesh Soni Linux Technology Center, IBM Software Lab, Bangalore, India email: [email protected] Phone: 91-80-25044999 Fax: 91-80-25268553 T/L : 9243696
Quoted 7 lines · Maneesh Soni
I am not very clear about how the first two behave. Still I can think of a solution within sysfs like this as Alen suggested. But again I am not very sure if this can be done properly without any races. But anyway I am trying.
1) backout my patch sysfs-pin-kobject.patch
I think we need to do this now, as it is not a correct fix, and causes more problems than good at this time. I suggest you try to fix the oops you were seeing in either another way, or in a way that does not break other things :)
I'll send a patch off to Linus to do this in a little bit...
thanks,
greg k-h
Quoted 8 lines · Greg KH
Quoted 6 lines · Maneesh Soni
I am not very clear about how the first two behave. Still I can think of a solution within sysfs like this as Alen suggested. But again I am not very sure if this can be done properly without any races. But anyway I am trying.
Quoted 4 lines
Quoted 1 line
1) backout my patch sysfs-pin-kobject.patch
I think we need to do this now, as it is not a correct fix, and causes more problems than good at this time.
But the patch was correct. sysfs retains a pointer to the kobject, it should take a ref on it?
Quoted 3 lines
I suggest you try to fix the oops
you were seeing in either another way, or in a way that does not break other things :)
Didn't we demonstrate that the code which broke was already broken? And that it has other problems regardless of the kobject pinning fix, such as the userpace-holding-a-file-open-wedges-khubd problem?
Worried that this is all heading in the wrong direction...
Quoted 9 lines · Andrew Morton
Quoted 8 lines · Greg KH
Quoted 6 lines · Maneesh Soni
I am not very clear about how the first two behave. Still I can think of a solution within sysfs like this as Alen suggested. But again I am not very sure if this can be done properly without any races. But anyway I am trying.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
1) backout my patch sysfs-pin-kobject.patch
I think we need to do this now, as it is not a correct fix, and causes more problems than good at this time.
But the patch was correct. sysfs retains a pointer to the kobject, it should take a ref on it?
Yes, but it was taking references for files that are not present in sysfs. That's not good :)
Quoted 7 lines
Quoted 3 lines
I suggest you try to fix the oops
you were seeing in either another way, or in a way that does not break other things :)Didn't we demonstrate that the code which broke was already broken? And that it has other problems regardless of the kobject pinning fix, such as the userpace-holding-a-file-open-wedges-khubd problem?
No. We fixed the wedge-khubd issue. That was my fault for allowing that change to go in the first place. Sorry about that.
Let me run some tests with Maneesh's patch pulled out to see if that solves the oopses I can generate...
Quoted 1 line
Worried that this is all heading in the wrong direction...
I'm worried about that too.
thanks,
greg k-h
Quoted 3 lines · Greg KH
Let me run some tests with Maneesh's patch pulled out to see if that solves the oopses I can generate...
With the patch removed, I can't oops the kernel. With it in, I can easily oops it. So I sent the backout patch to Linus.
I'll work with Maneesh to try to solve the original problem that he saw and tried to fix in such a way that does not cause other problems.
thanks,
greg k-h
Quoted 12 lines · Andrew Morton
Quoted 3 lines · Greg KH
I think we need to do this now, as it is not a correct fix, and causes more problems than good at this time.
But the patch was correct. sysfs retains a pointer to the kobject, it should take a ref on it?
Quoted 3 lines
I suggest you try to fix the oops
you were seeing in either another way, or in a way that does not break other things :)
Quoted 5 lines
Didn't we demonstrate that the code which broke was already broken? And that it has other problems regardless of the kobject pinning fix, such as the userpace-holding-a-file-open-wedges-khubd problem?
Worried that this is all heading in the wrong direction...
There are two problems to consider:
(1) sysfs retains pointers to kobjects long after they have been unregistered because of the negative dentrys.
(2) khubd blocks when removing configurations.
Maneesh's change caused (1) because it grabbed a reference to protect an existing pointer. The way to repair the damage is to delete the pointer ASAP so the reference can be dropped. That will require changing several other sysfs routines that assume the pointer is valid.
Such a change is needed because otherwise module unloading can be delayed indefinitely, until the system decides it needs to reuse the negative dentry.
(2) has been repaired temporarily. We're still discussing the right way to fix it permanently, though. The underlying cause for the reason that khubd blocked is an odd feature of sysfs: it's willing to delete directories that contain subdirectories, while leaving the subdirectories in existence. (The connection to khubd is slightly obscure but it can be traced.)
Greg has proposed using an awkward scheme for repeatedly parsing and creating dynamic data structures from USB configuration descriptors to fix (2). I'm in favor of changing the behavior of sysfs, so that either it refuses to delete directories that contain subdirectories or else it recursively deletes the subdirectories first. At this point nothing has been settled.
Alan Stern
Quoted 4 lines · Alan Stern
I'm in favor of changing the behavior of sysfs, so that either it
refuses to delete directories that contain subdirectories or else it recursively deletes the subdirectories first. At this point nothing has been settled.
Hmm, certainly I agree khubd should be deleting things bottom-up.
Are you say it isn't doing that already? Or that it's trying to, but something's preventing that from working?
- Dave
Quoted 11 lines · David Brownell
Quoted 4 lines · Alan Stern
I'm in favor of changing the behavior of sysfs, so that either it
refuses to delete directories that contain subdirectories or else it recursively deletes the subdirectories first. At this point nothing has been settled.Hmm, certainly I agree khubd should be deleting things bottom-up.
Are you say it isn't doing that already? Or that it's trying to, but something's preventing that from working?
Look at everything that hangs off of usb devices in the sysfs tree that the usb core knows nothing about (scsi hosts, tty devices, etc.)
Anyway, it's working properly now, and is why I added that kobject parent patch a long time ago (which Alan still seems to think is incorrect...)
thanks,
greg k-h
Quoted 11 lines · David Brownell
Quoted 4 lines · Alan Stern
I'm in favor of changing the behavior of sysfs, so that either it
refuses to delete directories that contain subdirectories or else it recursively deletes the subdirectories first. At this point nothing has been settled.Hmm, certainly I agree khubd should be deleting things bottom-up.
Are you say it isn't doing that already? Or that it's trying to, but something's preventing that from working?
No, no -- as far as I know khudb gets rid of things in a perfectly correct manner. But there are plenty of other parts in the Linux kernel, and I don't know that they all delete things bottom-up. Greg implies that usb-serial and the kernel's tty subsystem don't do this; I haven't had a chance to look into it.
Alan Stern
Quoted 18 lines · Alan Stern
Quoted 16 lines · Andrew Morton
Quoted 3 lines · Greg KH
I think we need to do this now, as it is not a correct fix, and causes more problems than good at this time.
But the patch was correct. sysfs retains a pointer to the kobject, it should take a ref on it?
Quoted 3 lines
I suggest you try to fix the oops
you were seeing in either another way, or in a way that does not break other things :)Didn't we demonstrate that the code which broke was already broken? And that it has other problems regardless of the kobject pinning fix, such as the userpace-holding-a-file-open-wedges-khubd problem?
Quoted 6 lines
Quoted 1 line
Worried that this is all heading in the wrong direction...
There are two problems to consider:
(1) sysfs retains pointers to kobjects long after they have been unregistered because of the negative dentrys.
That is now taken care of with the patch I just sent to Linus by taking out this patch.
Quoted 1 line
(2) khubd blocks when removing configurations.
That was fixed by the patch from you, undoing your previous patch which blocked.
So everyone is happy now, right? :)
thanks,
greg k-h
Quoted 9 lines · Greg KH
Quoted 5 lines · Alan Stern
There are two problems to consider:
(1) sysfs retains pointers to kobjects long after they have been unregistered because of the negative dentrys.
That is now taken care of with the patch I just sent to Linus by taking out this patch.
Yes, and Maneesh is working to replace it with a more suitable approach.
Quoted 6 lines
Quoted 1 line
(2) khubd blocks when removing configurations.
That was fixed by the patch from you, undoing your previous patch which blocked.
So everyone is happy now, right? :)
That change was just a temporary fix, never intended as a permanent solution.
Here's a suggestion for a correct solution. It's a little bit awkward, but less so than other ideas I've heard.
Define struct usb_interface_cache to be a subset of struct usb_interface. All it needs to contain is the altsetting pointer and num_altsetting. Within the usb_host_configuration structure add an array of usb_interface_cache pointers in addition to the existing array of usb_interface pointers.
When config.c parses the configuration descriptors, it will create the usb_interface_cache structures instead of creating the usb_interfaces. Things like configuration selection and /proc/bus/usb/devices can get all the information they need from this.
When a configuration is installed, usb_set_configuration() will dynamically perform a deep copy of the usb_interface_cache array and store the entries in the usb_interface array. These will be "live" entries, containing the struct device and everything else for use by drivers (exactly the same as they do now).
When a configuration is uninstalled, the usb_interface array can be erased and the usual kobject cleanup mechanism will delete the deep-copy array elements whenever it wants to. There will be no problems with reinstalling the configuration because when that happens, new interfaces will be allocated dynamically. Hence there will be no need for khubd to block, no need for changes to sysfs, and then everyone really will be happy! :-)
Although there is extra overhead in doing all the copying, it only happens when configurations are changed. Furthermore, since we will know in advance the sizes of all the structures to be copied, we will be able to allocate a single memory region to hold all the altsetting and endpoint structures for each interface.
If you think this sounds good, I will start working on it.
Alan Stern
Quoted 2 lines · Alan Stern
Here's a suggestion for a correct solution. It's a little bit awkward, but less so than other ideas I've heard.
<snip>
Quoted 1 line
If you think this sounds good, I will start working on it.
Yes, this sounds like an excellent way of doing this, and is what I was mulling over today in between doing real work :)
It would be great if you want to work on this, and have the time to. Feel free to post incremental changes, and if you need any other help with this.
thanks for volunteering.
greg k-h
PATCH 1/1 back out sysfs reference count change
In reply to Greg KHHi,
The patch below backs out Maneesh's sysfs patch that was recently added to the kernel. In its defense, the original patch did solve some fixes that could be duplicated on SMP machines, but the side affect of the patch caused lots of problems. Basically it caused kobjects to get their references incremented when files that are not present in the kobject are asked for (udev can easily trigger this when it looks for files call "dev" in directories that do not have that file). This can cause easy oopses when the VFS later ages out those old dentries and the kobject has its reference finally released (usually after the module that the kobject lived in was removed.)
I will continue to work with Maneesh to try to solve the original bug, but for now, this patch needs to be applied.
thanks,
greg k-h
fs/sysfs/dir.c +1 -14
@@ -20,18 +20,6 @@return 0 ; } -static void sysfs_d_iput (struct dentry * dentry,struct inode * inode) -{ -struct kobject * kobj = dentry->d_fsdata; - -if (kobj) -kobject_put (kobj); -iput (inode); -} - -static struct dentry_operations sysfs_dentry_operations = { - .d_iput = &sysfs_d_iput, -};static int create_dir (struct kobject * k,struct dentry * p,const char * n,struct dentry ** d)
@@ -45,8 +33,7 @@S_IFDIR |S_IRWXU |S_IRUGO |S_IXUGO , init_dir);if (!error) { - (*d)->d_op = &sysfs_dentry_operations; - (*d)->d_fsdata =kobject_get (k); + (*d)->d_fsdata = k; p->d_inode->i_nlink++; }dput (*d);
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
Quoted 12 lines · Greg KH
Hi,
The patch below backs out Maneesh's sysfs patch that was recently added to the kernel. In its defense, the original patch did solve some fixes that could be duplicated on SMP machines, but the side affect of the patch caused lots of problems. Basically it caused kobjects to get their references incremented when files that are not present in the kobject are asked for (udev can easily trigger this when it looks for files call "dev" in directories that do not have that file). This can cause easy oopses when the VFS later ages out those old dentries and the kobject has its reference finally released (usually after the module that the kobject lived in was removed.)
I think that the bug in the first place is to have an existing kobject that didn't bump the module ref count.
If a kobject exists that have a pointer to the module code (the release function), it _MUST_ have bumped the module ref count, that's the whole point of the module reference count.
If rmmod blocks forever because that kobject has a stale reference, that's a different problem, but khubd should not be involved in that process and should definitely not be blocked and not wait for the kobject to go away.
Ben.
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
Quoted 4 lines · Benjamin Herrenschmidt
I think that the bug in the first place is to have an existing kobject that didn't bump the module ref count.
Quoted 8 lines
If a kobject exists that have a pointer to the module code (the release function), it _MUST_ have bumped the module ref count, that's the whole point of the module reference count.
If rmmod blocks forever because that kobject has a stale reference, that's a different problem, but khubd should not be involved in that process and should definitely not be blocked and not wait for the kobject to go away.
Amen, Brother Ben. I think the hangup here is that lsmod would show module refcounts of 2,417. So for cosmetic reasons, the kobject should take a ref against an intermediate kref, which has a single ref on the module.
But it looks like that's all in a faraway perfect world, and Greg is going to fix stuff up somehow ;)
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
Quoted 3 lines · Andrew Morton
But it looks like that's all in a faraway perfect world, and Greg is going to fix stuff up somehow ;)
For convenience I will explain the race here..
cpu 0 cpu 1kobject_unregister() sysfs_open_file()
kobject_del() check_perm()
sysfs_remove_dir() :
(dentry remains alive due to ref. taken :
on the way to sysfs_open_file) :
kobject_put() :
kobject_cleanup() kobject_get(->d_fsdata)
cpu 1 could end up referring to a freed kobject through dentry->d_fsdata or starts spitting Badness in kobject_get at lib/kobject.c:429. For triggering this race try running these two loops simultaneously on SMP
# while true; do insmod drivers/net/dummy.ko; rmmod dummy; done # while true; do find /sys/class/net | xargs cat; done
Probably it can be solved by making sure that when sysfs file is opened/read/written some _race_ free check is done and fail if kobject if gone.
Maneesh
Signature · 6 lines
Maneesh Soni Linux Technology Center, IBM Software Lab, Bangalore, India email: [email protected] Phone: 91-80-25044999 Fax: 91-80-25268553 T/L : 9243696
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
Quoted 15 lines · Maneesh Soni
For convenience I will explain the race here..
cpu 0 cpu 1
kobject_unregister()sysfs_open_file()
kobject_del()check_perm()
sysfs_remove_dir():
(dentry remains alive due to ref. taken :
on the way tosysfs_open_file) :
kobject_put():kobject_cleanup() kobject_get(->d_fsdata)cpu 1 could end up referring to a freed kobject through
dentry->d_fsdataor starts spitting Badness inkobject_getatlib/kobject.c:429. For triggering this race try running these two loops simultaneously on SMP
Quoted 7 lines
# while true; do insmod drivers/net/dummy.ko; rmmod dummy; done # while true; do find /sys/class/net | xargs cat; doneProbably it can be solved by making sure that when sysfs file is opened/read/written some _race_ free check is done and fail if kobject if gone.
Maneesh
Here's a suggestion. At the start of check_perm() grab the dentry semaphore, then check whether d_fsdata is NULL, if it isn't then do the kobject_get(), then unlock the semaphore.
Alan Stern
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
Quoted 20 lines · Alan Stern
Quoted 18 lines · Maneesh Soni
For convenience I will explain the race here..
cpu 0 cpu 1
kobject_unregister()sysfs_open_file()
kobject_del()check_perm()
sysfs_remove_dir():
(dentry remains alive due to ref. taken :
on the way tosysfs_open_file) :
kobject_put():kobject_cleanup() kobject_get(->d_fsdata)cpu 1 could end up referring to a freed kobject through
dentry->d_fsdataor starts spitting Badness inkobject_getatlib/kobject.c:429. For triggering this race try running these two loops simultaneously on SMP# while true; do insmod drivers/net/dummy.ko; rmmod dummy; done # while true; do find /sys/class/net | xargs cat; done
Quoted 9 lines
Quoted 4 lines
Probably it can be solved by making sure that when sysfs file is opened/read/written some _race_ free check is done and fail if kobject if gone.
Maneesh
Here's a suggestion. At the start of
check_perm()grab the dentry semaphore, then check whetherd_fsdatais NULL, if it isn't then do thekobject_get(), then unlock the semaphore.
I have tried this with no luck. I still get
(Badness in kobject_get at lib/kobject.c:42) which means it is not correct fix.
I am out of any more ideas except something like making sysfs single threaded or requesting people to try my sysfs backing store patch set. It does not suffer from the negative dentries problem as it does not create any negative dentries. I have to re-diff the patch set again to take recent changes into account.
Maneesh
Signature · 6 lines
Maneesh Soni Linux Technology Center, IBM Software Lab, Bangalore, India email: [email protected] Phone: 91-80-25044999 Fax: 91-80-25268553 T/L : 9243696
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
Quoted 5 lines · Maneesh Soni
I am out of any more ideas except something like making sysfs single threaded or requesting people to try my sysfs backing store patch set. It does not suffer from the negative dentries problem as it does not create any negative dentries. I have to re-diff the patch set again to take recent changes into account.
Hm, well that is the best reason so far that I have heard to accept your patch set.
So let's revisit this after Al audits the patches, ok?
thanks,
greg k-h
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
In reply to Maneesh SoniQuoted 9 lines · Maneesh Soni
Quoted 5 lines · Alan Stern
Here's a suggestion. At the start of
check_perm()grab the dentry semaphore, then check whetherd_fsdatais NULL, if it isn't then do thekobject_get(), then unlock the semaphore.I have tried this with no luck. I still get
(Badness inkobject_getatlib/kobject.c:42) which means it is not correct fix.
Did you remember to set dentry->d_fsdata to NULL? This has to be done in sysfs_remove_dir() sometime during the period when dentry->d_inode->i_sem is held. Right now the pointer to the kobject never gets erased. That Badness message is an indication that you are using a valid pointer to a kobject whose refcount is 0.
Alan Stern
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
Quoted 9 lines · Alan Stern
Quoted 7 lines · Maneesh Soni
Quoted 5 lines · Alan Stern
Here's a suggestion. At the start of
check_perm()grab the dentry semaphore, then check whetherd_fsdatais NULL, if it isn't then do thekobject_get(), then unlock the semaphore.
Quoted 9 lines
Quoted 2 lines
I have tried this with no luck. I still get
(Badness inkobject_getatlib/kobject.c:42) which means it is not correct fix.Did you remember to set
dentry->d_fsdatato NULL? This has to be done insysfs_remove_dir()sometime during the period whendentry->d_inode->i_semis held. Right now the pointer to the kobject never gets erased. That Badness message is an indication that you are using a valid pointer to a kobject whose refcount is 0.
Yes, see the patch below. Probably the race window has become smaller but Badness message is also an indication that somewhere kobject_cleanup has started. I have not yet seen why race is still there.
There is one more bigger problem in this approach. It may miss kobject_put() in sysfs_release() call and result will be memory leak, ->release() is not called, rmmod hang, etc etc. So using ->d_fsdata for this purpose is not the proper solution, IMO.
cpu 0 kobj->refcount cpu 1kobject_unregister() (1) sysfs_open_file()
kobject_del() check_perm()
sysfs_remove_dir() (2) kobject_get(->d_fsdata) (->d_fsdata = NULL) kobject_put() (1) sysfs_release() kobj == NULL
fs/sysfs/dir.c +5 -0
@@ -132,6 +132,11 @@pr_debug ("sysfs %s: removing dir\n" ,dentry->d_name.name);down (&dentry->d_inode->i_sem); +/* try to avoid further references to kobject even if dentry +* is alive after remove dir. +*/ + dentry->d_fsdata =NULL ; +spin_lock (&dcache_lock); restart: node = dentry->d_subdirs.next;
fs/sysfs/file.c +42 -4
84 lines not on this page · raw message
Signature · 6 lines
Maneesh Soni Linux Technology Center, IBM Software Lab, Bangalore, India email: [email protected] Phone: 91-80-25044999 Fax: 91-80-25268553 T/L : 9243696
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
[CC: list pruned on the assumption that most recipients aren't interested]
Quoted 3 lines · Maneesh Soni
Yes, see the patch below. Probably the race window has become smaller but Badness message is also an indication that somewhere
kobject_cleanuphas started. I have not yet seen why race is still there.
Yes. Although the problem can't be due to a race if it involves code that's protected by a semaphore.
Did this Badness message come from within check_perm()? I don't see how that could happen with this patch. You're guaranteed that the refcount is positive until after sysfs_remove_dir() returns. It's _supposed_ to be positive, anyway; maybe it isn't. You could try checking for that, at the end of sysf_remove_dir().
Quoted 4 lines
There is one more bigger problem in this approach. It may miss
kobject_put()insysfs_release()call and result will be memory leak, ->release() is not called, rmmod hang, etc etc. So using ->d_fsdata for this purpose is not the proper solution, IMO.
You're quite correct. d_fsdata must remain set so that previous references can be dropped. Some other part of the dentry or the inode has to be used instead. I don't know what would be appropriate, perhaps one of the flags bits.
Alan Stern
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
Quoted 8 lines · Alan Stern
[CC: list pruned on the assumption that most recipients aren't interested]
Quoted 3 lines · Maneesh Soni
Yes, see the patch below. Probably the race window has become smaller but Badness message is also an indication that somewhere
kobject_cleanuphas started. I have not yet seen why race is still there.
Quoted 9 lines
Yes. Although the problem can't be due to a race if it involves code that's protected by a semaphore.
Did this Badness message come from within
check_perm()? I don't see how that could happen with this patch. You're guaranteed that the refcount is positive until aftersysfs_remove_dir()returns. It's _supposed_ to be positive, anyway; maybe it isn't. You could try checking for that, at the end ofsysf_remove_dir().
Yes, it came from check_perm(). I think I found the reason for that. The attribute group subdirectory's dentry also has a pointer to the same kobject as the corresponding directory's dentry. The kobject directory dentry was taken care of but the attribute group subdirectory was still pointing to the kobject. And that badness message was coming while opening a file under attribute subdir.
I am using dentry->d_flags and a new flag value DCACHE_SYSFS_CONNECTED to indicate that the dentry is connected to a vaild kobject. I could run my stress test of insmod/rmmod for more than 3 hours without any badness message.
I am copying to the maintainers also and hope to get their comments for this patch.
Thanks
Maneesh
o The following patch fixes the race involved between unregistering a kobject and simultaneously opeing a corresponding attribute file in sysfs.
Ideally sysfs should take a ref. to the kobject as long as it has dentries referring to the kobject, but because of current limitations in module/kobject ref counting, sysfs's pinning of kobject leads to hang/delays in rmmod of certain modules. The patch uses a new flag to mark dentries as disconnected from a valid kobject in the process of unregistering the kobject. The ->open in sysfs is failed if it finds a disconnected dentry. Marking and checking for the flag is done under dentry->d_inode->i_sem.
fs/sysfs/bin.c | 8 +++++++- fs/sysfs/dir.c | 6 ++++++ fs/sysfs/file.c | 11 ++++++++++- fs/sysfs/group.c | 19 +++++++++++++------ include/linux/dcache.h | 1 + 5 files changed, 37 insertions(+), 8 deletions(-)
include/linux/dcache.h renamed from include/linux/dcache.h~sysfs-d_fsdata-race-fix +1 -0
@@ -153,6 +153,7 @@ d_iput: no no yes#define DCACHE_REFERENCED 0x0008/* Recently used, don't discard. */ #define DCACHE_UNHASHED 0x0010 +#define DCACHE_SYSFS_CONNECTED 0x0020/* dentry connected to vaild kobject */ extern spinlock_t dcache_lock;
fs/sysfs/dir.c renamed from fs/sysfs/dir.c~sysfs-d_fsdata-race-fix +6 -0
@@ -34,6 +34,7 @@ static int create_dir(struct kobject * k init_dir);if (!error) { (*d)->d_fsdata = k; + (*d)->d_flags =DCACHE_SYSFS_CONNECTED ; p->d_inode->i_nlink++; }dput (*d);
@@ -119,6 +120,11 @@ void sysfs_remove_dir(struct kobject * kpr_debug ("sysfs %s: removing dir\n" ,dentry->d_name.name);down (&dentry->d_inode->i_sem); +/* avoid further references to kobject even if dentry +* is alive after remove dir. +*/ + dentry->d_flags &= ~DCACHE_SYSFS_CONNECTED ; +spin_lock (&dcache_lock); restart: node = dentry->d_subdirs.next;
fs/sysfs/file.c renamed from fs/sysfs/file.c~sysfs-d_fsdata-race-fix +10 -1
@@ -134,6 +134,9 @@ static int flush_read_buffer(struct sysf* is in the file's ->d_fsdata. The target object is in the directory's * ->d_fsdata. * +* It is safe to use ->d_parent->d_fsdata as both dentry and the kobject +* are pinned in ->open(). +* * We call fill_read_buffer() to allocate and fill the buffer from the * object's show() method exactly once (if the read is happening from * the beginning of the file). That should fill the entire buffer with
@@ -238,12 +241,18 @@ sysfs_write_file(struct file *file, consstatic int check_perm (struct inode * inode,struct file * file) { -struct kobject * kobj =kobject_get (file->f_dentry->d_parent->d_fsdata); +struct dentry * kobj_dir = file->f_dentry->d_parent; +struct kobject * kobj =NULL ;struct attribute * attr = file->f_dentry->d_fsdata;struct sysfs_buffer * buffer;struct sysfs_ops * ops =NULL ;int error =0 ; +down (&kobj_dir->d_inode->i_sem); +if (kobj_dir->d_flags &DCACHE_SYSFS_CONNECTED ) + kobj =kobject_get (kobj_dir->d_fsdata); +up (&kobj_dir->d_inode->i_sem); +if (!kobj || !attr) goto Einval;
fs/sysfs/bin.c renamed from fs/sysfs/bin.c~sysfs-d_fsdata-race-fix +7 -1
@@ -94,9 +94,15 @@ static ssize_t write(struct file * file,static int open (struct inode * inode,struct file * file) { -struct kobject * kobj =kobject_get (file->f_dentry->d_parent->d_fsdata); +struct dentry * kobj_dir = file->f_dentry->d_parent; +struct kobject * kobj =NULL ;struct bin_attribute * attr = file->f_dentry->d_fsdata;int error = -EINVAL ; + +down (&kobj_dir->d_inode->i_sem); +if (kobj_dir->d_flags &DCACHE_SYSFS_CONNECTED ) + kobj =kobject_get (kobj_dir->d_fsdata); +up (&kobj_dir->d_inode->i_sem);if (!kobj || !attr) goto Done;
fs/sysfs/group.c renamed from fs/sysfs/group.c~sysfs-d_fsdata-race-fix +13 -6
@@ -10,7 +10,7 @@#include <linux/kobject.h> #include <linux/module.h> -#include <linux/dcache.h> +#include <linux/fs.h> #include <linux/err.h> #include "sysfs.h"
@@ -70,11 +70,18 @@ void sysfs_remove_group(struct kobject *else dir =dget (kobj->dentry); -remove_files (dir,grp); -if (grp->name) -sysfs_remove_subdir (dir); -/* release the ref. taken in this routine */ -dput (dir); +if (!IS_ERR (dir)) { +if (dir && dir->d_inode) { +down (&dir->d_inode->i_sem); + dir->d_flags &= ~DCACHE_SYSFS_CONNECTED ; +up (&dir->d_inode->i_sem); +remove_files (dir,grp); +if (grp->name) +sysfs_remove_subdir (dir); +/* release the ref. taken in this routine */ +dput (dir); + } + } }
_
Signature · 6 lines
Maneesh Soni Linux Technology Center, IBM Software Lab, Bangalore, India email: [email protected] Phone: 91-80-25044999 Fax: 91-80-25268553 T/L : 9243696
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
Quoted 11 lines · Maneesh Soni
Yes, it came from
check_perm(). I think I found the reason for that. The attribute group subdirectory's dentry also has a pointer to the same kobject as the corresponding directory's dentry. The kobject directory dentry was taken care of but the attribute group subdirectory was still pointing to the kobject. And that badness message was coming while opening a file under attribute subdir.I am using
dentry->d_flagsand a new flag valueDCACHE_SYSFS_CONNECTEDto indicate that the dentry is connected to a vaild kobject. I could run my stress test of insmod/rmmod for more than 3 hours without any badness message.
Quoted 5 lines
I am copying to the maintainers also and hope to get their comments for this patch.
Thanks
Maneesh
I like your patch a lot. It suggests an additional possibility. Maybe your DCACHE_SYSFS_CONNECTED flag could be used to indicate that an attribute is registered. That way, after the file is unregistered (even if the kobject still exists) reads and writes of the attribute would return an error. This isn't really necessary since opening an attribute takes a reference to the module owning the attribute, but it is conceptually attractive.
Alan Stern
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
In reply to Maneesh SoniOn Tue, Apr 06, 2004 at 03:43:20PM +0530, Maneesh Soni wrote:
[..]
Quoted 4 lines
o The following patch fixes the race involved between unregistering a kobject and simultaneously opeing a corresponding attribute file in sysfs.
Quoted 8 lines
Ideally sysfs should take a ref. to the kobject as long as it has dentries referring to the kobject, but because of current limitations in module/kobject ref counting, sysfs's pinning of kobject leads to hang/delays in rmmod of certain modules. The patch uses a new flag to mark dentries as disconnected from a valid kobject in the process of unregistering the kobject. The ->open in sysfs is failed if it finds a disconnected dentry. Marking and checking for the flag is done under
dentry->d_inode->i_sem.
[..]
Hi Greg / Andrew,
This problem can be solved without using the d_flags as I did previously. Viro suggested to just check for DCACHE_UNHASHED in check_perm() and this also solves the race. Please see the following patch instead of the previous one.
Thanks
Maneesh
PS: As you might have seen sysfs-symlinks-fix.patch the sysfs_get_kobject() is
repeated in this patch also. So, once you have decided about the sysfs-syminks-fix.patch and this one, I can re-submit a combined one.
----------------------------------------------------------------------------
o The following patch fixes the race involved between unregistering a kobject and simultaneously opeing a corresponding attribute file in sysfs.
o Ideally sysfs should take a ref. to the kobject as long as it has dentries referring to the kobjects, but because of current limitations in module/kobject ref counting, sysfs's pinning of kobject leads to hang/delays in rmmod of certain modules. The patch checks for unhashed dentries in check_perm() while opening a sysfs file. If the dentry is still hashed then it goes ahead and takes the ref to kobject. This done under the per dentry lock. It does this in the inline routine sysfs_get_kobject(dentry).
fs/sysfs/bin.c | 2 +- fs/sysfs/file.c | 2 +- fs/sysfs/sysfs.h | 13 +++++++++++++ 3 files changed, 15 insertions(+), 2 deletions(-)
fs/sysfs/sysfs.h renamed from fs/sysfs/sysfs.h~sysfs-d_fsdata-race-fix-2 +13 -0
@@ -11,3 +11,16 @@ extern void sysfs_hash_and_remove(structextern int sysfs_create_subdir (struct kobject *,const char *,struct dentry **);extern void sysfs_remove_subdir (struct dentry *); + + +static inline struct kobject *sysfs_get_kobject (struct dentry * dentry) +{ +struct kobject * kobj =NULL ; + +spin_lock (&dentry->d_lock); +if (!d_unhashed (dentry)) + kobj =kobject_get (dentry->d_fsdata); +spin_unlock (&dentry->d_lock); + +return kobj; +}
fs/sysfs/file.c renamed from fs/sysfs/file.c~sysfs-d_fsdata-race-fix-2 +1 -1
@@ -238,7 +238,7 @@ sysfs_write_file(struct file *file, consstatic int check_perm (struct inode * inode,struct file * file) { -struct kobject * kobj =kobject_get (file->f_dentry->d_parent->d_fsdata); +struct kobject * kobj =sysfs_get_kobject (file->f_dentry->d_parent);struct attribute * attr = file->f_dentry->d_fsdata;struct sysfs_buffer * buffer;struct sysfs_ops * ops =NULL ;
fs/sysfs/bin.c renamed from fs/sysfs/bin.c~sysfs-d_fsdata-race-fix-2 +1 -1
@@ -94,7 +94,7 @@ static ssize_t write(struct file * file,static int open (struct inode * inode,struct file * file) { -struct kobject * kobj =kobject_get (file->f_dentry->d_parent->d_fsdata); +struct kobject * kobj =sysfs_get_kobject (file->f_dentry->d_parent);struct bin_attribute * attr = file->f_dentry->d_fsdata;int error = -EINVAL ;
_
Signature · 6 lines
Maneesh Soni Linux Technology Center, IBM Software Lab, Bangalore, India email: [email protected] Phone: 91-80-25044999 Fax: 91-80-25268553 T/L : 9243696
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
Quoted 14 lines · Maneesh Soni
On Tue, Apr 06, 2004 at 03:43:20PM +0530, Maneesh Soni wrote:
[..]Quoted 4 lines
o The following patch fixes the race involved between unregistering a kobject and simultaneously opeing a corresponding attribute file in sysfs.
Quoted 8 lines
Ideally sysfs should take a ref. to the kobject as long as it has dentries referring to the kobject, but because of current limitations in module/kobject ref counting, sysfs's pinning of kobject leads to hang/delays in rmmod of certain modules. The patch uses a new flag to mark dentries as disconnected from a valid kobject in the process of unregistering the kobject. The ->open in sysfs is failed if it finds a disconnected dentry. Marking and checking for the flag is done under
dentry->d_inode->i_sem.
Quoted 8 lines
[..]
Hi Greg / Andrew,
This problem can be solved without using the
d_flagsas I did previously. Viro suggested to just check forDCACHE_UNHASHEDincheck_perm()and this also solves the race. Please see the following patch instead of the previous one.
This patch looks sane, Andrew, can you let it sit in your -mm tree for a while to see if anything breaks with it?
thanks,
greg k-h
----------------------------------------------------------------------------
o The following patch fixes the race involved between unregistering a kobject and simultaneously opeing a corresponding attribute file in sysfs.
o Ideally sysfs should take a ref. to the kobject as long as it has dentries referring to the kobjects, but because of current limitations in module/kobject ref counting, sysfs's pinning of kobject leads to hang/delays in rmmod of certain modules. The patch checks for unhashed dentries in check_perm() while opening a sysfs file. If the dentry is still hashed then it goes ahead and takes the ref to kobject. This done under the per dentry lock. It does this in the inline routine sysfs_get_kobject(dentry).
fs/sysfs/bin.c | 2 +- fs/sysfs/file.c | 2 +- fs/sysfs/sysfs.h | 13 +++++++++++++ 3 files changed, 15 insertions(+), 2 deletions(-)
fs/sysfs/sysfs.h renamed from fs/sysfs/sysfs.h~sysfs-d_fsdata-race-fix-2 +13 -0
@@ -11,3 +11,16 @@ extern void sysfs_hash_and_remove(structextern int sysfs_create_subdir (struct kobject *,const char *,struct dentry **);extern void sysfs_remove_subdir (struct dentry *); + + +static inline struct kobject *sysfs_get_kobject (struct dentry * dentry) +{ +struct kobject * kobj =NULL ; + +spin_lock (&dentry->d_lock); +if (!d_unhashed (dentry)) + kobj =kobject_get (dentry->d_fsdata); +spin_unlock (&dentry->d_lock); + +return kobj; +}
fs/sysfs/file.c renamed from fs/sysfs/file.c~sysfs-d_fsdata-race-fix-2 +1 -1
@@ -238,7 +238,7 @@ sysfs_write_file(struct file *file, consstatic int check_perm (struct inode * inode,struct file * file) { -struct kobject * kobj =kobject_get (file->f_dentry->d_parent->d_fsdata); +struct kobject * kobj =sysfs_get_kobject (file->f_dentry->d_parent);struct attribute * attr = file->f_dentry->d_fsdata;struct sysfs_buffer * buffer;struct sysfs_ops * ops =NULL ;
fs/sysfs/bin.c renamed from fs/sysfs/bin.c~sysfs-d_fsdata-race-fix-2 +1 -1
@@ -94,7 +94,7 @@ static ssize_t write(struct file * file,static int open (struct inode * inode,struct file * file) { -struct kobject * kobj =kobject_get (file->f_dentry->d_parent->d_fsdata); +struct kobject * kobj =sysfs_get_kobject (file->f_dentry->d_parent);struct bin_attribute * attr = file->f_dentry->d_fsdata;int error = -EINVAL ;
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
Quoted 3 lines · Greg KH
This patch looks sane, Andrew, can you let it sit in your -mm tree for a while to see if anything breaks with it?
It needs work to make it live alongside ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5/2.6.5-mm6/broken-out/sysfs-d_fsdata-race-fix-2.patch
What are we doing with sysfs-d_fsdata-race-fix-2 btw?
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
Quoted 8 lines · Andrew Morton
Quoted 3 lines · Greg KH
This patch looks sane, Andrew, can you let it sit in your -mm tree for a while to see if anything breaks with it?
It needs work to make it live alongside ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5/2.6.5-mm6/broken-out/sysfs-d_fsdata-race-fix-2.patch
What are we doing with sysfs-d_fsdata-race-fix-2 btw?
Hi Andrew,
Greg, is talking for the same patch, which is included in -mm6 now.
Maneesh
Signature · 6 lines
Maneesh Soni Linux Technology Center, IBM Software Lab, Bangalore, India email: [email protected] Phone: 91-80-25044999 Fax: 91-80-25268553 T/L : 9243696
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
In reply to Benjamin HerrenschmidtQuoted 14 lines · Benjamin Herrenschmidt
Quoted 12 lines · Greg KH
Hi,
The patch below backs out Maneesh's sysfs patch that was recently added to the kernel. In its defense, the original patch did solve some fixes that could be duplicated on SMP machines, but the side affect of the patch caused lots of problems. Basically it caused kobjects to get their references incremented when files that are not present in the kobject are asked for (udev can easily trigger this when it looks for files call "dev" in directories that do not have that file). This can cause easy oopses when the VFS later ages out those old dentries and the kobject has its reference finally released (usually after the module that the kobject lived in was removed.)
Quoted 6 lines
I think that the bug in the first place is to have an existing kobject that didn't bump the module ref count.
If a kobject exists that have a pointer to the module code (the release function), it _MUST_ have bumped the module ref count, that's the whole point of the module reference count.
But that is impossible as has already been pointed out by Alan Stern. If a module creates a kobject, how can the module_exit() function ever be called if that kobject incremented the module reference count?
So what we do is any reference to the kobject grabbed by userspace causes the module reference count to go up. That fixes the issue for the most part (with the exception of the race that Maneesh has documented.)
thanks,
greg k-h
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
Quoted 4 lines
But that is impossible as has already been pointed out by Alan Stern. If a module creates a kobject, how can the
module_exit()function ever be called if that kobject incremented the module reference count?
I just had a loooong discussion with Rusty on that subject, it's indeed a nasty one. The problem is that the real solution is to change the module unload semantics. Regardless of the count, module exit should be called, and the actual unload (and eventually calling an additional module "release" function) then should only happen once the count is down to 0. That means that rmmod would block forever if the driver is opened, but that is just something that needs to be known.
But that's not something we'll do for 2.6. For that to work, it also need various subsystem unregister_* (netdev etc...) functions to not error when the device is opened, just prevent new opens, and operate asynchronously (freeing data structures on kobject release) etc...
Quoted 4 lines
So what we do is any reference to the kobject grabbed by userspace causes the module reference count to go up. That fixes the issue for the most part (with the exception of the race that Maneesh has documented.)
Well.... Maybe, I still think it's dodgy, but probably fine for 2.6
Ben.
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
Quoted 13 lines · Benjamin Herrenschmidt
Quoted 3 lines
But that is impossible as has already been pointed out by Alan Stern. If a module creates a kobject, how can the
module_exit()function ever be called if that kobject incremented the module reference count?I just had a loooong discussion with Rusty on that subject, it's indeed a nasty one. The problem is that the real solution is to change the module unload semantics. Regardless of the count, module exit should be called, and the actual unload (and eventually calling an additional module "release" function) then should only happen once the count is down to 0. That means that rmmod would block forever if the driver is opened, but that is just something that needs to be known.
Quoted 4 lines
But that's not something we'll do for 2.6. For that to work, it also need various subsystem unregister_* (netdev etc...) functions to not error when the device is opened, just prevent new opens, and operate asynchronously (freeing data structures on kobject release) etc...
There was another lengthy discussion about this back in January. Read these threads:
http://marc.theaimsgroup.com/?t=107487731800002&r=1&w=2
http://marc.theaimsgroup.com/?t=107509479200002&r=1&w=2
Lots of back-and-forth, but Linus was basically against the idea. For now at least.
Alan Stern
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
In reply to Benjamin HerrenschmidtQuoted 7 lines · Benjamin Herrenschmidt
Quoted 4 lines
But that is impossible as has already been pointed out by Alan Stern. If a module creates a kobject, how can the
module_exit()function ever be called if that kobject incremented the module reference count?I just had a loooong discussion with Rusty on that subject, it's indeed a nasty one.
Ah, he sucks another person into the "module unload" discussion. My sympathies :)
Quoted 8 lines
The problem is that the real solution is to
change the module unload semantics. Regardless of the count, module exit should be called, and the actual unload (and eventually calling an additional module "release" function) then should only happen once the count is down to 0. That means that rmmod would block forever if the driver is opened, but that is just something that needs to be known.
Quoted 4 lines
But that's not something we'll do for 2.6. For that to work, it also need various subsystem unregister_* (netdev etc...) functions to not error when the device is opened, just prevent new opens, and operate asynchronously (freeing data structures on kobject release) etc...
I agree, it's a difficult problem to solve, and something we aren't going to do for 2.6. That's one reason why module unload is its own config option :)
Anyway, Rusty's proposal of "never unload, just mark not used" and then load a new copy into memory that he did during the OLS timeframe last year sounds like the only sane way to get this completely correct, with the trade off of never releasing memory.
thanks,
greg k-h
Re: [linux-usb-devel] [PATCH] back out sysfs reference count change
Quoted 6 lines
Quoted 3 lines
I just had a loooong discussion with Rusty on that subject, it's indeed a nasty one.
Ah, he sucks another person into the "module unload" discussion. My sympathies :)
Nah, I started it, since he is about 2 meters from me in the office, that isn't difficult :)
Quoted 8 lines
I agree, it's a difficult problem to solve, and something we aren't going to do for 2.6. That's one reason why module unload is its own config option :)
Anyway, Rusty's proposal of "never unload, just mark not used" and then load a new copy into memory that he did during the OLS timeframe last year sounds like the only sane way to get this completely correct, with the trade off of never releasing memory.