* Question about nfs4_destroy_session()
@ 2014-02-12 21:42 Paul E. McKenney
2014-02-12 21:55 ` Trond Myklebust
0 siblings, 1 reply; 3+ messages in thread
From: Paul E. McKenney @ 2014-02-12 21:42 UTC (permalink / raw)
To: trond.myklebust; +Cc: linux-nfs, linux-kernel
Hello, Trond,
In nfs4_destroy_session(), there is an rcu_dereference() that looks to
leak the returned pointer out of an RCU read-side critical section.
If the pointed-to object might have just now been created, this is a
bug because xprt_destroy_backchannel() dereferences this pointer.
So, does xprt_destroy_backchannel() exclude creation-side code? (If so,
no bug -- but a comment might be good.)
Thanx, Paul
void nfs4_destroy_session(struct nfs4_session *session)
{
struct rpc_xprt *xprt;
struct rpc_cred *cred;
cred = nfs4_get_clid_cred(session->clp);
nfs4_proc_destroy_session(session, cred);
if (cred)
put_rpccred(cred);
rcu_read_lock();
xprt = rcu_dereference(session->clp->cl_rpcclient->cl_xprt);
rcu_read_unlock();
dprintk("%s Destroy backchannel for xprt %p\n",
__func__, xprt);
xprt_destroy_backchannel(xprt, NFS41_BC_MIN_CALLBACKS);
nfs4_destroy_session_slot_tables(session);
kfree(session);
}
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: Question about nfs4_destroy_session() 2014-02-12 21:42 Question about nfs4_destroy_session() Paul E. McKenney @ 2014-02-12 21:55 ` Trond Myklebust 2014-02-12 22:07 ` Paul E. McKenney 0 siblings, 1 reply; 3+ messages in thread From: Trond Myklebust @ 2014-02-12 21:55 UTC (permalink / raw) To: paulmck; +Cc: linuxnfs, Linux Kernel Mailing List On Feb 12, 2014, at 16:42, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote: > Hello, Trond, > > In nfs4_destroy_session(), there is an rcu_dereference() that looks to > leak the returned pointer out of an RCU read-side critical section. > If the pointed-to object might have just now been created, this is a > bug because xprt_destroy_backchannel() dereferences this pointer. > > So, does xprt_destroy_backchannel() exclude creation-side code? (If so, > no bug -- but a comment might be good.) > > Thanx, Paul > > void nfs4_destroy_session(struct nfs4_session *session) > { > struct rpc_xprt *xprt; > struct rpc_cred *cred; > > cred = nfs4_get_clid_cred(session->clp); > nfs4_proc_destroy_session(session, cred); > if (cred) > put_rpccred(cred); > > rcu_read_lock(); > xprt = rcu_dereference(session->clp->cl_rpcclient->cl_xprt); > rcu_read_unlock(); > dprintk("%s Destroy backchannel for xprt %p\n", > __func__, xprt); > xprt_destroy_backchannel(xprt, NFS41_BC_MIN_CALLBACKS); > nfs4_destroy_session_slot_tables(session); > kfree(session); > } > Hi Paul, nfs4_destroy_session() is only called when we’re tearing down the struct nfs_client that owns the cl_rppcclient, and the associated cl_xprt, so the code above should be safe, despite being ugly. Is there a better annotation for use in the above kind of situation? Cheers, Trond _________________________________ Trond Myklebust Linux NFS client maintainer, PrimaryData trond.myklebust@primarydata.com ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Question about nfs4_destroy_session() 2014-02-12 21:55 ` Trond Myklebust @ 2014-02-12 22:07 ` Paul E. McKenney 0 siblings, 0 replies; 3+ messages in thread From: Paul E. McKenney @ 2014-02-12 22:07 UTC (permalink / raw) To: Trond Myklebust; +Cc: linuxnfs, Linux Kernel Mailing List On Wed, Feb 12, 2014 at 04:55:02PM -0500, Trond Myklebust wrote: > > On Feb 12, 2014, at 16:42, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote: > > > Hello, Trond, > > > > In nfs4_destroy_session(), there is an rcu_dereference() that looks to > > leak the returned pointer out of an RCU read-side critical section. > > If the pointed-to object might have just now been created, this is a > > bug because xprt_destroy_backchannel() dereferences this pointer. > > > > So, does xprt_destroy_backchannel() exclude creation-side code? (If so, > > no bug -- but a comment might be good.) > > > > Thanx, Paul > > > > void nfs4_destroy_session(struct nfs4_session *session) > > { > > struct rpc_xprt *xprt; > > struct rpc_cred *cred; > > > > cred = nfs4_get_clid_cred(session->clp); > > nfs4_proc_destroy_session(session, cred); > > if (cred) > > put_rpccred(cred); > > > > rcu_read_lock(); > > xprt = rcu_dereference(session->clp->cl_rpcclient->cl_xprt); > > rcu_read_unlock(); > > dprintk("%s Destroy backchannel for xprt %p\n", > > __func__, xprt); > > xprt_destroy_backchannel(xprt, NFS41_BC_MIN_CALLBACKS); > > nfs4_destroy_session_slot_tables(session); > > kfree(session); > > } > > > > Hi Paul, > > nfs4_destroy_session() is only called when we’re tearing down the struct nfs_client that owns the cl_rppcclient, and the associated cl_xprt, so the code above should be safe, despite being ugly. > > Is there a better annotation for use in the above kind of situation? One approach would be to add a comment on the rcu_dereference() stating that creation-side code is excluded, e.g., via locking or by the data structures no longer being accessible. Another approach would be to move the rcu_read_unlock() to follow the xprt_destroy_backchannel(), assuming none of the code that would be pulled into the RCU read-side critical section can block. The second approach would prevent false positives from the RCU pointer leak detectors that are being worked on. Thanx, Paul ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-02-12 22:08 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-12 21:42 Question about nfs4_destroy_session() Paul E. McKenney 2014-02-12 21:55 ` Trond Myklebust 2014-02-12 22:07 ` Paul E. McKenney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox