Recently, I ran across a server-side bug that caused the server to send
truncated READDIR replies. The server would send a valid RPC response to
a READDIR call, but the contents of it were basically missing
(everything after the status).
The server problem had long been patched in mainline kernels, but the
interesting bit was that clients didn't return an error in this
situation. The XDR decoders for readdir calls are supposed to check the
validity of the response, but in this situation it just fudges the
contents of the pagecache to make it look like a completely empty
directory.
Shouldn't the client return an error in this situation? The response
obviously isn't valid so it seems like it shouldn't pretend that it is.
If so, would something like the following patch make sense?
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfs/nfs2xdr.c | 1 +
fs/nfs/nfs3xdr.c | 1 +
fs/nfs/nfs4xdr.c | 1 +
3 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
index 1f7ea67..aa6966a 100644
--- a/fs/nfs/nfs2xdr.c
+++ b/fs/nfs/nfs2xdr.c
@@ -478,6 +478,7 @@ nfs_xdr_readdirres(struct rpc_rqst *req, __be32 *p, void *dummy)
if (!nr) {
dprintk("NFS: readdir reply truncated!\n");
entry[1] = 1;
+ nr = -errno_NFSERR_IO;
}
goto out;
err_unmap:
diff --git a/fs/nfs/nfs3xdr.c b/fs/nfs/nfs3xdr.c
index 3917e2f..04ba525 100644
--- a/fs/nfs/nfs3xdr.c
+++ b/fs/nfs/nfs3xdr.c
@@ -592,6 +592,7 @@ nfs3_xdr_readdirres(struct rpc_rqst *req, __be32 *p, struct nfs3_readdirres *res
if (!nr) {
dprintk("NFS: readdir reply truncated!\n");
entry[1] = 1;
+ nr = -errno_NFSERR_IO;
}
goto out;
err_unmap:
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index db1ed9c..139c9e1 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3544,6 +3544,7 @@ short_pkt:
if (!nr) {
dprintk("NFS: readdir reply truncated!\n");
entry[1] = 1;
+ nr = -errno_NFSERR_IO;
}
goto out;
err_unmap:
--
1.5.3.8
On Fri, 2008-02-08 at 10:04 -0500, Jeff Layton wrote:
> Recently, I ran across a server-side bug that caused the server to send
> truncated READDIR replies. The server would send a valid RPC response to
> a READDIR call, but the contents of it were basically missing
> (everything after the status).
>
> The server problem had long been patched in mainline kernels, but the
> interesting bit was that clients didn't return an error in this
> situation. The XDR decoders for readdir calls are supposed to check the
> validity of the response, but in this situation it just fudges the
> contents of the pagecache to make it look like a completely empty
> directory.
>
> Shouldn't the client return an error in this situation? The response
> obviously isn't valid so it seems like it shouldn't pretend that it is.
> If so, would something like the following patch make sense?
It is quite valid (though silly!) for a server to return a READDIR reply
with no entries. AFAICR there were servers that actually did this at one
point (though I shall refrain from naming and shaming).
So whereas I agree that it might be correct to flag a READDIR reply that
contains no entries due to XDR encoding bugs, I'm not sure that we
should be flagging errors in the case where the XDR is correct.
Cheers
Trond
On Fri, 2008-02-08 at 10:13 -0500, Trond Myklebust wrote:
> So whereas I agree that it might be correct to flag a READDIR reply that
> contains no entries due to XDR encoding bugs, I'm not sure that we
> should be flagging errors in the case where the XDR is correct.
Another useful error change in the readdir code is that we may consider
flagging an ENAMETOOLONG error instead of EIO in the cases where the
filename is too large for us to parse.
Trond
Trond Myklebust wrote:
> On Fri, 2008-02-08 at 10:04 -0500, Jeff Layton wrote:
>
>> Recently, I ran across a server-side bug that caused the server to send
>> truncated READDIR replies. The server would send a valid RPC response to
>> a READDIR call, but the contents of it were basically missing
>> (everything after the status).
>>
>> The server problem had long been patched in mainline kernels, but the
>> interesting bit was that clients didn't return an error in this
>> situation. The XDR decoders for readdir calls are supposed to check the
>> validity of the response, but in this situation it just fudges the
>> contents of the pagecache to make it look like a completely empty
>> directory.
>>
>> Shouldn't the client return an error in this situation? The response
>> obviously isn't valid so it seems like it shouldn't pretend that it is.
>> If so, would something like the following patch make sense?
>>
>
> It is quite valid (though silly!) for a server to return a READDIR reply
> with no entries. AFAICR there were servers that actually did this at one
> point (though I shall refrain from naming and shaming).
>
> So whereas I agree that it might be correct to flag a READDIR reply that
> contains no entries due to XDR encoding bugs, I'm not sure that we
> should be flagging errors in the case where the XDR is correct.
In this case, I believe that the response was malformed. Pretty
much everything after the status was missing, including the EOF
indicator. I would agree that it would be silly to return a
response with no error indicated, no entries, and the eof
indication set to false.
This really boils down to how do we handle malformed responses?
Is there a general policy to retransmit the request? This would
seem to be the right thing because a malformed response would
result from many things including the TCP connection getting
dropped in the middle of receiving the response from a timeout
and other things. However, in this situation, retransmitting
the request would just have resulted in the same, broken response
from the server. This was due to a server bug, which has since
been fixed, but exists still out in nature.
Thanx...
ps
Trond Myklebust wrote:
> On Fri, 2008-02-08 at 10:13 -0500, Trond Myklebust wrote:
>
>
>> So whereas I agree that it might be correct to flag a READDIR reply that
>> contains no entries due to XDR encoding bugs, I'm not sure that we
>> should be flagging errors in the case where the XDR is correct.
>>
>
> Another useful error change in the readdir code is that we may consider
> flagging an ENAMETOOLONG error instead of EIO in the cases where the
> filename is too large for us to parse.
This brings up another question that I've had -- how to correctly
differentiate between XDR decoding errors and issues having to do
with local implementation limitations. I think that it would be
good to be able to make a determination, to properly know how to
do recovery from the situation.
Thanx...
ps
On Fri, 08 Feb 2008 10:13:16 -0500
Trond Myklebust <[email protected]> wrote:
>
> On Fri, 2008-02-08 at 10:04 -0500, Jeff Layton wrote:
> > Recently, I ran across a server-side bug that caused the server to send
> > truncated READDIR replies. The server would send a valid RPC response to
> > a READDIR call, but the contents of it were basically missing
> > (everything after the status).
> >
> > The server problem had long been patched in mainline kernels, but the
> > interesting bit was that clients didn't return an error in this
> > situation. The XDR decoders for readdir calls are supposed to check the
> > validity of the response, but in this situation it just fudges the
> > contents of the pagecache to make it look like a completely empty
> > directory.
> >
> > Shouldn't the client return an error in this situation? The response
> > obviously isn't valid so it seems like it shouldn't pretend that it is.
> > If so, would something like the following patch make sense?
>
> It is quite valid (though silly!) for a server to return a READDIR reply
> with no entries. AFAICR there were servers that actually did this at one
> point (though I shall refrain from naming and shaming).
>
> So whereas I agree that it might be correct to flag a READDIR reply that
> contains no entries due to XDR encoding bugs, I'm not sure that we
> should be flagging errors in the case where the XDR is correct.
>
Ok. In this case there was nothing in the packet after the status code,
so I don't think the XDR was correct. There was no "value follows" entry
and no EOF flag. Wireshark flagged it as a malformed packet.
If I read the spec right, a valid but empty response from the server
should look like:
status code == 0 (ok)
value follows == 0
EOF == 1 (or maybe 0?)
If we get a packet that looks like that and has EOF set to 1, then we
don't "goto short_pkt". That case would be unaffected by this patch.
If it looks like the above, but EOF is 0, then we *do* "goto
short_pkt", and that case would be affected by this. But in that case
we currently reset EOF to 1. The client won't retry the request. I'm
not sure that's what we want to do either...
PS: I have a binary capture if my description isn't clear...
--
Jeff Layton <[email protected]>
On Fri, 2008-02-08 at 10:56 -0500, Jeff Layton wrote:
> If it looks like the above, but EOF is 0, then we *do* "goto
> short_pkt", and that case would be affected by this. But in that case
> we currently reset EOF to 1. The client won't retry the request. I'm
> not sure that's what we want to do either...
That is to deal with the afore-mentioned broken server. I'm not
unwilling to change this (I _hate_ client side fixes for server bugs),
but it's important to note that there may be consequences if we do.
Cheers
Trond
On Fri, 08 Feb 2008 10:39:50 -0500
Peter Staubach <[email protected]> wrote:
> Trond Myklebust wrote:
> > On Fri, 2008-02-08 at 10:04 -0500, Jeff Layton wrote:
> >
> >> Recently, I ran across a server-side bug that caused the server to send
> >> truncated READDIR replies. The server would send a valid RPC response to
> >> a READDIR call, but the contents of it were basically missing
> >> (everything after the status).
> >>
> >> The server problem had long been patched in mainline kernels, but the
> >> interesting bit was that clients didn't return an error in this
> >> situation. The XDR decoders for readdir calls are supposed to check the
> >> validity of the response, but in this situation it just fudges the
> >> contents of the pagecache to make it look like a completely empty
> >> directory.
> >>
> >> Shouldn't the client return an error in this situation? The response
> >> obviously isn't valid so it seems like it shouldn't pretend that it is.
> >> If so, would something like the following patch make sense?
> >>
> >
> > It is quite valid (though silly!) for a server to return a READDIR reply
> > with no entries. AFAICR there were servers that actually did this at one
> > point (though I shall refrain from naming and shaming).
> >
> > So whereas I agree that it might be correct to flag a READDIR reply that
> > contains no entries due to XDR encoding bugs, I'm not sure that we
> > should be flagging errors in the case where the XDR is correct.
>
> In this case, I believe that the response was malformed. Pretty
> much everything after the status was missing, including the EOF
> indicator. I would agree that it would be silly to return a
> response with no error indicated, no entries, and the eof
> indication set to false.
>
> This really boils down to how do we handle malformed responses?
> Is there a general policy to retransmit the request? This would
> seem to be the right thing because a malformed response would
> result from many things including the TCP connection getting
> dropped in the middle of receiving the response from a timeout
> and other things. However, in this situation, retransmitting
> the request would just have resulted in the same, broken response
> from the server. This was due to a server bug, which has since
> been fixed, but exists still out in nature.
>
In the above case, wouldn't the malformed response also have meant that
the RPC layer was malformed (or lower layers even)? In that case, the
kernel would probably retransmit the request anyway, wouldn't it?
This case is odd in that the UDP/RPC layers were consistent length-wise.
The NFS payload was just incomplete...
--
Jeff Layton <[email protected]>
On Fri, 08 Feb 2008 11:13:07 -0500
Trond Myklebust <[email protected]> wrote:
>
> On Fri, 2008-02-08 at 10:56 -0500, Jeff Layton wrote:
>
> > If it looks like the above, but EOF is 0, then we *do* "goto
> > short_pkt", and that case would be affected by this. But in that case
> > we currently reset EOF to 1. The client won't retry the request. I'm
> > not sure that's what we want to do either...
>
> That is to deal with the afore-mentioned broken server. I'm not
> unwilling to change this (I _hate_ client side fixes for server bugs),
> but it's important to note that there may be consequences if we do.
>
Hmm, tough call...
I don't know that the current behavior is actually causing any
problems, per se. If changing it is likely to do so, then maybe leaving
this as it is the best thing, at least for lower NFS versions.
Maybe we should consider changing this behavior for NFSv4? It might be
good not to carry these sort of kludges forward into later revs. Was
this buggy nfs server a v4 server as well?
--
Jeff Layton <[email protected]>
On Feb 8, 2008, at 10:39 AM, Peter Staubach wrote:
> Trond Myklebust wrote:
>> On Fri, 2008-02-08 at 10:04 -0500, Jeff Layton wrote:
>>
>>> Recently, I ran across a server-side bug that caused the server
>>> to send
>>> truncated READDIR replies. The server would send a valid RPC
>>> response to
>>> a READDIR call, but the contents of it were basically missing
>>> (everything after the status).
>>>
>>> The server problem had long been patched in mainline kernels, but
>>> the
>>> interesting bit was that clients didn't return an error in this
>>> situation. The XDR decoders for readdir calls are supposed to
>>> check the
>>> validity of the response, but in this situation it just fudges the
>>> contents of the pagecache to make it look like a completely empty
>>> directory.
>>>
>>> Shouldn't the client return an error in this situation? The response
>>> obviously isn't valid so it seems like it shouldn't pretend that
>>> it is.
>>> If so, would something like the following patch make sense?
>>>
>>
>> It is quite valid (though silly!) for a server to return a READDIR
>> reply
>> with no entries. AFAICR there were servers that actually did this
>> at one
>> point (though I shall refrain from naming and shaming).
>>
>> So whereas I agree that it might be correct to flag a READDIR
>> reply that
>> contains no entries due to XDR encoding bugs, I'm not sure that we
>> should be flagging errors in the case where the XDR is correct.
>
> In this case, I believe that the response was malformed. Pretty
> much everything after the status was missing, including the EOF
> indicator. I would agree that it would be silly to return a
> response with no error indicated, no entries, and the eof
> indication set to false.
>
> This really boils down to how do we handle malformed responses?
> Is there a general policy to retransmit the request? This would
> seem to be the right thing because a malformed response would
> result from many things including the TCP connection getting
> dropped in the middle of receiving the response from a timeout
> and other things. However, in this situation, retransmitting
> the request would just have resulted in the same, broken response
> from the server. This was due to a server bug, which has since
> been fixed, but exists still out in nature.
Replies that are malformed network or RPC level packets are dropped
by the RPC client, and the matching requests are retransmitted by the
RPC client after a timeout. Network events (like your TCP connection
example) result in a malformed RPC level packet that the RPC client
never delivers to the XDR layer, and are thus retransmitted by the
RPC client.
Replies that have malformed XDR are treated by the NFS client as
errors. The problem is the decoders (on Linux) are not terribly
careful about checking the correctness of the server's XDR encoding,
especially in cases like READDIR (Not to mention compound RPCs!)
where the decoding can be complex. Olaf has mentioned the Linux XDR
layer was hand-coded rather than constructed with rpcgen to keep the
decoders simple and efficient.
Network-related corruption is likely to be caught by the lower
layers. I tend to think that malformed XDR is nearly always a
genuine software defect on the server, and thus not worth
retransmitting (especially if it's an idempotent request!).
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
Chuck Lever wrote:
> On Feb 8, 2008, at 10:39 AM, Peter Staubach wrote:
>> Trond Myklebust wrote:
>>> On Fri, 2008-02-08 at 10:04 -0500, Jeff Layton wrote:
>>>
>>>> Recently, I ran across a server-side bug that caused the server to
>>>> send
>>>> truncated READDIR replies. The server would send a valid RPC
>>>> response to
>>>> a READDIR call, but the contents of it were basically missing
>>>> (everything after the status).
>>>>
>>>> The server problem had long been patched in mainline kernels, but the
>>>> interesting bit was that clients didn't return an error in this
>>>> situation. The XDR decoders for readdir calls are supposed to check
>>>> the
>>>> validity of the response, but in this situation it just fudges the
>>>> contents of the pagecache to make it look like a completely empty
>>>> directory.
>>>>
>>>> Shouldn't the client return an error in this situation? The response
>>>> obviously isn't valid so it seems like it shouldn't pretend that it
>>>> is.
>>>> If so, would something like the following patch make sense?
>>>>
>>>
>>> It is quite valid (though silly!) for a server to return a READDIR
>>> reply
>>> with no entries. AFAICR there were servers that actually did this at
>>> one
>>> point (though I shall refrain from naming and shaming).
>>>
>>> So whereas I agree that it might be correct to flag a READDIR reply
>>> that
>>> contains no entries due to XDR encoding bugs, I'm not sure that we
>>> should be flagging errors in the case where the XDR is correct.
>>
>> In this case, I believe that the response was malformed. Pretty
>> much everything after the status was missing, including the EOF
>> indicator. I would agree that it would be silly to return a
>> response with no error indicated, no entries, and the eof
>> indication set to false.
>>
>> This really boils down to how do we handle malformed responses?
>> Is there a general policy to retransmit the request? This would
>> seem to be the right thing because a malformed response would
>> result from many things including the TCP connection getting
>> dropped in the middle of receiving the response from a timeout
>> and other things. However, in this situation, retransmitting
>> the request would just have resulted in the same, broken response
>> from the server. This was due to a server bug, which has since
>> been fixed, but exists still out in nature.
>
>
> Replies that are malformed network or RPC level packets are dropped by
> the RPC client, and the matching requests are retransmitted by the RPC
> client after a timeout. Network events (like your TCP connection
> example) result in a malformed RPC level packet that the RPC client
> never delivers to the XDR layer, and are thus retransmitted by the RPC
> client.
>
> Replies that have malformed XDR are treated by the NFS client as
> errors. The problem is the decoders (on Linux) are not terribly
> careful about checking the correctness of the server's XDR encoding,
> especially in cases like READDIR (Not to mention compound RPCs!) where
> the decoding can be complex. Olaf has mentioned the Linux XDR layer
> was hand-coded rather than constructed with rpcgen to keep the
> decoders simple and efficient.
>
> Network-related corruption is likely to be caught by the lower
> layers. I tend to think that malformed XDR is nearly always a genuine
> software defect on the server, and thus not worth retransmitting
> (especially if it's an idempotent request!).
What happens if a response is interrupted in the middle by the
TCP connection being broken? Is this caught at the RPC layer
and then rejected?
Thanx...
ps
On Feb 8, 2008, at 1:16 PM, Peter Staubach wrote:
> Chuck Lever wrote:
>> On Feb 8, 2008, at 10:39 AM, Peter Staubach wrote:
>>> Trond Myklebust wrote:
>>>> On Fri, 2008-02-08 at 10:04 -0500, Jeff Layton wrote:
>>>>
>>>>> Recently, I ran across a server-side bug that caused the server
>>>>> to send
>>>>> truncated READDIR replies. The server would send a valid RPC
>>>>> response to
>>>>> a READDIR call, but the contents of it were basically missing
>>>>> (everything after the status).
>>>>>
>>>>> The server problem had long been patched in mainline kernels,
>>>>> but the
>>>>> interesting bit was that clients didn't return an error in this
>>>>> situation. The XDR decoders for readdir calls are supposed to
>>>>> check the
>>>>> validity of the response, but in this situation it just fudges the
>>>>> contents of the pagecache to make it look like a completely empty
>>>>> directory.
>>>>>
>>>>> Shouldn't the client return an error in this situation? The
>>>>> response
>>>>> obviously isn't valid so it seems like it shouldn't pretend
>>>>> that it is.
>>>>> If so, would something like the following patch make sense?
>>>>>
>>>>
>>>> It is quite valid (though silly!) for a server to return a
>>>> READDIR reply
>>>> with no entries. AFAICR there were servers that actually did
>>>> this at one
>>>> point (though I shall refrain from naming and shaming).
>>>>
>>>> So whereas I agree that it might be correct to flag a READDIR
>>>> reply that
>>>> contains no entries due to XDR encoding bugs, I'm not sure that we
>>>> should be flagging errors in the case where the XDR is correct.
>>>
>>> In this case, I believe that the response was malformed. Pretty
>>> much everything after the status was missing, including the EOF
>>> indicator. I would agree that it would be silly to return a
>>> response with no error indicated, no entries, and the eof
>>> indication set to false.
>>>
>>> This really boils down to how do we handle malformed responses?
>>> Is there a general policy to retransmit the request? This would
>>> seem to be the right thing because a malformed response would
>>> result from many things including the TCP connection getting
>>> dropped in the middle of receiving the response from a timeout
>>> and other things. However, in this situation, retransmitting
>>> the request would just have resulted in the same, broken response
>>> from the server. This was due to a server bug, which has since
>>> been fixed, but exists still out in nature.
>>
>>
>> Replies that are malformed network or RPC level packets are
>> dropped by the RPC client, and the matching requests are
>> retransmitted by the RPC client after a timeout. Network events
>> (like your TCP connection example) result in a malformed RPC level
>> packet that the RPC client never delivers to the XDR layer, and
>> are thus retransmitted by the RPC client.
>>
>> Replies that have malformed XDR are treated by the NFS client as
>> errors. The problem is the decoders (on Linux) are not terribly
>> careful about checking the correctness of the server's XDR
>> encoding, especially in cases like READDIR (Not to mention
>> compound RPCs!) where the decoding can be complex. Olaf has
>> mentioned the Linux XDR layer was hand-coded rather than
>> constructed with rpcgen to keep the decoders simple and efficient.
>>
>> Network-related corruption is likely to be caught by the lower
>> layers. I tend to think that malformed XDR is nearly always a
>> genuine software defect on the server, and thus not worth
>> retransmitting (especially if it's an idempotent request!).
>
> What happens if a response is interrupted in the middle by the
> TCP connection being broken? Is this caught at the RPC layer
> and then rejected?
As I understand it, xs_tcp_read_request() checks for a truncated TCP
read, and discards the reply by not invoking xprt_complete_rqst().
If the TCP layer stops calling the RPC client back with more bytes on
the socket, then xprt_complete_rqst() is never invoked to mark the
RPC request as complete.
So, ostensibly, the RPC client will discard a partially received RPC
reply and at some later point, time out the pending request and
retransmit it.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com