Bug 463720 - nfsd v3: encode_fattr3() may get invalid inode attributes.
nfsd v3: encode_fattr3() may get invalid inode attributes.
Status: CLOSED WONTFIX
Product: Red Hat Enterprise Linux 4
Classification: Red Hat
Component: kernel (Show other bugs)
4.9
All Linux
medium Severity medium
: rc
: ---
Assigned To: Ric Wheeler
Martin Jenner
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-24 04:34 EDT by Wengang Wang
Modified: 2012-06-20 09:19 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-06-20 09:19:33 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
the patch that fixes the problem above (7.90 KB, patch)
2008-09-24 04:34 EDT, Wengang Wang
no flags Details | Diff
new patch (2.51 KB, patch)
2009-01-12 06:56 EST, Wengang Wang
no flags Details | Diff
the correct new patch (1.14 KB, patch)
2009-01-12 07:38 EST, Wengang Wang
no flags Details | Diff

  None (edit)
Description Wengang Wang 2008-09-24 04:34:57 EDT
Created attachment 317571 [details]
the patch that fixes the problem above

for nfsd v3.
the encode_fattr3() function calls vfs_getattr64(), but it doesn't check the return value.
so, in case of vfs_getattr64() return an error, the inode attributes are invalid.

I wrote a patch for this problem.
please have it included in next release if it's appropriate.
Comment 1 Phil Knirsch 2008-09-24 05:44:28 EDT
Reassigning to kernel component.

Regards, Phil
Comment 2 Jeff Layton 2009-01-05 09:16:24 EST
I don't think this patch is what we want. If the call succeeds, but we're unable to get the attributes for the file, I don't think we want return an error. We just want to not send any attributes.

Did you actually hit this problem in the field, or is this just something you noticed via inspection?
Comment 3 wengang wang 2009-01-05 21:05:15 EST
according to http://www.faqs.org/rfcs/rfc1813.html, for GETATTR if we return OK  fattr3 must follows.
we don't comply it?

I hitted this problem while the backend FS is a clustered FS(updating local file attributes failed).
Comment 4 Peter Staubach 2009-01-06 09:38:24 EST
In Comment #2, Jeff is correct.  For any operation other than GETATTR,
if we are unable to retrieve attributes, then we do not want to return
an error.  We want to return the status from the specific operation,
not any incidental errors that may occur from attempting to retrieve
the attributes.

That said, if an error does occur when attempting to retrieve attributes,
then we can not return attributes.  For most operations, we can turn off
either the pre_op_attr or the post_op_attr, whether part of a wcc_data
or not.

The GETATTR operation is the one operation which must return attributes
or return an error indicating why the attributes could not be returned.

Thus, I think that most of the patch which was attached is incorrect.
We definitely do not want to return an error in most of those cases
if an error occurs while attempting to retrieve the attributes.

We should do 5 things though.  1) Make sure that the GETATTR code
correctly handles failures.  2) Make sure that all of the other
operations correctly handle failures.  3) Ensure that the client
correctly handles cases where either the pre_op_attr is not
returned.  4) Ensure that the client correctly handles cases where
the post_op_attr is not returned.  5) Ensure that the client correctly
handles cases where GETATTR returns an error.
Comment 5 Jeff Layton 2009-01-06 10:02:03 EST
Thanks Peter,
  That's what I intended to say. The part that makes the GETATTR code correctly handle errors is probably fine (though I haven't looked at it very closely). For almost all of the other cases however they will need to be fixed.

Regarding Peter's comment #4 though, the last 3 points deal with client side code and should probably be dealt with in a separate BZ (if they are indeed a problem). The patch proposed here is a server-side patch.
Comment 6 Wengang Wang 2009-01-07 08:35:22 EST
I suddenly noticed I was updating the bug with different account, sorry.
wangwengang1976@gmail.com is wen.gang.wang@oracle.com

I clearly know the idea of Jeff and Peter. it's agiler than what the RFC defined.
while, doing so means we break the RFC standard.
I think the 5 steps Peter suggested works fine when both clients and server are linux. but when clients are HPUX/SOLARIS and they don't do steps 3) ~ 5) well, there will be problems. Or, we can ignore other OS?

thanks,
wengang.
Comment 7 Peter Staubach 2009-01-08 07:54:37 EST
Sorry, with all due respect, it does not appear that you understand how
the attributes in the responses in NFSv3 RPC requests are to be handled.

What I described is _exactly_ what the RFC defines.  I know because I
wrote the bulk of the specification that was reformatted to become rfc1813.

I also happen to know that other operating systems, such as Solaris,
handle the responses as I described.  I know this because I wrote the
bulk of that implementation.  I added code to the Solaris NFSv3 server
to selectively determine whether to return pre_op_attr or post_op_attr
fields and then checked that the client continued to function correctly.

Now, if Linux, because that is the operating system that we are looking
at here, does not comply with the semantics as defined by rfc1813, then
we should modify the implementation.
Comment 8 Wengang Wang 2009-01-08 09:11:27 EST
Aha, Peter you are right.
I didn't see post_op_attr described in rfc1813 very well. Within it, there is also a switch indicating whether attributes are following.

So no doubt your 5 steps works well and I need to repost the patch.

thank you,
wengang.
Comment 9 Wengang Wang 2009-01-12 06:56:33 EST
Created attachment 328723 [details]
new patch
Comment 10 Wengang Wang 2009-01-12 06:58:41 EST
new patch posted. please have it included in the next release if it's suitable.
Comment 11 Wengang Wang 2009-01-12 07:38:57 EST
Created attachment 328727 [details]
the correct new patch

1) add error checking for vfs_getattr64() in encode_fattr3()
2) for post_attr, ignore error when encode_fattr3() fails and returns no attributes.
3) for getattr, return error when encode_fattr3() fails.
Comment 13 Jiri Pallich 2012-06-20 09:19:33 EDT
Thank you for submitting this issue for consideration in Red Hat Enterprise Linux. The release for which you requested us to review is now End of Life. 
Please See https://access.redhat.com/support/policy/updates/errata/

If you would like Red Hat to re-consider your feature request for an active release, please re-open the request via appropriate support channels and provide additional supporting details about the importance of this issue.

Note You need to log in before you can comment on or make changes to this bug.