Subject: [nfsv4] Needed changes regarding chunk errors -- Take 2

From: David Noveck <[email-masked]>
Date: Mon, 26 Nov 2018 11:23:40 -0500

What follows is a revised version of the changes suggested for RFC 8166, to
include a treatment of chunk-related errors.

A number of changes have been made in response to Chuck's comments:

   - The replacement text in Section 3.4.4 has been simplified with the
   text regarding specifics of errors to be reported in response to incorrect
   chunk lengths moved to sections 3.4.5 and 3.4.6.
   - There is added use of RFC2119 terms in connection with the requester's
   use of incorrect lengths and positions and the responder's reporting of
   such errors.
   - In cases in which we know of the existence of clients which generate
   incorrect chunks or believe that servers exist that do not report errors
   when confronted with such chunks, "MUST" has been replaced by "SHOULD".  I
   have done so without being certain that "MUST" is incorrect but feel the
   issue needs to be discussed.

The last paragraph of Section 3.4.4 needs to be revised.   As written, it
causes confusion by drawing attention to the total rpc length despite the
fact that each chunk represents a single ddp-eligible data item.   Also, it
is not clear what the responder is to do if there is a failure of the
"sanity check".  Suggest replacing the pargraph by the following.

The RPC-over-RDMA version 1 transport protocol does not place a limit
on chunk size.  However, each ULP defines its own set of ddp-eligilble
data items and the transport places restrictions on the length of
chunks to prevent them diverging from the length of the data items
that they transfer.

A new paragraph needs to be added to section 3.4.5 following the current
second paragraph. I suggest that it read as follows:

The length of a Read chunk is the sum of the length of all the segments
within it. The length of a Read chunk MUST be no smaller than than the
DDP-eligible data item that it is used to transfer and SHOULD only be
larger as nceesary to provide necessary round-up, as described in Section
3.4.5.1 below. When a responder receives a chunk with an inappropriate
length, it SHOULD return an ERR_CHUNK error and MUST do so if the length is
smaller than that of the item being transferred.

In the seventh paragraph of Section 3.4.5, a number of changes are
necessary:

   - The second sentence needs to be replaced by the following:

The value in this field MUST be the byte offset  of a DDP-eligible
data item (or zero to accommodate the case discussed in Section
3.5.3).  As a result. it will always be evenly divisible by four.

   - An additional sentence needs to be added, as follows:

When the Position field in a read segment is not vallid, the responder
MUST return the error ERR_CHUNK.

In Section 3.5.6, need to add the following sentence to the second pargraph.

When a responder receives a Write chunk whose length is smaller than the
data item to be transferred, it MUST, except in the case of a Write chunk
with zero segments, return the error ERR_CHUNK.

In Section 4.5.2, the first paragraph needs to be revised and clarified.
Suggest replacing it by the following paragrphs:

A receiver might encounter an XDR parsing error that prevents it from
processing the incoming Transport stream.  One example of such an
error is an invalid value in the rdma_proc field. If the rdma_vers
field contains a recognized value, but an XDR parsing error occurs,
the Responder MUST reply with an RDMA_ERROR procedure and set the
rdma_err value to ERR_CHUNK.

In addition, a receiver might encounter a situation that prevents it
from processing the incoming Transport stream, even when the stream
conforms to the XDR definition.  Examples of such issues include an
RDMA_NOMSG message where the Read list, Write list, and Reply chunk
are all marked as not present, a case in which the value of the
rdma_xid field does not match the value of the XID field in the
accompanying RPC message, a Read chunk position which is not valid,
and  a chunk whose length is not valid.  In such cases, the Responder
MUST also reply with an RDMA_ERROR procedure and set the rdma_err
value to ERR_CHUNK.

---

From: Chuck Lever <[email-masked]>
Date: Mon, 26 Nov 2018 12:35:48 -0500

Hi Dave-

Inline comments.

> On Nov 26, 2018, at 11:23 AM, David Noveck <[email-masked]> wrote:
> 
> What follows is a revised version of the changes suggested for RFC 8166, to include a treatment of chunk-related errors.
> 
> 	• The replacement text in Section 3.4.4 has been simplified with the text regarding specifics of errors to be reported in response to incorrect chunk lengths moved to sections 3.4.5 and 3.4.6.
> 	• There is added use of RFC2119 terms in connection with the requester's use of incorrect lengths and positions and the responder's reporting of such errors.
> 	• In cases in which we know of the existence of clients which generate incorrect chunks or believe that servers exist that do not report errors  when confronted with such chunks, "MUST" has been replaced by "SHOULD".  I have done so without being certain that "MUST" is incorrect but feel the issue needs to be discussed.
>                                                                                                                                                                                                                 
> 
> The last paragraph of Section 3.4.4 needs to be revised.   As written, it causes confusion by drawing attention to the total rpc length despite the fact that each chunk represents a single ddp-eligible data item.   Also, it is not clear what the responder is to do if there is a failure of the "sanity check".  Suggest replacing the pargraph by the following.
>> The RPC-over-RDMA version 1 transport protocol does not place a limit on chunk size.  However, each ULP defines its own set of ddp-eligilble data items and the transport places restrictions on the length of chunks to prevent them diverging from the length of the data items that they transfer.

The current text is meant to suggest that there is a practical
upper bound on the size of data items in general, determined by
the upper layer protocol. This is generic advice to implementers.

I would prefer to stick with that meaning of the text. A better
approach would be to say that ULPs can place a size limit on data
items, and thus that restricts the potential size of chunks. The
receiver might utilize that knowledge to implement a basic check
of the chunks in a transport header before it passes the RPC to
the upper layer decoder.

> A new paragraph needs to be added to section 3.4.5 following the current second paragraph.  I suggest that it read as follows:
>> The length of a Read chunk is the sum of the length of all the segments within it.   The length of a Read chunk  MUST be no smaller than than the DDP-eligible data item that it is used to transfer and SHOULD only be larger as nceesary to provide necessary round-up, as described in Section 3.4.5.1 below.

Section 3.4.4.4 already says:

> Except in special cases (covered in Section 3.5.3), a chunk MUST
> contain exactly one XDR data item.

The proposed SHOULD implies there are cases when it is permissible
for a client to put more data in a chunk. That seems to contradict
the MUST in 3.4.4.4.

>> When a responder receives a chunk with an inappropriate length, it SHOULD return an ERR_CHUNK error and MUST do so if the length is smaller than that of the item being transferred.

Ignoring the extra bytes could lead to data corruption, so I'm
inclined to say that if the server detects a header that is mal-
formed in this way, it should not process it but should instead
respond with an error rather than ignoring the problem.

However current server implementations might not recognize this
issue. Would it be better to acknowledge that in this text but
say that even so, a Read chunk length that does not match the size
of the data item it transports MUST result in ERR_CHUNK?

Section 3.4.5.1 might be the best place for this.

Note however that determining whether there is exactly one data
item in a chunk might be left to the upper layer's XDR decoder.
This is one area where GARBAGE_ARGS is a desirable alternative.

>  In the seventh paragraph of Section  3.4.5, a number of changes are necessary:
> 	• The second sentence needs to be replaced by the following:
> The value in this field MUST be the byte offset  of a DDP-eligible data item (or zero to accommodate the case discussed in Section 3.5.3).  As a result. it will always be evenly divisible by four.  
> 	• An additional sentence needs to be added, as follows:
> When the Position field in a read segment is not vallid, the responder MUST return the error ERR_CHUNK.

> In Section 3.5.6, need to add the following sentence to the second paragraph.
>> When a responder receives a Write chunk whose length is smaller than the data item to be transferred, it MUST, except in the case of a Write chunk with zero segments, return the error ERR_CHUNK.

If the server cannot send a reply, then the client's RPC transaction
hangs indefinitely, which is undesirable. I think the current Linux
server is careful to check for this case.

> In Section 4.5.2, the first paragraph needs to be revised and clarified.  Suggest replacing it by the following paragrphs:
>> A receiver might encounter an XDR parsing error that prevents it from processing the incoming Transport stream.  One example of such an error is an invalid value in the rdma_proc field. If the rdma_vers field contains a recognized value, but an XDR parsing error occurs, the Responder MUST reply with an RDMA_ERROR procedure and set the rdma_err value to ERR_CHUNK.

This first case is where a server implementation might prefer to return
GARBAGE_ARGS instead. Other areas of RFC 8166 permit this.

>> In addition, a receiver might encounter a situation that prevents it from processing the incoming Transport stream, even when the stream conforms to the XDR definition.  Examples of such issues include an RDMA_NOMSG message where the Read list, Write list, and Reply chunk are all marked as not present, a case in which the value of the rdma_xid field does not match the value of the XID field in the accompanying RPC message, a Read chunk position which is not valid, and  a chunk whose length is not valid.  In such cases, the Responder MUST also reply with an RDMA_ERROR procedure and set the rdma_err value to ERR_CHUNK.

--
Chuck Lever

---

From: Chuck Lever <[email-masked]>
Date: Mon, 26 Nov 2018 13:47:12 -0500

> On Nov 26, 2018, at 12:35 PM, Chuck Lever <[email-masked]> wrote:
> 
> Hi Dave-
> 
> Inline comments.
> 
> 

Whoops. I misread this. GARBAGE_ARGS is not appropriate here.

> _______________________________________________
> nfsv4 mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/nfsv4

--
Chuck Lever

---

From: David Noveck <[email-masked]>
Date: Tue, 27 Nov 2018 07:54:25 -0500

Given the following "Whoops", it appears we seem to be in a agreement about
4.5.2.  I'll prepare a draft erratta.

With regard to 3.4.4:

> the upper layer protocol.

If that is the intention, the existing text doesn't really convey it, since
it refers to the
size of the messages as a whole, rather than to the data items.   It is
hard to refer to "data items" in genral since there is no clear defnition of
one, since they inherently can nest.

Also, this text assumes, as does much of rfc5666 that the client and server
are
each doing the right thing and there is little discussion of what to do if
the
other side does the wrong thing.

> This is generic advice to implementers.

I'm not sure what the advice is.  The phrase "sanity check" does not tell
me what my
server should do, as there is no ERR_INSANE.  A possibility for v2/v3 :-)

> I would prefer to stick with that meaning of the text.

I think the meaning/intention you described is what I tried to convey.

>A better
> items, and thus that restricts the potential size of chunks.

I think that is pretty much what my text says.   It does restrict itself
to DDP-eligible
data items since:

   - There has to be a clear definition of those
   - They cannot nest.
   - They are the only ones which affect the size of chunks.

> The

It might but it is getting too much into implementation details to discuss
that in the document.

The impportant point is that we agree that the mismatch is an error and so,
the resut of any check you make, is definitive.

>> contain exactly one XDR data item.

