Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NFS: Add a minimum support for NFSv4 #1100

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sikarash
Copy link

@sikarash sikarash commented Nov 10, 2023

tcpdump does not support NFSv4 and prints out wrong NFS data which always be "getattr" for NFSv4, because both NFSv4 COMPOUND procedure and NFSv2/3 GETATTR procedure are the same value '1'.

This is a minimum support for NFSv4, which detects NFSv4 and can print out NULL procedure or COMPOUND procedure at least.

Log of current no-NFSv4 support:

  NFS4_OK case
    18:03:38.718964 IP client.815 > server.nfs: Flags [P.], seq 192:312, ack 177, win 22430, options [nop,nop,TS val 2190058497 ecr 3789310763], length 120: NFS request xid 1660218411 116 getattr fh 0,1/53
    18:03:38.719279 IP server.nfs > client.815: Flags [P.], seq 177:261, ack 312, win 3333, options [nop,nop,TS val 3789310764 ecr 2190058497], length 84: NFS reply xid 1660218411 reply ok 80 getattr NON 1 ids 0/159 sz -719751424
  NFS4ERR_NOENT case
    18:03:44.707900 IP client.815 > server.nfs: Flags [P.], seq 312:512, ack 261, win 22430, options [nop,nop,TS val 2190064486 ecr 3789310864], length 200: NFS request xid 1676995627 196 getattr fh 0,1/53
    18:03:44.708575 IP server.nfs > client.815: Flags [P.], seq 261:361, ack 512, win 3333, options [nop,nop,TS val 3789316753 ecr 2190064486], length 100: NFS reply xid 1676995627 reply ok 96 getattr ERROR: No such file or directory

Log of this minimum NFSv4 support:

  NFS4_OK case
    18:06:18.975026 IP client.815 > server.nfs: Flags [P.], seq 488:608, ack 549, win 22430, options [nop,nop,TS val 2190218753 ecr 3789464067], length 120: NFS request xid 1861545003 116 v4 compound
    18:06:18.975276 IP server.nfs > client.815: Flags [P.], seq 549:633, ack 608, win 3333, options [nop,nop,TS val 3789471020 ecr 2190218753], length 84: NFS reply xid 1861545003 reply ok 80 v4 compound
  NFS4ERR_NOENT case
    17:51:08.684565 IP client.815 > server.nfs: Flags [P.], seq 432:632, ack 345, win 22430, options [nop,nop,TS val 2189308462 ecr 3788560729], length 200: NFS request xid 888466475 196 v4 compound
    17:51:08.685081 IP server.nfs > client.815: Flags [P.], seq 345:445, ack 632, win 3333, options [nop,nop,TS val 3788560730 ecr 2189308462], length 100: NFS reply xid 888466475 reply ok 96 v4 compound ERROR: No such file or directory

@sikarash
Copy link
Author

sikarash commented Jan 5, 2024

@fxlb
I added new test case "nfs-v4".

@infrastation
Copy link
Member

Thank you for waiting. I had a look at the proposed changes. The code style could be a bit better using tok2str(), but a more important point is whether it is correct to use parserep() and parsestatus() on a NFS version 4 packet. I looked into RFCs 7530 and 7531, but could not immediately relate the code with prose that does not seem to use conventional packet diagrams. Could you point to a particular section of a particular document that would make it easier to understand?

@sikarash
Copy link
Author

sikarash commented Feb 13, 2024

Thanks for review.

... whether it is correct to use parserep() and parsestatus() on a NFS version 4 packet. I looked into RFCs 7530 and 7531, but could not immediately relate the code with prose that does not seem to use conventional packet diagrams. Could you point to a particular section of a particular document that would make it easier to understand?

RFC 7531 defines the reply format for a compound request as

/// struct COMPOUND4res {
///         nfsstat4        status;
///         utf8str_cs      tag;
///         nfs_resop4      resarray<>;
/// };

The first four octets, "status", shall be one of the followings:

/// /*
///  * Error status
///  */
/// enum nfsstat4 {
///  NFS4_OK                = 0,    /* everything is okay       */
///  NFS4ERR_PERM           = 1,    /* caller not privileged    */
///  NFS4ERR_NOENT          = 2,    /* no such file/directory   */
///  NFS4ERR_IO             = 5,    /* hard I/O error           */
///  NFS4ERR_NXIO           = 6,    /* no such device           */
///  NFS4ERR_ACCESS         = 13,   /* access denied            */
///  NFS4ERR_EXIST          = 17,   /* file already exists      */
            ...

Current (static const struct tok) status2str[] is not enough for NFSv4, however,
it looks good for minimum support.

Or would you need more real support for NFSv4?

tcpdump does not support NFSv4 and prints out wrong NFS data which
always be "getattr" for NFSv4, because both NFSv4 COMPOUND procedure
and NFSv2/3 GETATTR procedure are the same value '1'.

This is a minimum support for NFSv4, which detects NFSv4 and can print
out NULL procedure or COMPOUND procedure at least.

Log of current no-NFSv4 support:
  NFS4_OK case
    18:03:38.718964 IP client.815 > server.nfs: Flags [P.], seq 192:312, ack 177, win 22430, options [nop,nop,TS val 2190058497 ecr 3789310763], length 120: NFS request xid 1660218411 116 getattr fh 0,1/53
    18:03:38.719279 IP server.nfs > client.815: Flags [P.], seq 177:261, ack 312, win 3333, options [nop,nop,TS val 3789310764 ecr 2190058497], length 84: NFS reply xid 1660218411 reply ok 80 getattr NON 1 ids 0/159 sz -719751424
  NFS4ERR_NOENT case
    18:03:44.707900 IP client.815 > server.nfs: Flags [P.], seq 312:512, ack 261, win 22430, options [nop,nop,TS val 2190064486 ecr 3789310864], length 200: NFS request xid 1676995627 196 getattr fh 0,1/53
    18:03:44.708575 IP server.nfs > client.815: Flags [P.], seq 261:361, ack 512, win 3333, options [nop,nop,TS val 3789316753 ecr 2190064486], length 100: NFS reply xid 1676995627 reply ok 96 getattr ERROR: No such file or directory

