crap with refcounting and rmmod races
Subsystems (3)
There's something seriously screwed up with nfs4_closedata and
path_get()/path_put() in nfs4_do_close()/nfs4_free_closedata().
Look: either we never call the latter before all preexisting
references to data->path.mnt are dropped, in which case we don't need to grab/put the damn thing at all. *OR* it is possible, in which case that data->path.mnt might be the only thing that still holds nfs.ko pinned down and right after the path_put() we might be running code in a module with refcount 0. Which is not a good thing...
Note that extra references to vfsmount do not prevent umount from
removing the sucker from the tree and dropping the preexisting reference to it. umount -l will do that just fine.
This thing is called as ->rpc_release(); do we have anything
protecting the issuer of rpc_run_task() from being rmmod'ed before (or during) the call of ->rpc_release()?
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 20110622191239.GD11521-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org
Quoted 7 lines · Al Viro
Note that extra references to vfsmount do not prevent umount from
removing the sucker from the tree and dropping the preexisting reference to it. umount -l will do that just fine.This thing is called as ->rpc_release(); do we have anythingprotecting the issuer of
rpc_run_task()from being rmmod'ed before (or during) the call of ->rpc_release()?
While we are at it, is ->rpc_release() allowed to trigger rpc_shutdown_client() on out ->tk_client?
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 7 lines · Al Viro
Note that extra references to vfsmount do not prevent umount from
removing the sucker from the tree and dropping the preexisting reference to it. umount -l will do that just fine.This thing is called as ->rpc_release(); do we have anythingprotecting the issuer of
rpc_run_task()from being rmmod'ed before (or during) the call of ->rpc_release()?
Ah, I see... All these suckers end up running from rpc_task_free(), scheduled via nfsiod_workqueue. Reference to vfsmount or superblock (in case of async_unlink; async close should also use nfs_sb_{,de}active() instead of playing with vfsmounts, BTW) pins the module down until we are into the ->rpc_release() in question. And rmmod nfd done after that point will wait in destroy_workqueue() called by nfsiod_stop().
Subtle and worth documenting explicitly, IMO...
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 6 lines · Al Viro
Quoted 4 lines · Al Viro
Note that extra references to vfsmount do not prevent umount from
removing the sucker from the tree and dropping the preexisting reference to it. umount -l will do that just fine.
Quoted 8 lines
Quoted 3 lines
This thing is called as ->rpc_release(); do we have anythingprotecting the issuer of
rpc_run_task()from being rmmod'ed before (or during) the call of ->rpc_release()?Ah, I see... All these suckers end up running from
rpc_task_free(), scheduled vianfsiod_workqueue. Reference to vfsmount or superblock (in case ofasync_unlink; async close should also use nfs_sb_{,de}active() instead of playing with vfsmounts, BTW) pins the module down until we
It is fairly trivial to define a 'struct nfs_path' that takes a reference to the dentry and a reference to the super block if you'd prefer that we get rid of the 'struct path' in nfs_open_context.
Quoted 4 lines
are into the ->rpc_release() in question. And rmmod nfd done after that point will wait in
destroy_workqueue()called bynfsiod_stop().Subtle and worth documenting explicitly, IMO...
The same thing is true for the sunrpc module: the rmmod sunrpc will wait in the destroy_workqueue() for rpciod until all the remaining asynchronous rpc tasks are done. That is documented in the changelog, at least, but I agree that we can add a few comments in the code too.
Signature · 6 lines
Trond Myklebust Linux NFS client maintainer NetApp [email protected] www.netapp.com
Al Viro
maintainerQuoted 3 lines · Trond Myklebust
It is fairly trivial to define a 'struct
nfs_path' that takes a reference to the dentry and a reference to the super block if you'd prefer that we get rid of the 'struct path' innfs_open_context.
Er... as opposed to struct dentry *dentry and its ->d_sb? ;-) See the last few patches in #untested (vfs-2.6.git) doing more or less that (and nfs4_closedata is just fine with data->inode->i_sb - it doesn't need to pin any dentries at all; nfs4_opendata and nfs_open_context need dentry, of course).
Quoted 11 lines · Al Viro
Quoted 3 lines · Trond Myklebust
It is fairly trivial to define a 'struct
nfs_path' that takes a reference to the dentry and a reference to the super block if you'd prefer that we get rid of the 'struct path' innfs_open_context.Er... as opposed to struct dentry *dentry and its ->d_sb? ;-) See the last few patches in #untested (vfs-2.6.git) doing more or less that (and
nfs4_closedatais just fine withdata->inode->i_sb- it doesn't need to pin any dentries at all;nfs4_opendataandnfs_open_contextneed dentry, of course).
Sure. As far as we're concerned, the main purpose of the vfsmount in nfs_open_context is to prevent triggering generic_shutdown_super() while we're finishing writebacks. It is convenient to carry that all the way through to the nfs4_closedata, but I agree that it is not necessary...
Signature · 6 lines
Trond Myklebust Linux NFS client maintainer NetApp [email protected] www.netapp.com