I missed this :-(

the MUST in 3.4.4.4.

I only put in the "SHOULD" because there are ("old") clients in existence
that do
this.   If you are OK, treating these as buggy, I'm OK switching back
to "MUST".

>> When a responder receives a chunk with an inappropriate length, it
SHOULD return an ERR_CHUNK error and MUST do so if the length is smaller
than that of the item being transferred.

> Ignoring the extra bytes could lead to data corruption,

It suppose it could, but in my (not very extensive) testing it hasn't.
It depends on the preciose natue of the screwed-up client's problem.

> so I'm

I'm OK wth making this a "MUST"

> issue.

I'm sure there are, depending on what you mean by "current".   The broken
clients discussed above must have worked successfully with some servers
ot they could not have been tested at all.

> Would it be better to acknowledge that in this text but

I don'r see how.   "MUST" says it is required for interoperability and that
not doing it puts interoperability in question.   Sure there are buggy
non-compliant
implementations but I don't think a stadards-track document is the place to
document the existence of particular bugs.  Even more troubling is putting
this
in an errata, somehow implying that the absense of such bug reports in
RFC8166
is an error that needs to be corrected :-(.

> Section 3.4.5.1 might be the best place for this.

I'll look at that.

> item in a chunk might be left to the upper layer's XDR decoder.

It might, if that decoder were modified to be aware of chunk boundaries.
I think the alternative, of making the transport level code aware of XDR
item
structure is simpler.   Either one is, if truth be told, really a layeing
violation,
but we are where we are and every implemenation will need to pass
information
acroos layer boundaries :-(..

> This is one area where GARBAGE_ARGS is a desirable alternative.

I think it is most desirable for a partiular malformation to be mappable to
a single error.   I think we should be flexible if necessary.  So one
question is
whether there are  servers that check for this issue and return
GARBAGE_ARGS.  We think ther are those that ignore the issue and those
that return ERR_CHUNK.

> hangs indefinitely, which is undesirable.

Yes, But there is no suggestion that irt t might do nothing.   The question
is whether it report an error or
tries to deal, as best it can with something arguable malformed.  Does
something need to be added
to clarify this?

> I think the current Linux

I asssume that when it checks for this, it returns ERR_CHUNK if the check
fails.  Does it?

On Mon, Nov 26, 2018 at 12:35 PM Chuck Lever <[email-masked]> wrote:

> > What follows is a revised version of the changes suggested for RFC 8166,
> to include a treatment of chunk-related errors.
> >
> >       • The replacement text in Section 3.4.4 has been simplified with
> the text regarding specifics of errors to be reported in response to
> incorrect chunk lengths moved to sections 3.4.5 and 3.4.6.
> >       • There is added use of RFC2119 terms in connection with the
> requester's use of incorrect lengths and positions and the responder's
> reporting of such errors.
> >       • In cases in which we know of the existence of clients which
> generate incorrect chunks or believe that servers exist that do not report
> errors  when confronted with such chunks, "MUST" has been replaced by
> "SHOULD".  I have done so without being certain that "MUST" is incorrect
> but feel the issue needs to be discussed.
> >
>
>
> >
> > The last paragraph of Section 3.4.4 needs to be revised.   As written,
> it causes confusion by drawing attention to the total rpc length despite
> the fact that each chunk represents a single ddp-eligible data item.
>  Also, it is not clear what the responder is to do if there is a failure of
> the "sanity check".  Suggest replacing the pargraph by the following.
> on chunk size.  However, each ULP defines its own set of ddp-eligilble data
> items and the transport places restrictions on the length of chunks to
> prevent them diverging from the length of the data items that they transfer.
>
> 3.4.5.1 below.
> >> When a responder receives a chunk with an inappropriate length, it
> SHOULD return an ERR_CHUNK error and MUST do so if the length is smaller
> than that of the item being transferred.
>
> > The value in this field MUST be the byte offset  of a DDP-eligible data
> item (or zero to accommodate the case discussed in Section 3.5.3).  As a
> result. it will always be evenly divisible by four.
> >> When a responder receives a Write chunk whose length is smaller than
> the data item to be transferred, it MUST, except in the case of a Write
> chunk with zero segments, return the error ERR_CHUNK.
>
> > In Section 4.5.2, the first paragraph needs to be revised and
> clarified.  Suggest replacing it by the following paragrphs:
> processing the incoming Transport stream.  One example of such an error is
> an invalid value in the rdma_proc field. If the rdma_vers field contains a
> recognized value, but an XDR parsing error occurs, the Responder MUST reply
> with an RDMA_ERROR procedure and set the rdma_err value to ERR_CHUNK.
>
> RDMA_NOMSG message where the Read list, Write list, and Reply chunk are all
> marked as not present, a case in which the value of the rdma_xid field does
> not match the value of the XID field in the accompanying RPC message, a
> Read chunk position which is not valid, and  a chunk whose length is not
> valid.  In such cases, the Responder MUST also reply with an RDMA_ERROR
> procedure and set the rdma_err value to ERR_CHUNK.
>
> --
> Chuck Lever
>
>
>
>

---

From: Chuck Lever <[email-masked]>
Date: Tue, 27 Nov 2018 10:47:57 -0500

> On Nov 27, 2018, at 7:54 AM, David Noveck <[email-masked]> wrote:
> 
> Given the following "Whoops", it appears we seem to be in a agreement about 4.5.2.  I'll prepare a draft erratta.
> 
> With regard to 3.4.4:
> 
> If that is the intention, the existing text doesn't really convey it, since it refers to the

The maximum size of an RPC message is the upper bound on the size of
a data item. Transitively, this also places a bound on the size of a
chunk. I'm not sure why that is controversial.

This text was added during IESG review, thus I'm not excited about
altering it significantly. Clarifications are OK.

> Also, this text assumes, as does much of rfc5666 that the client and server are
> each doing the right thing and there is little discussion of what to do if the

Agreed, there is no discussion here of recourse. It is assumed that
the reader is already familiar with the section on error handling.
Perhaps a forward reference to that section can be added.

> I'm not sure what the advice is.  The phrase "sanity check" does not tell me what my
> I think that is pretty much what my text says.   It does restrict itself  to DDP-eligible
> data items since:
> 	• There has to be a clear definition of those
> 	• They cannot nest.
> 	• They are the only ones which affect the size of chunks.
> > The
> I only put in the "SHOULD" because there are ("old") clients in existence that do
> not doing it puts interoperability in question.   Sure there are buggy non-compliant
> document the existence of particular bugs.  Even more troubling is putting this
> in an errata, somehow implying that the absense of such bug reports in RFC8166
> I think the alternative, of making the transport level code aware of XDR item
> structure is simpler.   Either one is, if truth be told, really a layeing violation,
> but we are where we are and every implemenation will need to pass information
> a single error.   I think we should be flexible if necessary.  So one question is 

If we were inventing this protocol from scratch we would have the
freedom to restrict the server response as you propose. However, there
are existing implementations that do perform chunk sanity checking in
the upper layer decoder, and will return GARBAGE_ARGS in this case.
The errata text must permit this behavior.

> Yes, But there is no suggestion that irt t might do nothing.   The question is whether it report an error or 
> tries to deal, as best it can with something arguable malformed.  Does something need to be added
> to clarify this? 
> 
> I asssume that when it checks for this, it returns ERR_CHUNK if the check fails.  Does it?

The current Linux NFS/RDMA server implementation returns ERR_CHUNK in this
case. Documenting comment says:

711 /* Given the client-provided Write and Reply chunks, the server was not
712  * able to form a complete reply. Return an RDMA_ERROR message so the
713  * client can retire this RPC transaction. As above, the Send completion
714  * routine releases payload pages that were part of a previous RDMA Write.

--
Chuck Lever

---

From: David Noveck <[email-masked]>
Date: Tue, 27 Nov 2018 11:07:47 -0500

> chunk.

Yes, but it isn't the appropriate/relevant bound.   We don't want the
server to
something different if the chunk is bigger than maximum size of the data
message
than it does if the chunk is bigger than the data itembut smaller than the
maximum
size of a message.  The chunk is too big in either case.

> I'm not sure why that is controversial.

I'm not sure either.

On Tue, Nov 27, 2018 at 10:48 AM Chuck Lever <[email-masked]> wrote:

> > Given the following "Whoops", it appears we seem to be in a agreement
> about 4.5.2.  I'll prepare a draft erratta.
> >
> > With regard to 3.4.4:
> >
> > Yes, But there is no suggestion that irt t might do nothing.   The
> question is whether it report an error or
> > > What follows is a revised version of the changes suggested for RFC
> 8166, to include a treatment of chunk-related errors.
> > >
> > > A new paragraph needs to be added to section 3.4.5 following the
> current second paragraph.  I suggest that it read as follows:
> > >> The length of a Read chunk is the sum of the length of all the
> segments within it.   The length of a Read chunk  MUST be no smaller than
> than the DDP-eligible data item that it is used to transfer and SHOULD only
> be larger as nceesary to provide necessary round-up, as described in
> Section 3.4.5.1 below.
> >
> data item (or zero to accommodate the case discussed in Section 3.5.3).  As
> a result. it will always be evenly divisible by four.

---

From: Chuck Lever <[email-masked]>
Date: Tue, 27 Nov 2018 11:10:00 -0500

> On Nov 27, 2018, at 11:07 AM, David Noveck <[email-masked]> wrote:
> 
> Yes, but it isn't the appropriate/relevant bound.   We don't want the server to 
> something different if the chunk is bigger than maximum size of the data message
> than it does if the chunk is bigger than the data itembut smaller than the maximum

A single chunk can be the size of an RPC message.

The smaller upper bounds you refer to apply only to normal Read and Write
chunks. A PZRC or a Reply chunk can be larger than a single data item.

--
Chuck Lever

---

From: David Noveck <[email-masked]>
Date: Tue, 27 Nov 2018 11:31:44 -0500

> A single chunk can be the size of an RPC message.

I think there also cases in which it can be larger than the size of an
RPC message.    See below.  This is starting to make my head hurt :-)

> chunks.

True.

> A PZRC or a Reply chunk can be larger than a single data item.

But a Reply chunk can be larger than the reply it is to be used to
transfer.  It pretty much
has to be given that the requester cannot predict many elements of the
response.   The
question then is if the server should (or is obligated to) treat reply
chunks that, for example,
exceed the maximum reply size for the session differently.   i don't know
of any servers that
do so.

For the PZRC, the is no good reason for the chunk to be larger than the
message but I
don't see any need for the server to reject these with ERR_CHUNK If they
are a bit too large.
RFC8166 didn't say anything about this, but that is as expected.   If an
enormous PZRC
(gigabyes long) were presented , I coululd see a server shortening it to a
reasonable uppper
bound to avoid a DOS attack, but i don't see a need to return an
ERR_CHUNK.  if there are
servers that do, then it's a whole new ballgame.  :-(

---

From: Chuck Lever <[email-masked]>
Date: Tue, 27 Nov 2018 11:37:32 -0500

> On Nov 27, 2018, at 11:31 AM, David Noveck <[email-masked]> wrote:
> 
> 
> But a Reply chunk can be larger than the reply it is to be used to transfer.  It pretty much
> has to be given that the requester cannot predict many elements of the response.   The
> question then is if the server should (or is obligated to) treat reply chunks that, for example,
> exceed the maximum reply size for the session differently.   i don't know of any servers that
> do so.

Linux server will complain if there are too many segments or the
chunk is larger than the maximum expected size of an RPC message
in the upper layer protocol.

I don't think there's anything in RFC 8166 says a server has to
respond this way. I don't think there's a need to be specific
about it; it's an implementation quality issue IMO.

> For the PZRC, the is no good reason for the chunk to be larger than the message but I
> don't see any need for the server to reject these with ERR_CHUNK If they are a bit too large.
> RFC8166 didn't say anything about this, but that is as expected.   If an enormous PZRC
> (gigabyes long) were presented , I coululd see a server shortening it to a reasonable uppper 
> bound to avoid a DOS attack, but i don't see a need to return an ERR_CHUNK.  if there are 

A DOS was precisely the concern of the IESG reviewer.

But the point is, the server needs a way to detect a bogusly large
PZRC, so that it can respond appropriately (by not reading the
extra segments, or by responding with an ERR_CHUNK, or by sending
up red sparks with its wand).

--
Chuck Lever