Log of this minimum NFSv4 support:
  NFS4_OK case
    18:06:18.975026 IP client.815 > server.nfs: Flags [P.], seq 488:608, ack 549, win 22430, options [nop,nop,TS val 2190218753 ecr 3789464067], length 120: NFS request xid 1861545003 116 v4 compound
    18:06:18.975276 IP server.nfs > client.815: Flags [P.], seq 549:633, ack 608, win 3333, options [nop,nop,TS val 3789471020 ecr 2190218753], length 84: NFS reply xid 1861545003 reply ok 80 v4 compound
  NFS4ERR_NOENT case
    17:51:08.684565 IP client.815 > server.nfs: Flags [P.], seq 432:632, ack 345, win 22430, options [nop,nop,TS val 2189308462 ecr 3788560729], length 200: NFS request xid 888466475 196 v4 compound
    17:51:08.685081 IP server.nfs > client.815: Flags [P.], seq 345:445, ack 632, win 3333, options [nop,nop,TS val 3788560730 ecr 2189308462], length 100: NFS reply xid 888466475 reply ok 96 v4 compound ERROR: No such file or directory

Signed-off-by: Seiichi Ikarashi <[email protected]>
A test for NFSv4 support.

Signed-off-by: Seiichi Ikarashi <[email protected]>
Support NFSv4 errors, and update "nfs-v4" test case to include NFSv4 NULL procedure.
@sikarash
Copy link
Author

sikarash commented Feb 15, 2024

Hi @infrastation

Modified not to use NFSv3 errors but to use NFSv4 errors.
Also fixed not to check the reply status for NULL procedure because it has no status.

... The code style could be a bit better using tok2str()

Modified to use tok2str() for showing NFSv4 procedures.

Add support for showing NFSv4 minor version and numops from the COMPOUND request.
Also fix some tab-indent bug in interp_reply().
@sikarash
Copy link
Author

Added one more commit to show the NFSv4 minor version and the number of ops from COMPOUND requests.

@sikarash
Copy link
Author

sikarash commented Feb 19, 2024

Spec references:

As for the COMPOUND procedure:

RFC 7530 14.1. COMPOUND Procedure

   The basic structure of the COMPOUND procedure is:

   +-----+--------------+--------+-----------+-----------+-----------+--
   | tag | minorversion | numops | op + args | op + args | op + args |
   +-----+--------------+--------+-----------+-----------+-----------+--

   and the reply's structure is:

     +------------+-----+--------+-----------------------+--
     |last status | tag | numres | status + op + results |
     +------------+-----+--------+-----------------------+--

RFC 7530 15.2. Procedure 1: COMPOUND - COMPOUND Operations

   struct COMPOUND4args {
           utf8str_cs      tag;
           uint32_t        minorversion;
           nfs_argop4      argarray<>;
   };
   struct COMPOUND4res {
           nfsstat4        status;
           utf8str_cs      tag;
           nfs_resop4      resarray<>;
   };

utf8str_cs is defined in RFC 7530 2.1. Basic Data Types

   | utf8str_cs      | typedef utf8string utf8str_cs;                  |
   |                 |                                                 |
   |                 | Case-sensitive UTF-8 string.                    |
   | utf8string      | typedef opaque utf8string<>;                    |
   |                 |                                                 |
   |                 | UTF-8 encoding for strings.                     |

opaque is defined in RFC 4506 4.10. Variable-Length Opaque Data

            0     1     2     3     4     5   ...
         +-----+-----+-----+-----+-----+-----+...+-----+-----+...+-----+
         |        length n       |byte0|byte1|...| n-1 |  0  |...|  0  |
         +-----+-----+-----+-----+-----+-----+...+-----+-----+...+-----+
         |<-------4 bytes------->|<------n bytes------>|<---r bytes--->|
                                 |<----n+r (where (n+r) mod 4 = 0)---->|
                                                  VARIABLE-LENGTH OPAQUE

As for the NULL procedure:

RFC 7530 15.1. Procedure 0: NULL - No Operation

15.1.2. ARGUMENT

 void;

15.1.3. RESULT

 void;

@sikarash
Copy link
Author

sikarash commented Apr 6, 2024

Hi, @infrastation @fxlb

Would you please review the codes?
This is an enhancement indeed, however, is also a kind of bug fix for wrong NFS outputs I described at first above.
I suppose that a lot of users do not know they are seeing wrong NFS data and misunderstanding the communications, like me before.

@infrastation
Copy link
Member

Thank you for providing the reference documentation. Please have a bit more patience as there is other, more important work that needs to be done now. I hope in one or two weeks it will be a much better time to review this again.

@sikarash
Copy link
Author

sikarash commented Jul 3, 2024

Hi @infrastation @fxlb
Could you review this pull request?
If something is not clear, I'll explain.
I recently found another case that someone misunderstood NFSv4 traffic as GETATTR procedures with this bug.

@infrastation
Copy link
Member

Unfortunately, this and other improvements will have to wait a little bit more due to backlog. Thank you for understanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants