[nfs, rpc] crap with refcounting and rmmod races

[nfs, rpc] crap with refcounting and rmmod races

[Al Viro]

	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()?

[nfs, rpc] crap with refcounting and rmmod races

[Al Viro]

	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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [nfs, rpc] crap with refcounting and rmmod races

[Al Viro]

On Wed, Jun 22, 2011 at 08:12:40PM +0100, Al Viro wrote:

> 	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()?

While we are at it, is ->rpc_release() allowed to trigger rpc_shutdown_client()
on out ->tk_client?

Re: [nfs, rpc] crap with refcounting and rmmod races

[Al Viro]

On Wed, Jun 22, 2011 at 08:12:40PM +0100, Al Viro wrote:

> 	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()?

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [nfs, rpc] crap with refcounting and rmmod races

[Al Viro]

On Wed, Jun 22, 2011 at 08:12:40PM +0100, Al Viro wrote:

> 	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()?

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...

Re: [nfs, rpc] crap with refcounting and rmmod races

[Al Viro]

On Wed, Jun 22, 2011 at 08:12:40PM +0100, Al Viro wrote:

> 	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()?

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [nfs, rpc] crap with refcounting and rmmod races

[Trond Myklebust]

On Wed, 2011-06-22 at 22:51 +0100, Al Viro wrote: 
> On Wed, Jun 22, 2011 at 08:12:40PM +0100, Al Viro wrote:
> 
> > 	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()?
> 
> 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

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.

> 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...

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.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [nfs, rpc] crap with refcounting and rmmod races

[Al Viro]

On Wed, Jun 22, 2011 at 06:48:54PM -0400, Trond Myklebust wrote:

> 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.

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).

Re: [nfs, rpc] crap with refcounting and rmmod races

[Trond Myklebust]

On Wed, 2011-06-22 at 23:58 +0100, Al Viro wrote: 
> On Wed, Jun 22, 2011 at 06:48:54PM -0400, Trond Myklebust wrote:
> 
> > 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.
> 
> 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).

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...

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com