| Message ID | 20250123-nfsd-6-14-v1-1-c1137a4fa2ae@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Chuck Lever |
| Headers | show |
| Series | nfsd: CB_SEQUENCE error handling fixes and cleanups | expand |
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 >>> >>> >> >
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(-)