| Message ID | 20250123-nfsd-6-14-v1-1-c1137a4fa2ae@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | nfsd: CB_SEQUENCE error handling fixes and cleanups | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 1/23/25 3:25 PM, Jeff Layton wrote: > This is problematic, since the RPC might have been entirely successful. > There is no point in restarting a v4.1+ callback just because > RPC_SIGNALLED is true. The v4.1+ error handling has other mechanisms for > detecting when it should retransmit the RPC. > > Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors") > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4callback.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > index 50e468bdb8d4838b5217346dcc2bd0fec1765c1a..e12205ef16ca932ffbcc86d67b0817aec2436c89 100644 > --- a/fs/nfsd/nfs4callback.c > +++ b/fs/nfsd/nfs4callback.c > @@ -1403,9 +1403,6 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > } > trace_nfsd_cb_free_slot(task, cb); > nfsd41_cb_release_slot(cb); > - > - if (RPC_SIGNALLED(task)) > - goto need_restart; > out: > return ret; > retry_nowait: > I too am skeptical about this logic, but I don't entirely understand it yet. More importantly, though, I don't recall seeing (mis)behavior that I can directly attribute to it, so I can't yet confirm or deny your assertion that "This is problematic". Before making a code change here, let's gather a little evidence of a real problem. For instance, we might want to replace this logic with something better rather than wholesale removing it. You might start by enabling aggressive disconnect injection to see how backchannel recovery works (or that it doesn't work!). I'm trying this on my kdevops NFSD while running fstests: cd /sys/kernel/debug/fail_sunrpc/ echo Y > ignore-cache-wait echo Y > ignore-client-disconnect echo 24847 > interval echo 97 > times echo 100 > probability
On Sat, 2025-01-25 at 11:24 -0500, Chuck Lever wrote: > On 1/23/25 3:25 PM, Jeff Layton wrote: > > This is problematic, since the RPC might have been entirely successful. > > There is no point in restarting a v4.1+ callback just because > > RPC_SIGNALLED is true. The v4.1+ error handling has other mechanisms for > > detecting when it should retransmit the RPC. > > > > Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors") > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/nfs4callback.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > index 50e468bdb8d4838b5217346dcc2bd0fec1765c1a..e12205ef16ca932ffbcc86d67b0817aec2436c89 100644 > > --- a/fs/nfsd/nfs4callback.c > > +++ b/fs/nfsd/nfs4callback.c > > @@ -1403,9 +1403,6 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > > } > > trace_nfsd_cb_free_slot(task, cb); > > nfsd41_cb_release_slot(cb); > > - > > - if (RPC_SIGNALLED(task)) > > - goto need_restart; > > out: > > return ret; > > retry_nowait: > > > > I too am skeptical about this logic, but I don't entirely understand it > yet. More importantly, though, I don't recall seeing (mis)behavior that > I can directly attribute to it, so I can't yet confirm or deny your > assertion that "This is problematic". > I haven't seen behavior that I can directly attribute to this either, but we have seen a number of strange panics and weird behaviors in the callback code over the years that may be related. At this point, I think you're correct that we will probably need to do more than just small, incremental changes here. > Before making a code change here, let's gather a little evidence of a > real problem. For instance, we might want to replace this logic with > something better rather than wholesale removing it. > > You might start by enabling aggressive disconnect injection to see how > backchannel recovery works (or that it doesn't work!). I'm trying this > on my kdevops NFSD while running fstests: > > cd /sys/kernel/debug/fail_sunrpc/ > echo Y > ignore-cache-wait > echo Y > ignore-client-disconnect > echo 24847 > interval > echo 97 > times > echo 100 > probability > > Unfortunately, I've found an even bigger problem in the callback code. It accesses the clp->cl_cb_session pointer when processing the call and reply, but that pointer doesn't imply a reference and nothing else ensures that the nfsd4_session object will stick around while this happens. I think a callback can race with a DESTROY_SESSION and cause a UAF. I started working on patches to fix this up, but it's a bit complex and will take some time. Please don't apply any of these until I get a better picture of what will need to be changed. Stay tuned!
On Fri, 24 Jan 2025, Jeff Layton wrote: > This is problematic, since the RPC might have been entirely successful. > There is no point in restarting a v4.1+ callback just because > RPC_SIGNALLED is true. The v4.1+ error handling has other mechanisms for > detecting when it should retransmit the RPC. But why might RPC_SIGNALLED() ever be true? The flag RPC_TASK_SIGNALLED is only ever set by rpc_signal_task() which is only called from rpc_killall_tasks() and __rpc_execute() for non-async tasks which doesn't apply to nfsd callbacks as they are started with rpc_call_async(). rpc_killall_tasks() is called by fs/nfs/ which isn't relevant for us, and from rpc_shutdown_client(). In those cases we certainly don't want the request to be retried, though the nfsd4_process_cb_update() case is a little interesting as we do want it to be retried, but in a different client. So the code you are removing is either dead code because something else will prevent the restart when a client is being shut down, or it is bad code because it would delay rpc_shutdown_client() while the request is retried. I haven't dug the extra step to figure out which, but either way I think the code should go. NeilBrown > > Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors") > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4callback.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > index 50e468bdb8d4838b5217346dcc2bd0fec1765c1a..e12205ef16ca932ffbcc86d67b0817aec2436c89 100644 > --- a/fs/nfsd/nfs4callback.c > +++ b/fs/nfsd/nfs4callback.c > @@ -1403,9 +1403,6 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > } > trace_nfsd_cb_free_slot(task, cb); > nfsd41_cb_release_slot(cb); > - > - if (RPC_SIGNALLED(task)) > - goto need_restart; > out: > return ret; > retry_nowait: > > -- > 2.48.1 > >
On Sun, 2025-01-26 at 10:01 +1100, NeilBrown wrote: > On Fri, 24 Jan 2025, Jeff Layton wrote: > > This is problematic, since the RPC might have been entirely successful. > > There is no point in restarting a v4.1+ callback just because > > RPC_SIGNALLED is true. The v4.1+ error handling has other mechanisms for > > detecting when it should retransmit the RPC. > > But why might RPC_SIGNALLED() ever be true? > The flag RPC_TASK_SIGNALLED is only ever set by rpc_signal_task() which > is only called from rpc_killall_tasks() and __rpc_execute() for > non-async tasks which doesn't apply to nfsd callbacks as they are > started with rpc_call_async(). > > rpc_killall_tasks() is called by fs/nfs/ which isn't relevant for us, > and from rpc_shutdown_client(). In those cases we certainly don't want > the request to be retried, though the nfsd4_process_cb_update() case is > a little interesting as we do want it to be retried, but in a different > client. > > So the code you are removing is either dead code because something else > will prevent the restart when a client is being shut down, or it is bad > code because it would delay rpc_shutdown_client() while the request is > retried. > > I haven't dug the extra step to figure out which, but either way I think > the code should go. > > Thanks. That was my analysis too. rpc_shutdown_client() is called when we tear down and rebuild the rpc_client. nfsd does this in setup_callback_client(), which gets called from nfsd4_process_cb_update() (basically when we detect that the backchannel is having problems). There are really only two states: We either got a reply from the server before the client went down, or we didn't. In the case where we got a reply, there is no need to retry anything. In the case where we didn't, the tk_status will be '1', so there is no need to check RPC_SIGNALLED() at all here. The existing code could lead to the call being retried when we had already gotten a perfectly valid reply. > > > > Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors") > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/nfs4callback.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > index 50e468bdb8d4838b5217346dcc2bd0fec1765c1a..e12205ef16ca932ffbcc86d67b0817aec2436c89 100644 > > --- a/fs/nfsd/nfs4callback.c > > +++ b/fs/nfsd/nfs4callback.c > > @@ -1403,9 +1403,6 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback > > } > > trace_nfsd_cb_free_slot(task, cb); > > nfsd41_cb_release_slot(cb); > > - > > - if (RPC_SIGNALLED(task)) > > - goto need_restart; > > out: > > return ret; > > retry_nowait: > > > > -- > > 2.48.1 > > > > >
On 1/26/25 6:18 AM, Jeff Layton wrote: > On Sun, 2025-01-26 at 10:01 +1100, NeilBrown wrote: >> On Fri, 24 Jan 2025, Jeff Layton wrote: >>> This is problematic, since the RPC might have been entirely successful. >>> There is no point in restarting a v4.1+ callback just because >>> RPC_SIGNALLED is true. The v4.1+ error handling has other mechanisms for >>> detecting when it should retransmit the RPC. >> >> But why might RPC_SIGNALLED() ever be true? >> The flag RPC_TASK_SIGNALLED is only ever set by rpc_signal_task() which >> is only called from rpc_killall_tasks() and __rpc_execute() for >> non-async tasks which doesn't apply to nfsd callbacks as they are >> started with rpc_call_async(). >> >> rpc_killall_tasks() is called by fs/nfs/ which isn't relevant for us, >> and from rpc_shutdown_client(). In those cases we certainly don't want >> the request to be retried, though the nfsd4_process_cb_update() case is >> a little interesting as we do want it to be retried, but in a different >> client. >> >> So the code you are removing is either dead code because something else >> will prevent the restart when a client is being shut down, or it is bad >> code because it would delay rpc_shutdown_client() while the request is >> retried. >> >> I haven't dug the extra step to figure out which, but either way I think >> the code should go. > > Thanks. That was my analysis too. Agreed, this code is problematic, but it appears to me that some of these problems are not resolved by simply removing the signal check. > rpc_shutdown_client() is called when we tear down and rebuild the > rpc_client. nfsd does this in setup_callback_client(), which gets > called from nfsd4_process_cb_update() (basically when we detect that > the backchannel is having problems). > > There are really only two states: We either got a reply from the server > before the client went down, or we didn't. In the case where we got a > reply, there is no need to retry anything. In the case where we didn't, > the tk_status will be '1', so there is no need to check RPC_SIGNALLED() > at all here. Fwiw, the "cb_seq_status == 1" arm skips the signal check in the current code. > The existing code could lead to the call being retried when we had > already gotten a perfectly valid reply. Here's a case-by-case audit: - NFS_OK: SEQUENCE was decoded and passed sanity checks. So this should not ever requeue in here. It might be requeued during subsequent processing. - ESERVERFAULT: SEQUENCE was decoded but failed sanity checking. The reply should be dropped now, and the session marked FAULT. No requeue is ever needed here. [ I question whether the sequence number should be bumped in this case -- the client's callback server replied with the identity of some other slot. And anyway, this session is about to become toast. ] - 1: The timeout case. We want a fresh session here, so it falls through to BADSESSION. - NFS4ERR_BADSESSION: This needs a requeue so that the operation can be retried with a fresh session. But it should always check if the rpc_clnt is shutting down before doing so. This is a problem in the current code. - NFS4ERR_DELAY: Skips the signal check, but shouldn't. If the rpc_clnt is shutting down, this RPC should not be requeued. - NFS4ERR_BAD_SLOT: Skips the signal check, but shouldn't. I need to think more about BAD_SLOT recovery best practice. - NFS4ERR_SEQ_MISORDERED: Does one retry with a seq_nr of 1. It probably should terminate if that fails. IMO this should check for rpc_clnt shutdown before requeuing the retry. - default: I don't think this case should ever be requeued, but it appears that it could be if the rpc_clnt is shutting down. >>> Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors") >>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>> --- >>> fs/nfsd/nfs4callback.c | 3 --- >>> 1 file changed, 3 deletions(-) >>> >>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c >>> index 50e468bdb8d4838b5217346dcc2bd0fec1765c1a..e12205ef16ca932ffbcc86d67b0817aec2436c89 100644 >>> --- a/fs/nfsd/nfs4callback.c >>> +++ b/fs/nfsd/nfs4callback.c >>> @@ -1403,9 +1403,6 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback >>> } >>> trace_nfsd_cb_free_slot(task, cb); >>> nfsd41_cb_release_slot(cb); >>> - >>> - if (RPC_SIGNALLED(task)) >>> - goto need_restart; >>> out: >>> return ret; >>> retry_nowait: >>> >>> -- >>> 2.48.1 >>> >>> >> >
On Sun, 2025-01-26 at 11:41 -0500, Chuck Lever wrote: > On 1/26/25 6:18 AM, Jeff Layton wrote: > > On Sun, 2025-01-26 at 10:01 +1100, NeilBrown wrote: > > > On Fri, 24 Jan 2025, Jeff Layton wrote: > > > > This is problematic, since the RPC might have been entirely successful. > > > > There is no point in restarting a v4.1+ callback just because > > > > RPC_SIGNALLED is true. The v4.1+ error handling has other mechanisms for > > > > detecting when it should retransmit the RPC. > > > > > > But why might RPC_SIGNALLED() ever be true? > > > The flag RPC_TASK_SIGNALLED is only ever set by rpc_signal_task() which > > > is only called from rpc_killall_tasks() and __rpc_execute() for > > > non-async tasks which doesn't apply to nfsd callbacks as they are > > > started with rpc_call_async(). > > > > > > rpc_killall_tasks() is called by fs/nfs/ which isn't relevant for us, > > > and from rpc_shutdown_client(). In those cases we certainly don't want > > > the request to be retried, though the nfsd4_process_cb_update() case is > > > a little interesting as we do want it to be retried, but in a different > > > client. > > > > > > So the code you are removing is either dead code because something else > > > will prevent the restart when a client is being shut down, or it is bad > > > code because it would delay rpc_shutdown_client() while the request is > > > retried. > > > > > > I haven't dug the extra step to figure out which, but either way I think > > > the code should go. > > > > Thanks. That was my analysis too. > > Agreed, this code is problematic, but it appears to me that some of > these problems are not resolved by simply removing the signal check. > > > > rpc_shutdown_client() is called when we tear down and rebuild the > > rpc_client. nfsd does this in setup_callback_client(), which gets > > called from nfsd4_process_cb_update() (basically when we detect that > > the backchannel is having problems). > > > > There are really only two states: We either got a reply from the server > > before the client went down, or we didn't. In the case where we got a > > reply, there is no need to retry anything. In the case where we didn't, > > the tk_status will be '1', so there is no need to check RPC_SIGNALLED() > > at all here. > > Fwiw, the "cb_seq_status == 1" arm skips the signal check in the current > code. > > > > The existing code could lead to the call being retried when we had > > already gotten a perfectly valid reply. > > Here's a case-by-case audit: > > - NFS_OK: SEQUENCE was decoded and passed sanity checks. So this should > not ever requeue in here. It might be requeued during subsequent > processing. > > - ESERVERFAULT: SEQUENCE was decoded but failed sanity checking. The > reply should be dropped now, and the session marked FAULT. No requeue > is ever needed here. > > [ I question whether the sequence number should be bumped in this > case -- the client's callback server replied with the identity of > some other slot. And anyway, this session is about to become > toast. ] > It didn't necessarily reply with the ID of a different slot. It's just that the decoding failed in some way. It could have been any of the cases in decode_cb_sequence4resok(). Maybe that function needs to return more distinct error codes so we know what was mangled. > - 1: The timeout case. We want a fresh session here, so it falls > through to BADSESSION. > Ok. > - NFS4ERR_BADSESSION: This needs a requeue so that the operation can > be retried with a fresh session. But it should always check if the > rpc_clnt is shutting down before doing so. This is a problem in the > current code. > I'm not sure I understand the problem you see with that in the existing code. There's a rather complicated dance in nfsd4_process_cb_update(), but if the nfs4_client is shutting down, then clp->cl_cb_client will be NULL after it, and the callback will end. You said "rpc_clnt" though, so I'm not sure I understand the scenario you mean. > - NFS4ERR_DELAY: Skips the signal check, but shouldn't. If the rpc_clnt > is shutting down, this RPC should not be requeued. > Good point -- ot sure how we deal with that in a non-racy way. I'll think about it. > - NFS4ERR_BAD_SLOT: Skips the signal check, but shouldn't. I need to > think more about BAD_SLOT recovery best practice. > RPC_SIGNALLED() is irrelevant here. I think what we want to do is mark the backchannel as faulty, _leak_ the slot and retry via the workqueue (not just requeue the rpc_task). That should just cause the callback to exit once it runs again. We should also mark the backchannel as faulty, since the client and server no longer agree on the size of the slot table. > - NFS4ERR_SEQ_MISORDERED: Does one retry with a seq_nr of 1. It > probably should terminate if that fails. IMO this should check for > rpc_clnt shutdown before requeuing the retry. > Fair enough. There is a frustrating lack of guidance in the spec about SEQ_MISORDERED. We should probably mark the BC as having a FAULT too if the retry fails. > - default: I don't think this case should ever be requeued, but it > appears that it could be if the rpc_clnt is shutting down. > Yeah. Might not hurt to throw a pr_warn() here too. I think we never want to fall into this case. In any case, my intention is to fix up the cb_session lifetime problem first, and then we can rework the error handling from the callbacks on top of that.
On 1/27/25 10:43 AM, Jeff Layton wrote: > On Sun, 2025-01-26 at 11:41 -0500, Chuck Lever wrote: >> - ESERVERFAULT: SEQUENCE was decoded but failed sanity checking. The >> reply should be dropped now, and the session marked FAULT. No requeue >> is ever needed here. >> >> [ I question whether the sequence number should be bumped in this >> case -- the client's callback server replied with the identity of >> some other slot. And anyway, this session is about to become >> toast. ] > > It didn't necessarily reply with the ID of a different slot. It's just > that the decoding failed in some way. My read is that if the XDR decode failed in any way, the decoder sets cb_seq_status to -EIO. -ESERVERFAULT means the decoding went fine, but one or more of the session ID, slot number, or sequence did not match what NFSD's callback client expected. It's not the same slot if either the session ID or slot number doesn't match what the server sent in its CB_SEQUENCE Call. That seems equivalent to BAD_SLOT without any question. If the sequence number is wrong, then it's equivalent to SEQ_MISORDERED, maybe? > It could have been any of the > cases in decode_cb_sequence4resok(). Maybe that function needs to > return more distinct error codes so we know what was mangled. My preference would be that decode_cb_sequence() should simply decode these fields, and let nfsd4_cb_sequence_done() do the sanity checking. I don't think decode_cb_sequence4resok() should be doing any sanity checking beyond "does this unmarshal in the space allowed?"
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c index 50e468bdb8d4838b5217346dcc2bd0fec1765c1a..e12205ef16ca932ffbcc86d67b0817aec2436c89 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -1403,9 +1403,6 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback } trace_nfsd_cb_free_slot(task, cb); nfsd41_cb_release_slot(cb); - - if (RPC_SIGNALLED(task)) - goto need_restart; out: return ret; retry_nowait:
This is problematic, since the RPC might have been entirely successful. There is no point in restarting a v4.1+ callback just because RPC_SIGNALLED is true. The v4.1+ error handling has other mechanisms for detecting when it should retransmit the RPC. Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors") Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/nfs4callback.c | 3 --- 1 file changed, 3 deletions(-)