netdev - Re: [PATCH 3/8] nfsd: when CB_SEQUENCE gets NFS4ERR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACSpFtA4hqoLW4_u5dxb+XxmKH1On0=OSvzMdovmimz75KzdXQ@mail.gmail.com>
Date: Fri, 24 Jan 2025 12:47:53 -0500
From: Olga Kornievskaia <okorniev@...hat.com>
To: "J. Bruce Fields" <bfields@...ldses.org>
Cc: Jeff Layton <jlayton@...nel.org>, Chuck Lever <chuck.lever@...cle.com>, 
	Neil Brown <neilb@...e.de>, Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>, 
	Kinglong Mee <kinglongmee@...il.com>, Trond Myklebust <trondmy@...nel.org>, 
	Anna Schumaker <anna@...nel.org>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Simon Horman <horms@...nel.org>, linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org, 
	netdev@...r.kernel.org
Subject: Re: [PATCH 3/8] nfsd: when CB_SEQUENCE gets NFS4ERR_DELAY, release
 the slot

On Fri, Jan 24, 2025 at 12:45 PM Olga Kornievskaia <okorniev@...hat.com> wrote:
>
> On Fri, Jan 24, 2025 at 9:08 AM J. Bruce Fields <bfields@...ldses.org> wrote:
> >
> > On Thu, Jan 23, 2025 at 06:20:08PM -0500, Jeff Layton wrote:
> > > On Thu, 2025-01-23 at 17:18 -0500, Chuck Lever wrote:
> > > > On 1/23/25 3:25 PM, Jeff Layton wrote:
> > > > > RFC8881, 15.1.1.3 says this about NFS4ERR_DELAY:
> > > > >
> > > > > "For any of a number of reasons, the replier could not process this
> > > > >   operation in what was deemed a reasonable time. The client should wait
> > > > >   and then try the request with a new slot and sequence value."
> > > >
> > > > A little farther down, Section 15.1.1.3 says this:
> > > >
> > > > "If NFS4ERR_DELAY is returned on a SEQUENCE operation, the request is
> > > >   retried in full with the SEQUENCE operation containing the same slot
> > > >   and sequence values."
> > > >
> > > > And:
> > > >
> > > > "If NFS4ERR_DELAY is returned on an operation other than the first in
> > > >   the request, the request when retried MUST contain a SEQUENCE operation
> > > >   that is different than the original one, with either the slot ID or the
> > > >   sequence value different from that in the original request."
> > > >
> > > > My impression is that the slot needs to be held and used again only if
> > > > the server responded with NFS4ERR_DELAY on the SEQUENCE operation. If
> > > > the NFS4ERR_DELAY was the status of the 2nd or later operation in the
> > > > COMPOUND, then yes, a different slot, or the same slot with a bumped
> > > > sequence number, must be used.
> > > >
> > > > The current code in nfsd4_cb_sequence_done() appears to be correct in
> > > > this regard.
> > > >
> > >
> > > Ok! I stand corrected. We should be able to just drop this patch, but
> > > some of the later patches may need some trivial merge conflicts fixed
> > > up.
> > >
> > > Any idea why SEQUENCE is different in this regard?
> >
> > Isn't DELAY on SEQUENCE an indication that the operation is still in
> > progress?  The difference between retrying the same slot or not is
> > whether you're asking the server again for the result of the previous
> > operation, or whether you're asking it to perform a new one.
> >
> > If you get DELAY on a later op and then keep retrying with the same
> > seqid/slot then I'd expect you to get stuck in an infinite loop getting
> > a DELAY response out of the reply cache.
>
> If the client would keep re-using the same seqid for the operation it
> got a DELAY on then it would be a broken client. When the linux client
> gets ERR_DELAY, it retries the operation but it increments the seqid.

Urgh I stand corrected this is on the SEQUENCE not an operation.
>
> >
> > --b.
> >
> >
> > > This rule seems a
> > > bit arbitrary. If the response is NFS4ERR_DELAY, then why would it
> > > matter which slot you use when retransmitting? The responder is just
> > > saying "go away and come back later".
> > >
> > > What if the responder repeatedly returns NFS4ERR_DELAY (maybe because
> > > it's under resource pressure), and also shrinks the slot table in the
> > > meantime? It seems like that might put the requestor in an untenable
> > > position.
> > >
> > > Maybe we should lobby to get this changed in the spec?
> > >
> > > >
> > > > > This is CB_SEQUENCE, but I believe the same rule applies. Release the
> > > > > slot before submitting the delayed RPC.
> > > > >
> > > > > Fixes: 7ba6cad6c88f ("nfsd: New helper nfsd4_cb_sequence_done() for processing more cb errors")
> > > > > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > > > > ---
> > > > >   fs/nfsd/nfs4callback.c | 1 +
> > > > >   1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > > > > index bfc9de1fcb67b4f05ed2f7a28038cd8290809c17..c26ccb9485b95499fc908833a384d741e966a8db 100644
> > > > > --- a/fs/nfsd/nfs4callback.c
> > > > > +++ b/fs/nfsd/nfs4callback.c
> > > > > @@ -1392,6 +1392,7 @@ static bool nfsd4_cb_sequence_done(struct rpc_task *task, struct nfsd4_callback
> > > > >                   goto need_restart;
> > > > >           case -NFS4ERR_DELAY:
> > > > >                   cb->cb_seq_status = 1;
> > > > > +         nfsd41_cb_release_slot(cb);
> > > > >                   if (!rpc_restart_call(task))
> > > > >                           goto out;
> > > > >
> > > > >
> > > >
> > > >
> > >
> > > --
> > > Jeff Layton <jlayton@...nel.org>
> >