Re: [PATCH_V3 1/7] NFS dont free shared socket on backchannel put xprt

["J. Bruce Fields" <bfields@fieldses.org>]

On Tue, Dec 14, 2010 at 05:28:51PM -0500, Andy Adamson wrote:
> 
> On Dec 14, 2010, at 4:56 PM, J. Bruce Fields wrote:
> 
> >On Tue, Dec 14, 2010 at 04:44:58PM -0500, Andy Adamson wrote:
> >>
> >>On Dec 14, 2010, at 1:19 PM, J. Bruce Fields wrote:
> >>
> >>>On Mon, Dec 13, 2010 at 03:19:39PM -0500, Andy Adamson wrote:
> >>>>Fixes this bug:
> >>>>fedora-64 kernel: Invoking bc_svc_procass()
> >>>>fedora-64 kernel: nfs_callback_authenticate SVC_DROP
> >>>>fedora-64 kernel: BUG: unable to handle kernel NULL pointer
> >>>>dereference at 0000000000000018 IP: [<ffffffffa0156140>]
> >>>>svc_sock_free+0x32/0x56 [sunrpc]
> >>>>
> >>>>Signed-off-by: Andy Adamson <andros@netapp.com>
> >>>>---
> >>>>fs/nfs/callback.c               |    3 +++
> >>>>include/linux/sunrpc/svc_xprt.h |    1 +
> >>>>net/sunrpc/svc_xprt.c           |    3 ++-
> >>>>3 files changed, 6 insertions(+), 1 deletions(-)
> >>>>
> >>>>diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> >>>>index 93a8b3b..023a9eb 100644
> >>>>--- a/fs/nfs/callback.c
> >>>>+++ b/fs/nfs/callback.c
> >>>>@@ -193,6 +193,9 @@ nfs41_callback_up(struct svc_serv *serv,
> >>>>struct rpc_xprt *xprt)
> >>>>	serv->bc_xprt = bc_xprt;
> >>>>	xprt->bc_serv = serv;
> >>>>
> >>>>+	/* socket is shared with the fore channel */
> >>>>+	set_bit(XPT_SHARE_SOCK, &bc_xprt->xpt_flags);
> >>>>+
> >>>>	INIT_LIST_HEAD(&serv->sv_cb_list);
> >>>>	spin_lock_init(&serv->sv_cb_lock);
> >>>>	init_waitqueue_head(&serv->sv_cb_waitq);
> >>>>diff --git a/include/linux/sunrpc/svc_xprt.h
> >>>>b/include/linux/sunrpc/svc_xprt.h
> >>>>index aea0d43..600c669 100644
> >>>>--- a/include/linux/sunrpc/svc_xprt.h
> >>>>+++ b/include/linux/sunrpc/svc_xprt.h
> >>>>@@ -62,6 +62,7 @@ struct svc_xprt {
> >>>>#define	XPT_DETACHED	10		/* detached from tempsocks list */
> >>>>#define XPT_LISTENER	11		/* listening endpoint */
> >>>>#define XPT_CACHE_AUTH	12		/* cache auth info */
> >>>>+#define XPT_SHARE_SOCK	13		/* fore and back channel share
> >>>>socket */
> >>>>
> >>>>	struct svc_pool		*xpt_pool;	/* current pool iff queued */
> >>>>	struct svc_serv		*xpt_server;	/* service for transport */
> >>>>diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> >>>>index ea2ff78..8c4d9ad 100644
> >>>>--- a/net/sunrpc/svc_xprt.c
> >>>>+++ b/net/sunrpc/svc_xprt.c
> >>>>@@ -128,7 +128,8 @@ static void svc_xprt_free(struct kref *kref)
> >>>>	if (test_bit(XPT_CACHE_AUTH, &xprt->xpt_flags))
> >>>>		svcauth_unix_info_release(xprt);
> >>>>	put_net(xprt->xpt_net);
> >>>>-	xprt->xpt_ops->xpo_free(xprt);
> >>>>+	if (!test_bit(XPT_SHARE_SOCK, &xprt->xpt_flags))
> >>>>+		xprt->xpt_ops->xpo_free(xprt);
> >>>
> >>>So when does the svc_xprt get freed if not here?
> >>
> >>svc_sock_destroy frees the bc_xprt, called by svc_destroy on the
> >>serv->bc_xprt.
> >
> >Can you remove the
> >
> >	#if defined(CONFIG_NFS_V4_1)
> >       	svc_sock_destroy(serv->bc_xprt);
> >	#endif /* CONFIG_NFS_V4_1 */
> >
> >from svc_destroy instead?
> 
> Instead of what?

Instead of the patch above.

--b.

> Where do you want svc_sock_destroy to be called?

> From svc_xprt_free?
> 
> >
> >>>And why is it OK to do
> >>>the put_net() and module_put() but not the xpo_free()?
> >>
> >>svc_sock_create()  called only by nfs41_callback_up creates the
> >>bc_xprt. It calls svc_xprt_init which calls get_net(), so put_net is
> >>needed.
> >>
> >>Hmmm. It looks like the module_put is also not needed - the matching
> >>module_get is in svc_create_xprt() which is not called for the
> >>NFSv4.1 callback service. (but is called for the NFSv4.0 callback
> >>service ... sheese).
> >>
> >>I'll move the module_put so it is also not called when the
> >>XPT_SHARE_SOCK flag is set.
> >
> >The xpt_net reference counting doesn't look right either.
> 
> I'm not familiar with the xpt_net reference counting.
> 
> >
> >I think we shouldn't need XPT_SHARE_SOCK if we just get the
> >reference counting
> >on the svc_xprt right instead.
> 
> At the end of the day svc_xprt_free will be called on this svc_xprt.
> At that time, xpo_free can not be called because xpo_create was not
> used to create it. svc_sock_destroy needs to be called to free the
> storage kzalloc'ed in svc_sock_create.
> 
> I don't get what you are suggesting.....
> 
> >
> >>
> >>>
> >>>Something feels wrong here....
> >>
> >>Yeah - how about naming the server back channel xprt (bc_xprt) the
> >>same as the client back channel xprt (bc_xprt)!!! How confusing is
> >>THAT! :)
> >
> >Well, and it's also the name of a field in svc_serv.  If nothing
> >else, would
> >you mind renaming that to sv_bc_xprt?
> 
> I can clean up the naming after we figure out the above.
> 
> -->Andy
> 
> >
> >--b.
> >
> >>
> >>Thanks for the review.
> >>
> >>-->Andy
> >>
> >>>
> >>>--b.
> >>>
> >>>>	module_put(owner);
> >>>>}
> >>>>
> >>>>-- 
> >>>>1.6.6
> >>>>
> >>>>--
> >>>>To unsubscribe from this list: send the line "unsubscribe linux-
> >>>>nfs" in
> >>>>the body of a message to majordomo@vger.kernel.org
> >>>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>--
> >>>To unsubscribe from this list: send the line "unsubscribe
> >>>linux-nfs" in
> >>>the body of a message to majordomo@vger.kernel.org
> >>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
>