Bug 463720
| Summary: | nfsd v3: encode_fattr3() may get invalid inode attributes. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Red Hat Enterprise Linux 4 | Reporter: | Wengang Wang <wen.gang.wang> | ||||||||
| Component: | kernel | Assignee: | Ric Wheeler <rwheeler> | ||||||||
| Status: | CLOSED WONTFIX | QA Contact: | Martin Jenner <mjenner> | ||||||||
| Severity: | medium | Docs Contact: | |||||||||
| Priority: | medium | ||||||||||
| Version: | 4.9 | CC: | greg.marsden, jlayton, rwheeler, steved, wangwengang1976, wen.gang.wang | ||||||||
| Target Milestone: | rc | ||||||||||
| Target Release: | --- | ||||||||||
| Hardware: | All | ||||||||||
| OS: | Linux | ||||||||||
| Whiteboard: | |||||||||||
| Fixed In Version: | Doc Type: | Bug Fix | |||||||||
| Doc Text: | Story Points: | --- | |||||||||
| Clone Of: | Environment: | ||||||||||
| Last Closed: | 2012-06-20 13:19:33 UTC | Type: | --- | ||||||||
| Regression: | --- | Mount Type: | --- | ||||||||
| Documentation: | --- | CRM: | |||||||||
| Verified Versions: | Category: | --- | |||||||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||||||
| Embargoed: | |||||||||||
| Attachments: |
|
||||||||||
Reassigning to kernel component. Regards, Phil 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? 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). 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. 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. I suddenly noticed I was updating the bug with different account, sorry. wangwengang1976 is wen.gang.wang 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. 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. 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. Created attachment 328723 [details]
new patch
new patch posted. please have it included in the next release if it's suitable. 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.
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. |
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.