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@xxxxxxxxxx>
> >>---
> >>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?
> >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 think we shouldn't need XPT_SHARE_SOCK if we just get the reference counting
on the svc_xprt right instead.
>
> >
> >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?
--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@xxxxxxxxxxxxxxx
> >>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@xxxxxxxxxxxxxxx
> >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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
