Subject: [nfsv4] pynfs replay cache test SEQ9f

From: Thomas Haynes <[email-masked]>
Date: Thu, 12 Oct 2017 18:32:09 +0000

Hi Bruce,

In this test:

def testReplayCache006(t, env):
   """Send two solo sequence compounds with same seqid

   FLAGS: sequence all
   CODE: SEQ9f
   """
   c = env.c1.new_client(env.testname(t))
   sess = c.create_session()
   res1 = sess.compound([])
   check(res1)
   res2 = sess.compound([], cache_this=True, seq_delta=0)
   check(res2)
   res1.tag = res2.tag = ""
   if not nfs4lib.test_equal(res1, res2):
       fail("Replay results not equal")

I don't see why the result should be NFS4_OK.

The first compound does not set sa_cachethis and the second
does set it. According to RFC5661, the server is not required
to cache the first request if the client does not request it.

And if the server does not cache the response, when it gets
a request to replay it, it can return NFS4ERR_RETRY_UNCACHED_REP:

2.10.6.1.3.  Optional Reply Caching

  On a per-request basis, the requester can choose to direct the
  replier to cache the reply to all operations after the first
  operation (SEQUENCE or CB_SEQUENCE) via the sa_cachethis or
  csa_cachethis fields of the arguments to SEQUENCE or CB_SEQUENCE.
  The reason it would not direct the replier to cache the entire reply
  is that the request is composed of all idempotent operations [34].
  Caching the reply may offer little benefit.  If the reply is too
  large (see Section 2.10.6.4), it may not be cacheable anyway.  Even
  if the reply to idempotent request is small enough to cache,
  unnecessarily caching the reply slows down the server and increases
  RPC latency.

  Whether or not the requester requests the reply to be cached has no
  effect on the slot processing.  If the results of SEQUENCE or
  CB_SEQUENCE are NFS4_OK, then the slot's sequence ID MUST be
  incremented by one.  If a requester does not direct the replier to
  cache the reply, the replier MUST do one of following:

  o  The replier can cache the entire original reply.  Even though
     sa_cachethis or csa_cachethis is FALSE, the replier is always free
     to cache.  It may choose this approach in order to simplify
     implementation.

  o  The replier enters into its reply cache a reply consisting of the
     original results to the SEQUENCE or CB_SEQUENCE operation, and
     with the next operation in COMPOUND or CB_COMPOUND having the
     error NFS4ERR_RETRY_UNCACHED_REP.  Thus, if the requester later
     retries the request, it will get NFS4ERR_RETRY_UNCACHED_REP.  If a
     replier receives a retried Sequence operation where the reply to
     the COMPOUND or CB_COMPOUND was not cached, then the replier,

Further, since the parameters to SEQUENCE are different it is a false
retry according to this:

2.10.6.1.3.1.  False Retry

If a requester sent a Sequence operation with a slot ID and sequence
ID that are in the reply cache but the replier detected that the
retried request is not the same as the original request, including a
retry that has different operations or different arguments in the
operations from the original and a retry that uses a different
 principal in the RPC request's credential field that translates to a
different user, then this is a false retry.  When the replier detects
a false retry, it is permitted (but not always obligated) to return
NFS4ERR_SEQ_FALSE_RETRY in response to the Sequence operation when it
detects a false retry.

I'm not sure if the test should be fixed to either

1) allow either NFS4_OK or NFS4ERR_RETRY_UNCACHED_REP
2) just remove the test.

Thoughts?

---

From: "J. Bruce Fields" <[email-masked]>
Date: Thu, 12 Oct 2017 15:49:46 -0400

> a request to replay it, it can return NFS4ERR_RETRY_UNCACHED_REP:

If I'm reading that section correctly, it can't return
NFS4ERR_RETRY_UNCACHED_REP, that can only be returned on the next op of
the compound (and there is no next op in this case, only the one
SEQUENCE).

So I *think* the only correct options OK or FALSE_RETRY?

> 1) allow either NFS4_OK or NFS4ERR_RETRY_UNCACHED_REP

Maybe just add cache_this to the first?

Looks like the following test is off too--I think a server's allowed to
return a cached reply instead of RETRY_UNCACHED_REP?

