On Wed, 2025-01-29 at 09:22 -0500, Chuck Lever wrote: > On 1/29/25 8:39 AM, Jeff Layton wrote: > > While looking over the CB_SEQUENCE error handling, I discovered that > > callbacks don't hold a reference to a session, and the > > clp->cl_cb_session could easily change between request and response. > > If that happens at an inopportune time, there could be UAFs or weird > > slot/sequence handling problems. > > > > This series changes the nfsd4_session to be RCU-freed, and then adds a > > new method of session refcounting that is compatible with the old. > > nfsd4_callback RPCs will now hold a lightweight reference to the session > > in addition to the slot. Then, all of the callback handling is switched > > to use that session instead of dereferencing clp->cb_cb_session. > > I've also reworked the error handling in nfsd4_cb_sequence_done() > > based on review comments, and lifted the v4.0 handing out of that > > function. > > > > This passes pynfs, nfstests, and fstests for me, but I'm not sure how > > much any of that stresses the backchannel's error handling. > > > > These should probably go in via Chuck's tree, but the last patch touches > > some NFS cnd sunrpc client code, so it'd be good to have R-b's or A-b's > > from Trond and/or Anna on that one. > > A few initial reactions as I get to know this new revision. > > - I have no objection to 7/7, but it does seem a bit out of place in > this series. Maybe hold it back and send it separately after this > series goes in? > > - The fact that the session can be replaced while a callback operation > is pending suggests that, IIUC, decode_cb_sequence() sanity checking > will fail in such cases, and it's not because of a bug in the client's > callback server. Or maybe I'm overthinking it - that is exactly what > you are trying to prevent? > That's the best-case scenario, but callbacks can run at any time. If this happens at the wrong time this could crash or cause more subtle problems than just a spurious ESERVERFAULT. IOW, we could pass decode_cb_sequence(), then the pointer changes and then nfsd4_cb_sequence_done() ends up working with a different session. > - In 1/7, the kdoc comment for "get" should enumerate the return values > and their meanings. > Ack > - cb_session_changed => nfsd4_cb_session_changed. > Ack > - I'm still not convinced it's wise to bump the slot number in the > ESERVERFAULT case. > It's debatable for sure. The client _did_ respond with NFS4_OK in this case, but it is a bit sketchy since something else didn't match. I'm fine with removing that seq bump if you prefer. > - IMO the cb_sequence_done rework should rename "need_restart" to > "need_requeue" or just "requeue" -- there is a call to > rpc_restart_call_prepare() here that is a little confusing and > could do with some disambiguation. > Good point. I'll change that too. Thanks for the review! > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> > > --- > > Changes in v2: > > - make nfsd4_session be RCU-freed > > - change code to keep reference to session over callback RPCs > > - rework error handling in nfsd4_cb_sequence_done() > > - move NFSv4.0 handling out of nfsd4_cb_sequence_done() > > - Link to v1: https://lore.kernel.org/r/20250123-nfsd-6-14-v1-0-c1137a4fa2ae@xxxxxxxxxx > > > > --- > > Jeff Layton (7): > > nfsd: add routines to get/put session references for callbacks > > nfsd: make clp->cl_cb_session be an RCU managed pointer > > nfsd: add a cb_ses pointer to nfsd4_callback and use it instead of clp->cb_cb_session > > nfsd: overhaul CB_SEQUENCE error handling > > nfsd: remove unneeded forward declaration of nfsd4_mark_cb_fault() > > nfsd: lift NFSv4.0 handling out of nfsd4_cb_sequence_done() > > sunrpc: make rpc_restart_call() and rpc_restart_call_prepare() void return > > > > fs/nfs/nfs4proc.c | 12 ++- > > fs/nfsd/nfs4callback.c | 212 ++++++++++++++++++++++++++++++++------------ > > fs/nfsd/nfs4state.c | 43 ++++++++- > > fs/nfsd/state.h | 6 +- > > fs/nfsd/trace.h | 6 +- > > include/linux/sunrpc/clnt.h | 4 +- > > net/sunrpc/clnt.c | 7 +- > > 7 files changed, 210 insertions(+), 80 deletions(-) > > --- > > base-commit: a05af3c6103b703d1d38d8180b3ebbe0a03c2f07 > > change-id: 20250123-nfsd-6-14-b0797e385dc0 > > > > Best regards, > > -- Jeff Layton <jlayton@xxxxxxxxxx>