Should truncated READDIR replies return -EIO?

Should truncated READDIR replies return -EIO?

[Jeff Layton]

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 <jlayton@redhat.com>
---
 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

Re: Should truncated READDIR replies return -EIO?

[Trond Myklebust]

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

Re: Should truncated READDIR replies return -EIO?

[Trond Myklebust]

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

Re: Should truncated READDIR replies return -EIO?

[Peter Staubach]

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

Re: Should truncated READDIR replies return -EIO?

[Peter Staubach]

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

Re: Should truncated READDIR replies return -EIO?

[Jeff Layton]

On Fri, 08 Feb 2008 10:39:50 -0500
Peter Staubach <staubach@redhat.com> 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 <jlayton@redhat.com>

Re: Should truncated READDIR replies return -EIO?

[Chuck Lever]

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

Re: Should truncated READDIR replies return -EIO?

[Peter Staubach]

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

Re: Should truncated READDIR replies return -EIO?

[Chuck Lever]

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

Re: Should truncated READDIR replies return -EIO?

[Jeff Layton]

On Fri, 08 Feb 2008 10:13:16 -0500
Trond Myklebust <trond.myklebust@fys.uio.no> 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 <jlayton@redhat.com>

Re: Should truncated READDIR replies return -EIO?

[Trond Myklebust]

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

Re: Should truncated READDIR replies return -EIO?

[Jeff Layton]

On Fri, 08 Feb 2008 11:13:07 -0500
Trond Myklebust <trond.myklebust@fys.uio.no> 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 <jlayton@redhat.com>

Re: Should truncated READDIR replies return -EIO?

[Jeff Layton]

On Fri, 08 Feb 2008 11:13:07 -0500
Trond Myklebust <trond.myklebust@fys.uio.no> 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.

Perhaps we can just narrow down this special case so that this server
still works, but we can return a proper error when we get other bogus
packets? We currently have this:

	if (!nr && (entry[0] != 0 || entry[1] == 0))

...but if nr==0, then I think entry[0] must be 0 as well. So, I'm
guessing that the server always set value_follows=0 and EOF=0. Is that
correct?

If so, what about something like the following patch? This should
hopefully make it so that packets that are badly sized return -EIO, but
the special case you mention should continue to work.

Note that this patch is just an RFC, untested and v2-only. If this
seems reasonable, I can do similar patches for later revs though my
suggestion would be that we remove this hackery from v4 (and maybe v3
if it's not truly needed there).

I also added some comments to clarify the logic.

>From fd52250cebdaeabdd6f6d561f3ffed7e0e1cdc74 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@redhat.com>
Date: Tue, 12 Feb 2008 08:19:43 -0500
Subject: [PATCH] NFS: clean up short packet handling for NFSv2

Try to make sure that bogus responses with no entries return error.
Also try to preserve existing workaround for servers that send empty
responses with the EOF marker unset. Finally, add some comments to
clarify the logic in nfs_xdr_readdirres().

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/nfs2xdr.c |   33 ++++++++++++++++++++++++++-------
 1 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/nfs2xdr.c b/fs/nfs/nfs2xdr.c
index 1f7ea67..008caec 100644
--- a/fs/nfs/nfs2xdr.c
+++ b/fs/nfs/nfs2xdr.c
@@ -452,6 +452,11 @@ nfs_xdr_readdirres(struct rpc_rqst *req, __be32 *p, void *dummy)
 	kaddr = p = kmap_atomic(*page, KM_USER0);
 	end = (__be32 *)((char *)p + pglen);
 	entry = p;
+
+	/* Make sure the packet actually has a value_follows and EOF entry */
+	if ((entry + 1) > end)
+		goto short_pkt;
+
 	for (nr = 0; *p++; nr++) {
 		if (p + 2 > end)
 			goto short_pkt;
@@ -467,18 +472,32 @@ nfs_xdr_readdirres(struct rpc_rqst *req, __be32 *p, void *dummy)
 			goto short_pkt;
 		entry = p;
 	}
-	if (!nr && (entry[0] != 0 || entry[1] == 0))
-		goto short_pkt;
+
+	/*
+	 * Apparently some server sends responses that are a valid size, but
+	 * contain no entries, and have value_follows==0 and EOF==0. For
+	 * those, just set the EOF marker.
+	 */
+	if (!nr && entry[1] == 0) {
+		dprintk("NFS: readdir reply truncated!\n");
+		entry[1] = 1;
+	}
  out:
 	kunmap_atomic(kaddr, KM_USER0);
 	return nr;
  short_pkt:
+	/*
+	 * When we get a short packet there are 2 possibilities. We can
+	 * return an error, or fix up the response to look like a valid
+	 * response and return what we have so far. If there are no
+	 * entries and the packet was short, then return -EIO. If there
+	 * are valid entries in the response, return them and pretend that
+	 * the call was successful, but incomplete. The caller can retry the
+	 * readdir starting at the last cookie.
+	 */
 	entry[0] = entry[1] = 0;
-	/* truncate listing ? */
-	if (!nr) {
-		dprintk("NFS: readdir reply truncated!\n");
-		entry[1] = 1;
-	}
+	if (!nr)
+		nr = -errno_NFSERR_IO;
 	goto out;
 err_unmap:
 	nr = -errno_NFSERR_IO;
-- 
1.5.3.8

Re: Should truncated READDIR replies return -EIO?

[Trond Myklebust]

On Tue, 2008-02-12 at 08:20 -0500, Jeff Layton wrote:
> On Fri, 08 Feb 2008 11:13:07 -0500
> Trond Myklebust <trond.myklebust@fys.uio.no> 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.
> 
> Perhaps we can just narrow down this special case so that this server
> still works, but we can return a proper error when we get other bogus
> packets? We currently have this:
> 
> 	if (!nr && (entry[0] != 0 || entry[1] == 0))
> 
> ...but if nr==0, then I think entry[0] must be 0 as well. So, I'm
> guessing that the server always set value_follows=0 and EOF=0. Is that
> correct?
> 
> If so, what about something like the following patch? This should
> hopefully make it so that packets that are badly sized return -EIO, but
> the special case you mention should continue to work.

Yup. This looks like it should cover most known cases...

Cheers
  Trond

Re: Should truncated READDIR replies return -EIO?

[Jeff Layton]

On Tue, 12 Feb 2008 19:13:03 -0500
Trond Myklebust <trond.myklebust@fys.uio.no> wrote:

> 
> On Tue, 2008-02-12 at 08:20 -0500, Jeff Layton wrote:
> > On Fri, 08 Feb 2008 11:13:07 -0500
> > Trond Myklebust <trond.myklebust@fys.uio.no> 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.
> > 
> > Perhaps we can just narrow down this special case so that this server
> > still works, but we can return a proper error when we get other bogus
> > packets? We currently have this:
> > 
> > 	if (!nr && (entry[0] != 0 || entry[1] == 0))
> > 
> > ...but if nr==0, then I think entry[0] must be 0 as well. So, I'm
> > guessing that the server always set value_follows=0 and EOF=0. Is that
> > correct?
> > 
> > If so, what about something like the following patch? This should
> > hopefully make it so that packets that are badly sized return -EIO, but
> > the special case you mention should continue to work.
> 
> Yup. This looks like it should cover most known cases...
> 
> Cheers
>   Trond
> 

Good. I still need to test this out, but was wondering what your
thoughts are on keeping the client-side hack in place for NFSv3 and
NFSv4? Should we remove it from either or both of the later NFS
versions?

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>