--b.

---

From: Thomas Haynes <[email-masked]>
Date: Thu, 12 Oct 2017 21:39:04 +0000

On Oct 12, 2017, at 12:49 PM, J. Bruce Fields <[email-masked]<mailto:[email-masked]>> wrote:

In this test:

  FLAGS: sequence all
  CODE: SEQ9f
  """

It can’t be OK if the parameters to SEQUENCE differ.

Ack. I’ll send in a patch for this approach.

Yes, it is allowed to do so since it can always cache.

--b.

---

From: "J. Bruce Fields" <[email-masked]>
Date: Thu, 12 Oct 2017 17:44:54 -0400

Your mailer's not quoting right, it's a little hard for me to find your
replies.  Wading in:

> It can’t be OK if the parameters to SEQUENCE differ.

I'm getting that from: "When the replier detects a false retry, it is
permitted (but not always obligated) to return NFS4ERR_FALSE_RETRY in
response to the Sequence operation when it detects a false retry."

If i understand the following language, we're required to return
FALSE_RETRY in the case the rpc credentials of the caller map to
different principals, but not otherwise.

--b

---

From: Tom Haynes <[email-masked]>
Date: Thu, 12 Oct 2017 15:00:51 -0700

> response to the Sequence operation when it detects a false retry."

I think you are agreeing with me that OK is not appropriate here?

> different principals, but not otherwise.

This one drove me crazy:

   operations from the original

SEQUENCE is not special - both the compounds in this example
only have the SEQUENCE op and they differ only in that in the
first sa_cachethis is False and in the second it is True.

So we have to return FALSE_SEQ_RETRY here...

> 
> --b
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email-masked]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

---

From: "J. Bruce Fields" <[email-masked]>
Date: Thu, 12 Oct 2017 21:52:23 -0400

> So we have to return FALSE_SEQ_RETRY here...

It says "if the replier detected" a difference, not "if there is" a
difference.  So the replier is not required to do such detection.  This
agrees with the "not always obligated" above.

So I think it's allowed for the server to just return an old cached
response here (with the cached OK).  And I can't see any practical
problem that would create--a client shouldn't be sending a different
request with the same (slot, sequence) anyway.  The only potential risk
is the malicious client trying to snoop somebody else's reply cache,
hence the requirement in the case principals differ.

--b.

---

From: Trond Myklebust <[email-masked]>
Date: Fri, 13 Oct 2017 13:34:28 +0000

On Thu, 2017-10-12 at 21:52 -0400, J. Bruce Fields wrote:
> > > > On Oct 12, 2017, at 12:49 PM, J. Bruce Fields <bfields@fieldses
> > > > .org<mailto:[email-masked]>> wrote:

Clients may indeed send a different request with the same slot and
sequence if they don't know that the server received the last request.
This is tbe "user pressed ^C" scenario...

Servers MAY ignore that fact, but they'd be really stupid to do so...

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email-masked]

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 13 Oct 2017 11:00:21 -0400

> Servers MAY ignore that fact, but they'd be really stupid to do so...

OK, OK, I'll look into fixing the server (I'm pretty sure we get this
wrong).

You've explained the ctrl-C case before and I don't think I understood
it.  I guess otherwise the only way for the client to sort out the
situation would be to retry the original request.  And that requires
keeping the arguments and credentials around to handle potential
retries.  And that's impractical if the process is going away?  OK.

--b.

---

From: Trond Myklebust <[email-masked]>
Date: Fri, 13 Oct 2017 15:26:51 +0000

On Fri, 2017-10-13 at 11:00 -0400, [email-masked] wrote:
> > > > > I'm getting that from: "When the replier detects a false
> > > > > retry,
> > > > > it is
> > > >    retry that has different operations or different arguments
> > > > in
> > > > the
> > > request with the same (slot, sequence) anyway.  The only
> > > potential
> > > risk

Right, we're not going to do that just for data that is just going to
be tossed away anyway. We already guarantee that non-idempotent
operations (the ones that we actually do ask the server to cache) are
guaranteed to complete whether or not the user presses ^C, so this is
mainly about what happens when somebody interrupts an operation that we
did not want the server to cache.

I have a patch out there that just replays a SEQUENCE op if we detect
that an RPC call was interrupted. That should be sufficient to deal
with servers that cache everything (whether or not the client sets
sa_cachethis), but don't want to do NFS4ERR_SEQ_FALSE_RETRY. That
particular combination has been seen to be extremely toxic to the
current client, because it can get replayed LOOKUP or GETATTR requests
after someone presses ^C.

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 13 Oct 2017 14:50:15 -0400

> after someone presses ^C.

Those all involve uncached compounds with more than one op.  My reading
of knfsd code is that it will return RETRY_UNCACHED_REP in this case,
and I think (I might be misunderstanding) that the client will bump the
slot seqid and retry in that case.  So I *think* you shouldn't be seeing
that problem with knfsd?

--b.

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 13 Oct 2017 16:19:23 -0400

> slot seqid and retry in that case.  So I *think* you shouldn't be seeing

So to bring it back to the original question, I'm still not sure it's
actually a problem if a server returns NFS_OK when cache_this is set
differently betwen two compound calls each containing just a single
SEQUENCE.

But, maybe it's worth at least warning about.

--b.

---

From: "Frank Filz" <[email-masked]>
Date: Mon, 16 Oct 2017 09:15:45 -0700

> > > > > > Your mailer's not quoting right, it's a little hard for me to
> > > > > > find your replies.  Wading in:
> > > > > >
> > > > > > > On Oct 12, 2017, at 12:49 PM, J. Bruce Fields <bfields@fiel
> > > > > > > dses .org<mailto:[email-masked]>> wrote:
> > > > > > retry, it is permitted (but not always obligated) to return
> > > > > > NFS4ERR_FALSE_RETRY in response to the Sequence operation
> when
> > > > > > it detects a false retry."
> > > > >
> > > > > > If i understand the following language, we're required to
> > > > > > return FALSE_RETRY in the case the rpc credentials of the
> > > > > > caller map to different principals, but not otherwise.
> > > > >
> > > > > This one drove me crazy:
> > > > >
> > > > difference.  So the replier is not required to do such detection.
> > > > This agrees with the "not always obligated" above.
> > > >
> > > > So I think it's allowed for the server to just return an old
> > > > cached response here (with the cached OK).  And I can't see any
> > > > practical problem that would create--a client shouldn't be sending
> > > > a different request with the same (slot, sequence) anyway.  The
> > > > only potential risk is the malicious client trying to snoop
> > > > somebody else's reply cache, hence the requirement in the case
> > > > principals differ.
> > >
> Right, we're not going to do that just for data that is just going to be tossed
> away anyway. We already guarantee that non-idempotent operations (the
> ones that we actually do ask the server to cache) are guaranteed to complete
> whether or not the user presses ^C, so this is mainly about what happens
> when somebody interrupts an operation that we did not want the server to
> cache.
> 
> I have a patch out there that just replays a SEQUENCE op if we detect that an
> RPC call was interrupted. That should be sufficient to deal with servers that
> cache everything (whether or not the client sets sa_cachethis), but don't
> want to do NFS4ERR_SEQ_FALSE_RETRY. That particular combination has
> been seen to be extremely toxic to the current client, because it can get
> replayed LOOKUP or GETATTR requests after someone presses ^C.

I'm wondering if Ganesha does this right...

Bruce, if there's something not covered by the pynfs tests, can we make sure to add one?

Frank

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 17 Oct 2017 17:31:20 -0400

> slot seqid and retry in that case.  So I *think* you shouldn't be seeing

Argh, no, you're sending a bare SEQUENCE so of course there's just one
op.

And looking at Olga's COPY example and the code....  The server gets
confused in this case and returns a reply to the SEQUENCE, nothing else,
but sets the reply's opcnt to the count taken from the original call,
for some reason.

So, the server's returning a corrupt reply.  It needs to return a reply
that's actually legal xdr and SEQUENCE results that match the call.
Beyond that it probably doesn't matter exactly what it returns--either
it handles it as a replay and doesn't bump the seqid, or a new call and
does, but either way the seqid ends up in the same place, which is the
goal here.  OK.

--b.