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:40:19PM -0500, Andy Adamson wrote:
> 
> On Dec 14, 2010, at 5:36 PM, J. Bruce Fields wrote:
> 
> >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.
> 
> Sorry, I just don't understand how that will solve the sock_free BUG
> above.

It may not, sorry, I'd need to look at it more closely.  Maybe you could
explain in more detail how the bug happens and why?  (Which pointer is
it that's null, and why?)

--b.