[PATCH v2 0/4] NFSv1 support
This patch series adds support for NFSv1 and is more or less a rebased version of an older series.
During V1 there was a discussion if it really makes sense to bring more features into the network stack of U-Boot as it is just a bootloader. As TCP support landed I thought I might give this patch series another try.
Thomas RIENOESSL (4): nfs: convert supported_nfs_versions bitfield to an enum nfs: factor out generic reply error handling nfs: handle rpc errors for mount calls net: add NFSv1 support
net/nfs.c | 192 ++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 120 insertions(+), 72 deletions(-)
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
Prep. work to support nfs v1.
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info --- net/nfs.c | 93 +++++++++++++++++++++++++++++++------------------------ 1 file changed, 52 insertions(+), 41 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c index c6a124885e..d1858faaa9 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -76,10 +76,13 @@ static char *nfs_filename; static char *nfs_path; static char nfs_path_buff[2048];
-#define NFSV2_FLAG 1 -#define NFSV3_FLAG 1 << 1 -static char supported_nfs_versions = NFSV2_FLAG | NFSV3_FLAG; +enum nfs_version { + NFS_UNKOWN = 0, + NFS_V2 = 2, + NFS_V3 = 3, +};
+static enum nfs_version choosen_nfs_version = NFS_V3; static inline int store_block(uchar *src, unsigned offset, unsigned len) { ulong newsize = offset + len; @@ -188,10 +191,19 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) rpc_pkt.u.call.prog = htonl(rpc_prog); switch (rpc_prog) { case PROG_NFS: - if (supported_nfs_versions & NFSV2_FLAG) - rpc_pkt.u.call.vers = htonl(2); /* NFS v2 */ - else /* NFSV3_FLAG */ - rpc_pkt.u.call.vers = htonl(3); /* NFS v3 */ + switch (choosen_nfs_version) { + case NFS_V2: + rpc_pkt.u.call.vers = htonl(2); + break; + + case NFS_V3: + rpc_pkt.u.call.vers = htonl(3); + break; + + case NFS_UNKOWN: + /* nothing to do */ + break; + } break; case PROG_PORTMAP: case PROG_MOUNT: @@ -299,10 +311,10 @@ static void nfs_readlink_req(void) p = &(data[0]); p = rpc_add_credentials(p);
- if (supported_nfs_versions & NFSV2_FLAG) { + if (choosen_nfs_version == NFS_V2) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); - } else { /* NFSV3_FLAG */ + } else { /* NFS_V3 */ *p++ = htonl(filefh3_length); memcpy(p, filefh, filefh3_length); p += (filefh3_length / 4); @@ -328,7 +340,7 @@ static void nfs_lookup_req(char *fname) p = &(data[0]); p = rpc_add_credentials(p);
- if (supported_nfs_versions & NFSV2_FLAG) { + if (choosen_nfs_version == NFS_V2) { memcpy(p, dirfh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(fnamelen); @@ -340,7 +352,7 @@ static void nfs_lookup_req(char *fname) len = (uint32_t *)p - (uint32_t *)&(data[0]);
rpc_req(PROG_NFS, NFS_LOOKUP, data, len); - } else { /* NFSV3_FLAG */ + } else { /* NFS_V3 */ *p++ = htonl(NFS_FHSIZE); /* Dir handle length */ memcpy(p, dirfh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); @@ -368,13 +380,13 @@ static void nfs_read_req(int offset, int readlen) p = &(data[0]); p = rpc_add_credentials(p);
- if (supported_nfs_versions & NFSV2_FLAG) { + if (choosen_nfs_version == NFS_V2) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(offset); *p++ = htonl(readlen); *p++ = 0; - } else { /* NFSV3_FLAG */ + } else { /* NFS_V3 */ *p++ = htonl(filefh3_length); memcpy(p, filefh, filefh3_length); p += (filefh3_length / 4); @@ -398,15 +410,15 @@ static void nfs_send(void)
switch (nfs_state) { case STATE_PRCLOOKUP_PROG_MOUNT_REQ: - if (supported_nfs_versions & NFSV2_FLAG) + if (choosen_nfs_version == NFS_V2) rpc_lookup_req(PROG_MOUNT, 1); - else /* NFSV3_FLAG */ + else /* NFS_V3 */ rpc_lookup_req(PROG_MOUNT, 3); break; case STATE_PRCLOOKUP_PROG_NFS_REQ: - if (supported_nfs_versions & NFSV2_FLAG) + if (choosen_nfs_version == NFS_V2) rpc_lookup_req(PROG_NFS, 2); - else /* NFSV3_FLAG */ + else /* NFS_V3 */ rpc_lookup_req(PROG_NFS, 3); break; case STATE_MOUNT_REQ: @@ -531,31 +543,30 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len) switch (ntohl(rpc_pkt.u.reply.astatus)) { case NFS_RPC_SUCCESS: /* Not an error */ break; - case NFS_RPC_PROG_MISMATCH: + case NFS_RPC_PROG_MISMATCH: { /* Remote can't support NFS version */ - switch (ntohl(rpc_pkt.u.reply.data[0])) { - /* Minimal supported NFS version */ - case 3: - debug("*** Warning: NFS version not supported: Requested: V%d, accepted: min V%d - max V%d\n", - (supported_nfs_versions & NFSV2_FLAG) ? - 2 : 3, - ntohl(rpc_pkt.u.reply.data[0]), - ntohl(rpc_pkt.u.reply.data[1])); - debug("Will retry with NFSv3\n"); - /* Clear NFSV2_FLAG from supported versions */ - supported_nfs_versions &= ~NFSV2_FLAG; - return -NFS_RPC_PROG_MISMATCH; - case 4: - default: + const int min = ntohl(rpc_pkt.u.reply.data[0]); + const int max = ntohl(rpc_pkt.u.reply.data[1]); + + if (max < NFS_V2 || max > NFS_V3 || min > NFS_V3) { puts("*** ERROR: NFS version not supported"); debug(": Requested: V%d, accepted: min V%d - max V%d\n", - (supported_nfs_versions & NFSV2_FLAG) ? - 2 : 3, + choosen_nfs_version, ntohl(rpc_pkt.u.reply.data[0]), ntohl(rpc_pkt.u.reply.data[1])); puts("\n"); + choosen_nfs_version = NFS_UNKOWN; + break; } - break; + + debug("*** Warning: NFS version not supported: Requested: V%d, accepted: min V%d - max V%d\n", + choosen_nfs_version, + ntohl(rpc_pkt.u.reply.data[0]), + ntohl(rpc_pkt.u.reply.data[1])); + debug("Will retry with NFSv%d\n", min); + choosen_nfs_version = min; + return -NFS_RPC_PROG_MISMATCH; + } case NFS_RPC_PROG_UNAVAIL: case NFS_RPC_PROC_UNAVAIL: case NFS_RPC_GARBAGE_ARGS: @@ -568,11 +579,11 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len) return -1; }
- if (supported_nfs_versions & NFSV2_FLAG) { + if (choosen_nfs_version == NFS_V2) { if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + NFS_FHSIZE) > len) return -NFS_RPC_DROP; memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE); - } else { /* NFSV3_FLAG */ + } else { /* NFS_V3 */ filefh3_length = ntohl(rpc_pkt.u.reply.data[1]); if (filefh3_length > NFS3_FHSIZE) filefh3_length = NFS3_FHSIZE; @@ -631,7 +642,7 @@ static int nfs_readlink_reply(uchar *pkt, unsigned len) rpc_pkt.u.reply.data[0]) return -1;
- if (!(supported_nfs_versions & NFSV2_FLAG)) { /* NFSV3_FLAG */ + if (choosen_nfs_version == NFS_V3) { nfsv3_data_offset = nfs3_get_attributes_offset(rpc_pkt.u.reply.data); } @@ -692,10 +703,10 @@ static int nfs_read_reply(uchar *pkt, unsigned len) if (!(nfs_offset % ((NFS_READ_SIZE / 2) * 10))) putc('#');
- if (supported_nfs_versions & NFSV2_FLAG) { + if (choosen_nfs_version == NFS_V2) { rlen = ntohl(rpc_pkt.u.reply.data[18]); data_ptr = (uchar *)&(rpc_pkt.u.reply.data[19]); - } else { /* NFSV3_FLAG */ + } else { /* NFS_V3 */ int nfsv3_data_offset = nfs3_get_attributes_offset(rpc_pkt.u.reply.data);
@@ -801,7 +812,7 @@ static void nfs_handler(uchar *pkt, unsigned dest, struct in_addr sip, nfs_state = STATE_UMOUNT_REQ; nfs_send(); } else if (reply == -NFS_RPC_PROG_MISMATCH && - supported_nfs_versions != 0) { + choosen_nfs_version != NFS_UNKOWN) { /* umount */ nfs_state = STATE_UMOUNT_REQ; nfs_send();
On Fri, Mar 10, 2023 at 10:51:52AM +0100, Christian Gmeiner wrote:
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
Prep. work to support nfs v1.
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info
For the series, applied to u-boot/master, thanks!
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info --- net/nfs.c | 94 ++++++++++++++++++++++++++++++------------------------- 1 file changed, 52 insertions(+), 42 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c index d1858faaa9..a2749bae82 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -443,6 +443,54 @@ static void nfs_send(void) Handlers for the reply from server **************************************************************************/
+static int rpc_handle_error(struct rpc_t *rpc_pkt) +{ + if (rpc_pkt->u.reply.rstatus || + rpc_pkt->u.reply.verifier || + rpc_pkt->u.reply.astatus || + rpc_pkt->u.reply.data[0]) { + switch (ntohl(rpc_pkt->u.reply.astatus)) { + case NFS_RPC_SUCCESS: /* Not an error */ + break; + case NFS_RPC_PROG_MISMATCH: { + /* Remote can't support NFS version */ + const int min = ntohl(rpc_pkt->u.reply.data[0]); + const int max = ntohl(rpc_pkt->u.reply.data[1]); + + if (max < NFS_V2 || max > NFS_V3 || min > NFS_V3) { + puts("*** ERROR: NFS version not supported"); + debug(": Requested: V%d, accepted: min V%d - max V%d\n", + choosen_nfs_version, + ntohl(rpc_pkt->u.reply.data[0]), + ntohl(rpc_pkt->u.reply.data[1])); + puts("\n"); + choosen_nfs_version = NFS_UNKOWN; + break; + } + + debug("*** Warning: NFS version not supported: Requested: V%d, accepted: min V%d - max V%d\n", + choosen_nfs_version, + ntohl(rpc_pkt->u.reply.data[0]), + ntohl(rpc_pkt->u.reply.data[1])); + debug("Will retry with NFSv%d\n", min); + choosen_nfs_version = min; + return -NFS_RPC_PROG_MISMATCH; + } + case NFS_RPC_PROG_UNAVAIL: + case NFS_RPC_PROC_UNAVAIL: + case NFS_RPC_GARBAGE_ARGS: + case NFS_RPC_SYSTEM_ERR: + default: /* Unknown error on 'accept state' flag */ + debug("*** ERROR: accept state error (%d)\n", + ntohl(rpc_pkt->u.reply.astatus)); + break; + } + return -1; + } + + return 0; +} + static int rpc_lookup_reply(int prog, uchar *pkt, unsigned len) { struct rpc_t rpc_pkt; @@ -526,6 +574,7 @@ static int nfs_umountall_reply(uchar *pkt, unsigned len) static int nfs_lookup_reply(uchar *pkt, unsigned len) { struct rpc_t rpc_pkt; + int ret;
debug("%s\n", __func__);
@@ -536,48 +585,9 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len) else if (ntohl(rpc_pkt.u.reply.id) < rpc_id) return -NFS_RPC_DROP;
- if (rpc_pkt.u.reply.rstatus || - rpc_pkt.u.reply.verifier || - rpc_pkt.u.reply.astatus || - rpc_pkt.u.reply.data[0]) { - switch (ntohl(rpc_pkt.u.reply.astatus)) { - case NFS_RPC_SUCCESS: /* Not an error */ - break; - case NFS_RPC_PROG_MISMATCH: { - /* Remote can't support NFS version */ - const int min = ntohl(rpc_pkt.u.reply.data[0]); - const int max = ntohl(rpc_pkt.u.reply.data[1]); - - if (max < NFS_V2 || max > NFS_V3 || min > NFS_V3) { - puts("*** ERROR: NFS version not supported"); - debug(": Requested: V%d, accepted: min V%d - max V%d\n", - choosen_nfs_version, - ntohl(rpc_pkt.u.reply.data[0]), - ntohl(rpc_pkt.u.reply.data[1])); - puts("\n"); - choosen_nfs_version = NFS_UNKOWN; - break; - } - - debug("*** Warning: NFS version not supported: Requested: V%d, accepted: min V%d - max V%d\n", - choosen_nfs_version, - ntohl(rpc_pkt.u.reply.data[0]), - ntohl(rpc_pkt.u.reply.data[1])); - debug("Will retry with NFSv%d\n", min); - choosen_nfs_version = min; - return -NFS_RPC_PROG_MISMATCH; - } - case NFS_RPC_PROG_UNAVAIL: - case NFS_RPC_PROC_UNAVAIL: - case NFS_RPC_GARBAGE_ARGS: - case NFS_RPC_SYSTEM_ERR: - default: /* Unknown error on 'accept state' flag */ - debug("*** ERROR: accept state error (%d)\n", - ntohl(rpc_pkt.u.reply.astatus)); - break; - } - return -1; - } + ret = rpc_handle_error(&rpc_pkt); + if (ret) + return ret;
if (choosen_nfs_version == NFS_V2) { if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + NFS_FHSIZE) > len)
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info --- net/nfs.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c index a2749bae82..21cae52f35 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -524,6 +524,7 @@ static int rpc_lookup_reply(int prog, uchar *pkt, unsigned len) static int nfs_mount_reply(uchar *pkt, unsigned len) { struct rpc_t rpc_pkt; + int ret;
debug("%s\n", __func__);
@@ -534,11 +535,9 @@ static int nfs_mount_reply(uchar *pkt, unsigned len) else if (ntohl(rpc_pkt.u.reply.id) < rpc_id) return -NFS_RPC_DROP;
- if (rpc_pkt.u.reply.rstatus || - rpc_pkt.u.reply.verifier || - rpc_pkt.u.reply.astatus || - rpc_pkt.u.reply.data[0]) - return -1; + ret = rpc_handle_error(&rpc_pkt); + if (ret) + return ret;
fs_mounted = 1; /* NFSv2 and NFSv3 use same structure */ @@ -794,6 +793,10 @@ static void nfs_handler(uchar *pkt, unsigned dest, struct in_addr sip, /* just to be sure... */ nfs_state = STATE_UMOUNT_REQ; nfs_send(); + } else if (reply == -NFS_RPC_PROG_MISMATCH && + choosen_nfs_version != NFS_UNKOWN) { + nfs_state = STATE_MOUNT_REQ; + nfs_send(); } else { nfs_state = STATE_LOOKUP_REQ; nfs_send();
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
NFSv1 support added by Christian Gmeiner, Thomas Rienoessl, September 27, 2018. As of now, NFSv3 is the default choice. if the server does not support NFSv3, we fall back to versions 2 or 1.
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info --- net/nfs.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c index 21cae52f35..7a8887ef23 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -26,6 +26,10 @@ * NFSv2 is still used by default. But if server does not support NFSv2, then * NFSv3 is used, if available on NFS server. */
+/* NOTE 5: NFSv1 support added by Christian Gmeiner, Thomas Rienoessl, + * September 27, 2018. As of now, NFSv3 is the default choice. If the server + * does not support NFSv3, we fall back to versions 2 or 1. */ + #include <common.h> #include <command.h> #include <display_options.h> @@ -78,6 +82,7 @@ static char nfs_path_buff[2048];
enum nfs_version { NFS_UNKOWN = 0, + NFS_V1 = 1, NFS_V2 = 2, NFS_V3 = 3, }; @@ -192,6 +197,7 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) switch (rpc_prog) { case PROG_NFS: switch (choosen_nfs_version) { + case NFS_V1: case NFS_V2: rpc_pkt.u.call.vers = htonl(2); break; @@ -205,8 +211,26 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) break; } break; - case PROG_PORTMAP: case PROG_MOUNT: + switch (choosen_nfs_version) { + case NFS_V1: + rpc_pkt.u.call.vers = htonl(1); + break; + + case NFS_V2: + rpc_pkt.u.call.vers = htonl(2); + break; + + case NFS_V3: + rpc_pkt.u.call.vers = htonl(3); + break; + + case NFS_UNKOWN: + /* nothing to do */ + break; + } + break; + case PROG_PORTMAP: default: rpc_pkt.u.call.vers = htonl(2); /* portmapper is version 2 */ } @@ -311,7 +335,7 @@ static void nfs_readlink_req(void) p = &(data[0]); p = rpc_add_credentials(p);
- if (choosen_nfs_version == NFS_V2) { + if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); } else { /* NFS_V3 */ @@ -340,7 +364,7 @@ static void nfs_lookup_req(char *fname) p = &(data[0]); p = rpc_add_credentials(p);
- if (choosen_nfs_version == NFS_V2) { + if (choosen_nfs_version != NFS_V3) { memcpy(p, dirfh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(fnamelen); @@ -380,7 +404,7 @@ static void nfs_read_req(int offset, int readlen) p = &(data[0]); p = rpc_add_credentials(p);
- if (choosen_nfs_version == NFS_V2) { + if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(offset); @@ -410,13 +434,13 @@ static void nfs_send(void)
switch (nfs_state) { case STATE_PRCLOOKUP_PROG_MOUNT_REQ: - if (choosen_nfs_version == NFS_V2) + if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_MOUNT, 1); else /* NFS_V3 */ rpc_lookup_req(PROG_MOUNT, 3); break; case STATE_PRCLOOKUP_PROG_NFS_REQ: - if (choosen_nfs_version == NFS_V2) + if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_NFS, 2); else /* NFS_V3 */ rpc_lookup_req(PROG_NFS, 3); @@ -457,7 +481,7 @@ static int rpc_handle_error(struct rpc_t *rpc_pkt) const int min = ntohl(rpc_pkt->u.reply.data[0]); const int max = ntohl(rpc_pkt->u.reply.data[1]);
- if (max < NFS_V2 || max > NFS_V3 || min > NFS_V3) { + if (max < NFS_V1 || max > NFS_V3 || min > NFS_V3) { puts("*** ERROR: NFS version not supported"); debug(": Requested: V%d, accepted: min V%d - max V%d\n", choosen_nfs_version, @@ -588,7 +612,7 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len) if (ret) return ret;
- if (choosen_nfs_version == NFS_V2) { + if (choosen_nfs_version != NFS_V3) { if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + NFS_FHSIZE) > len) return -NFS_RPC_DROP; memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE); @@ -712,7 +736,7 @@ static int nfs_read_reply(uchar *pkt, unsigned len) if (!(nfs_offset % ((NFS_READ_SIZE / 2) * 10))) putc('#');
- if (choosen_nfs_version == NFS_V2) { + if (choosen_nfs_version != NFS_V3) { rlen = ntohl(rpc_pkt.u.reply.data[18]); data_ptr = (uchar *)&(rpc_pkt.u.reply.data[19]); } else { /* NFS_V3 */
Hello! I must admit that this patch is broken and does not add any NFSv1 support. Just look below....
On Friday 10 March 2023 10:51:55 Christian Gmeiner wrote:
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
NFSv1 support added by Christian Gmeiner, Thomas Rienoessl, September 27, 2018. As of now, NFSv3 is the default choice. if the server does not support NFSv3, we fall back to versions 2 or 1.
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info
net/nfs.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c index 21cae52f35..7a8887ef23 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -26,6 +26,10 @@
- NFSv2 is still used by default. But if server does not support NFSv2, then
- NFSv3 is used, if available on NFS server. */
+/* NOTE 5: NFSv1 support added by Christian Gmeiner, Thomas Rienoessl,
- September 27, 2018. As of now, NFSv3 is the default choice. If the server
- does not support NFSv3, we fall back to versions 2 or 1. */
#include <common.h> #include <command.h> #include <display_options.h> @@ -78,6 +82,7 @@ static char nfs_path_buff[2048];
enum nfs_version { NFS_UNKOWN = 0,
- NFS_V1 = 1, NFS_V2 = 2, NFS_V3 = 3,
}; @@ -192,6 +197,7 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) switch (rpc_prog) { case PROG_NFS: switch (choosen_nfs_version) {
case NFS_V1:
So if NFSv1 is chosen then this code uses NFSv2. This is either rebasing problem or just prove that this patch does not add any NFSv1 support.
break;@@ -205,8 +211,26 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) break; } break;
- case PROG_PORTMAP: case PROG_MOUNT:
switch (choosen_nfs_version) {case NFS_V1:rpc_pkt.u.call.vers = htonl(1);break;
And later here for NFSv1 we are trying to use Mount Server, which NFSv1 did not use at all. So this patch really does not have to work with old NFSv1 servers.
Instead NFSv1 uses NFSPROC_ROOT RPC call exported by NFS server. (See that this RPC call is deprecated in NFSv2 and MNT server is used in NFSv2 instead.)
MNTv1 is service used by the NFSv2 and it returns NFSv2 file handles (not NFSv1 file handles). MNTv2 is also used by NFSv2 and as addition to MNTv1, it adds DIRPATH rpc call. So if NFSv2 does not need to use DIRPATH then it is fine to use just MNTv1 in NFSv2.
case NFS_V2:rpc_pkt.u.call.vers = htonl(2);break;case NFS_V3:rpc_pkt.u.call.vers = htonl(3);break;case NFS_UNKOWN:/* nothing to do */break;}break;- case PROG_PORTMAP: default: rpc_pkt.u.call.vers = htonl(2); /* portmapper is version 2 */ }
@@ -311,7 +335,7 @@ static void nfs_readlink_req(void) p = &(data[0]); p = rpc_add_credentials(p);
- if (choosen_nfs_version == NFS_V2) {
- if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); } else { /* NFS_V3 */
@@ -340,7 +364,7 @@ static void nfs_lookup_req(char *fname) p = &(data[0]); p = rpc_add_credentials(p);
- if (choosen_nfs_version == NFS_V2) {
- if (choosen_nfs_version != NFS_V3) { memcpy(p, dirfh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(fnamelen);
@@ -380,7 +404,7 @@ static void nfs_read_req(int offset, int readlen) p = &(data[0]); p = rpc_add_credentials(p);
- if (choosen_nfs_version == NFS_V2) {
- if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(offset);
@@ -410,13 +434,13 @@ static void nfs_send(void)
switch (nfs_state) { case STATE_PRCLOOKUP_PROG_MOUNT_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_MOUNT, 1);
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_NFS, 2);@@ -457,7 +481,7 @@ static int rpc_handle_error(struct rpc_t *rpc_pkt) const int min = ntohl(rpc_pkt->u.reply.data[0]); const int max = ntohl(rpc_pkt->u.reply.data[1]);
if (max < NFS_V2 || max > NFS_V3 || min > NFS_V3) {
if (max < NFS_V1 || max > NFS_V3 || min > NFS_V3) { puts("*** ERROR: NFS version not supported"); debug(": Requested: V%d, accepted: min V%d - max V%d\n", choosen_nfs_version,@@ -588,7 +612,7 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len) if (ret) return ret;
- if (choosen_nfs_version == NFS_V2) {
- if (choosen_nfs_version != NFS_V3) { if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + NFS_FHSIZE) > len) return -NFS_RPC_DROP; memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);
@@ -712,7 +736,7 @@ static int nfs_read_reply(uchar *pkt, unsigned len) if (!(nfs_offset % ((NFS_READ_SIZE / 2) * 10))) putc('#');
- if (choosen_nfs_version == NFS_V2) {
- if (choosen_nfs_version != NFS_V3) { rlen = ntohl(rpc_pkt.u.reply.data[18]); data_ptr = (uchar *)&(rpc_pkt.u.reply.data[19]); } else { /* NFS_V3 */
-- 2.39.2
And looking at the other changes here, there is really _no_ code which adds NFSv1 support.
So what is this patch doing? The only thing which it does is that for NFSv1 requests it does NFSv2 calls. On every place is just check that choosen_nfs_version is not NFS_V3.
Which just basically duplicates NFSv2 to be used two times.
I would suggest to revisit this patch (who reviewed it at all?) and either fix it or revert it. And of course properly test it. (And I really curious where you find NFSv1 server because Linux has already removed also NFSv2 support from userspace...)
On Sun, Jun 11, 2023 at 1:54 PM Pali Rohár pali@kernel.org wrote:
Hello! I must admit that this patch is broken and does not add any NFSv1 support. Just look below....
On Friday 10 March 2023 10:51:55 Christian Gmeiner wrote:
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
NFSv1 support added by Christian Gmeiner, Thomas Rienoessl, September 27, 2018. As of now, NFSv3 is the default choice. if the server does not support NFSv3, we fall back to versions 2 or 1.
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info
net/nfs.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c index 21cae52f35..7a8887ef23 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -26,6 +26,10 @@
- NFSv2 is still used by default. But if server does not support NFSv2, then
- NFSv3 is used, if available on NFS server. */
+/* NOTE 5: NFSv1 support added by Christian Gmeiner, Thomas Rienoessl,
- September 27, 2018. As of now, NFSv3 is the default choice. If the server
- does not support NFSv3, we fall back to versions 2 or 1. */
#include <common.h> #include <command.h> #include <display_options.h> @@ -78,6 +82,7 @@ static char nfs_path_buff[2048];
enum nfs_version { NFS_UNKOWN = 0,
NFS_V1 = 1, NFS_V2 = 2, NFS_V3 = 3,}; @@ -192,6 +197,7 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) switch (rpc_prog) { case PROG_NFS: switch (choosen_nfs_version) {
case NFS_V1: case NFS_V2: rpc_pkt.u.call.vers = htonl(2);So if NFSv1 is chosen then this code uses NFSv2. This is either rebasing problem or just prove that this patch does not add any NFSv1 support.
break;@@ -205,8 +211,26 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) break; } break;
case PROG_PORTMAP: case PROG_MOUNT:
switch (choosen_nfs_version) {case NFS_V1:rpc_pkt.u.call.vers = htonl(1);break;And later here for NFSv1 we are trying to use Mount Server, which NFSv1 did not use at all. So this patch really does not have to work with old NFSv1 servers.
Instead NFSv1 uses NFSPROC_ROOT RPC call exported by NFS server. (See that this RPC call is deprecated in NFSv2 and MNT server is used in NFSv2 instead.)
MNTv1 is service used by the NFSv2 and it returns NFSv2 file handles (not NFSv1 file handles). MNTv2 is also used by NFSv2 and as addition to MNTv1, it adds DIRPATH rpc call. So if NFSv2 does not need to use DIRPATH then it is fine to use just MNTv1 in NFSv2.
case NFS_V2:rpc_pkt.u.call.vers = htonl(2);break;case NFS_V3:rpc_pkt.u.call.vers = htonl(3);break;case NFS_UNKOWN:/* nothing to do */break;}break;case PROG_PORTMAP: default: rpc_pkt.u.call.vers = htonl(2); /* portmapper is version 2 */ }@@ -311,7 +335,7 @@ static void nfs_readlink_req(void) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); } else { /* NFS_V3 */@@ -340,7 +364,7 @@ static void nfs_lookup_req(char *fname) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, dirfh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(fnamelen);@@ -380,7 +404,7 @@ static void nfs_read_req(int offset, int readlen) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(offset);@@ -410,13 +434,13 @@ static void nfs_send(void)
switch (nfs_state) { case STATE_PRCLOOKUP_PROG_MOUNT_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_MOUNT, 1); else /* NFS_V3 */ rpc_lookup_req(PROG_MOUNT, 3); break; case STATE_PRCLOOKUP_PROG_NFS_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_NFS, 2); else /* NFS_V3 */ rpc_lookup_req(PROG_NFS, 3);@@ -457,7 +481,7 @@ static int rpc_handle_error(struct rpc_t *rpc_pkt) const int min = ntohl(rpc_pkt->u.reply.data[0]); const int max = ntohl(rpc_pkt->u.reply.data[1]);
if (max < NFS_V2 || max > NFS_V3 || min > NFS_V3) {
if (max < NFS_V1 || max > NFS_V3 || min > NFS_V3) { puts("*** ERROR: NFS version not supported"); debug(": Requested: V%d, accepted: min V%d - max V%d\n", choosen_nfs_version,@@ -588,7 +612,7 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len) if (ret) return ret;
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + NFS_FHSIZE) > len) return -NFS_RPC_DROP; memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);@@ -712,7 +736,7 @@ static int nfs_read_reply(uchar *pkt, unsigned len) if (!(nfs_offset % ((NFS_READ_SIZE / 2) * 10))) putc('#');
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { rlen = ntohl(rpc_pkt.u.reply.data[18]); data_ptr = (uchar *)&(rpc_pkt.u.reply.data[19]); } else { /* NFS_V3 */-- 2.39.2
And looking at the other changes here, there is really _no_ code which adds NFSv1 support.
So what is this patch doing? The only thing which it does is that for NFSv1 requests it does NFSv2 calls. On every place is just check that choosen_nfs_version is not NFS_V3.
Which just basically duplicates NFSv2 to be used two times.
I would suggest to revisit this patch (who reviewed it at all?) and either fix it or revert it. And of course properly test it. (And I really curious where you find NFSv1 server because Linux has already removed also NFSv2 support from userspace...)
I was opposed to it, according to a number of resources on the internet NFSv1 was never publicly released, I suspect the NFS server that "works" with this isn't v1 at all but rather has bugs or differences in implementation that need to be worked around that this patch just so happens to by accident implement.
Am So., 11. Juni 2023 um 15:02 Uhr schrieb Peter Robinson pbrobinson@gmail.com:
On Sun, Jun 11, 2023 at 1:54 PM Pali Rohár pali@kernel.org wrote:
Hello! I must admit that this patch is broken and does not add any NFSv1 support. Just look below....
On Friday 10 March 2023 10:51:55 Christian Gmeiner wrote:
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
NFSv1 support added by Christian Gmeiner, Thomas Rienoessl, September 27, 2018. As of now, NFSv3 is the default choice. if the server does not support NFSv3, we fall back to versions 2 or 1.
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info
net/nfs.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c index 21cae52f35..7a8887ef23 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -26,6 +26,10 @@
- NFSv2 is still used by default. But if server does not support NFSv2, then
- NFSv3 is used, if available on NFS server. */
+/* NOTE 5: NFSv1 support added by Christian Gmeiner, Thomas Rienoessl,
- September 27, 2018. As of now, NFSv3 is the default choice. If the server
- does not support NFSv3, we fall back to versions 2 or 1. */
#include <common.h> #include <command.h> #include <display_options.h> @@ -78,6 +82,7 @@ static char nfs_path_buff[2048];
enum nfs_version { NFS_UNKOWN = 0,
NFS_V1 = 1, NFS_V2 = 2, NFS_V3 = 3,}; @@ -192,6 +197,7 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) switch (rpc_prog) { case PROG_NFS: switch (choosen_nfs_version) {
case NFS_V1: case NFS_V2: rpc_pkt.u.call.vers = htonl(2);So if NFSv1 is chosen then this code uses NFSv2. This is either rebasing problem or just prove that this patch does not add any NFSv1 support.
break;@@ -205,8 +211,26 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) break; } break;
case PROG_PORTMAP: case PROG_MOUNT:
switch (choosen_nfs_version) {case NFS_V1:rpc_pkt.u.call.vers = htonl(1);break;And later here for NFSv1 we are trying to use Mount Server, which NFSv1 did not use at all. So this patch really does not have to work with old NFSv1 servers.
Instead NFSv1 uses NFSPROC_ROOT RPC call exported by NFS server. (See that this RPC call is deprecated in NFSv2 and MNT server is used in NFSv2 instead.)
MNTv1 is service used by the NFSv2 and it returns NFSv2 file handles (not NFSv1 file handles). MNTv2 is also used by NFSv2 and as addition to MNTv1, it adds DIRPATH rpc call. So if NFSv2 does not need to use DIRPATH then it is fine to use just MNTv1 in NFSv2.
case NFS_V2:rpc_pkt.u.call.vers = htonl(2);break;case NFS_V3:rpc_pkt.u.call.vers = htonl(3);break;case NFS_UNKOWN:/* nothing to do */break;}break;case PROG_PORTMAP: default: rpc_pkt.u.call.vers = htonl(2); /* portmapper is version 2 */ }@@ -311,7 +335,7 @@ static void nfs_readlink_req(void) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); } else { /* NFS_V3 */@@ -340,7 +364,7 @@ static void nfs_lookup_req(char *fname) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, dirfh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(fnamelen);@@ -380,7 +404,7 @@ static void nfs_read_req(int offset, int readlen) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(offset);@@ -410,13 +434,13 @@ static void nfs_send(void)
switch (nfs_state) { case STATE_PRCLOOKUP_PROG_MOUNT_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_MOUNT, 1); else /* NFS_V3 */ rpc_lookup_req(PROG_MOUNT, 3); break; case STATE_PRCLOOKUP_PROG_NFS_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_NFS, 2); else /* NFS_V3 */ rpc_lookup_req(PROG_NFS, 3);@@ -457,7 +481,7 @@ static int rpc_handle_error(struct rpc_t *rpc_pkt) const int min = ntohl(rpc_pkt->u.reply.data[0]); const int max = ntohl(rpc_pkt->u.reply.data[1]);
if (max < NFS_V2 || max > NFS_V3 || min > NFS_V3) {
if (max < NFS_V1 || max > NFS_V3 || min > NFS_V3) { puts("*** ERROR: NFS version not supported"); debug(": Requested: V%d, accepted: min V%d - max V%d\n", choosen_nfs_version,@@ -588,7 +612,7 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len) if (ret) return ret;
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + NFS_FHSIZE) > len) return -NFS_RPC_DROP; memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);@@ -712,7 +736,7 @@ static int nfs_read_reply(uchar *pkt, unsigned len) if (!(nfs_offset % ((NFS_READ_SIZE / 2) * 10))) putc('#');
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { rlen = ntohl(rpc_pkt.u.reply.data[18]); data_ptr = (uchar *)&(rpc_pkt.u.reply.data[19]); } else { /* NFS_V3 */-- 2.39.2
And looking at the other changes here, there is really _no_ code which adds NFSv1 support.
So what is this patch doing? The only thing which it does is that for NFSv1 requests it does NFSv2 calls. On every place is just check that choosen_nfs_version is not NFS_V3.
Which just basically duplicates NFSv2 to be used two times.
I would suggest to revisit this patch (who reviewed it at all?) and either fix it or revert it. And of course properly test it. (And I really curious where you find NFSv1 server because Linux has already removed also NFSv2 support from userspace...)
I was opposed to it, according to a number of resources on the internet NFSv1 was never publicly released, I suspect the NFS server that "works" with this isn't v1 at all but rather has bugs or differences in implementation that need to be worked around that this patch just so happens to by accident implement.
The patch is really about adding support for version one of the mount protocol. Sadly I do not have access to the vxwork 5 source code anymore.
Hello
Hello! I must admit that this patch is broken and does not add any NFSv1 support. Just look below....
So .. let see what happend here.
On Friday 10 March 2023 10:51:55 Christian Gmeiner wrote:
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
NFSv1 support added by Christian Gmeiner, Thomas Rienoessl, September 27, 2018. As of now, NFSv3 is the default choice. if the server does not support NFSv3, we fall back to versions 2 or 1.
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info
net/nfs.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c index 21cae52f35..7a8887ef23 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -26,6 +26,10 @@
- NFSv2 is still used by default. But if server does not support NFSv2, then
- NFSv3 is used, if available on NFS server. */
+/* NOTE 5: NFSv1 support added by Christian Gmeiner, Thomas Rienoessl,
- September 27, 2018. As of now, NFSv3 is the default choice. If the server
- does not support NFSv3, we fall back to versions 2 or 1. */
#include <common.h> #include <command.h> #include <display_options.h> @@ -78,6 +82,7 @@ static char nfs_path_buff[2048];
enum nfs_version { NFS_UNKOWN = 0,
NFS_V1 = 1, NFS_V2 = 2, NFS_V3 = 3,}; @@ -192,6 +197,7 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) switch (rpc_prog) { case PROG_NFS: switch (choosen_nfs_version) {
case NFS_V1: case NFS_V2: rpc_pkt.u.call.vers = htonl(2);So if NFSv1 is chosen then this code uses NFSv2. This is either rebasing problem or just prove that this patch does not add any NFSv1 support.
break;@@ -205,8 +211,26 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) break; } break;
case PROG_PORTMAP: case PROG_MOUNT:
switch (choosen_nfs_version) {case NFS_V1:rpc_pkt.u.call.vers = htonl(1);break;And later here for NFSv1 we are trying to use Mount Server, which NFSv1 did not use at all. So this patch really does not have to work with old NFSv1 servers.
Instead NFSv1 uses NFSPROC_ROOT RPC call exported by NFS server. (See that this RPC call is deprecated in NFSv2 and MNT server is used in NFSv2 instead.)
MNTv1 is service used by the NFSv2 and it returns NFSv2 file handles (not NFSv1 file handles). MNTv2 is also used by NFSv2 and as addition to MNTv1, it adds DIRPATH rpc call. So if NFSv2 does not need to use DIRPATH then it is fine to use just MNTv1 in NFSv2.
case NFS_V2:rpc_pkt.u.call.vers = htonl(2);break;case NFS_V3:rpc_pkt.u.call.vers = htonl(3);break;case NFS_UNKOWN:/* nothing to do */break;}break;case PROG_PORTMAP: default: rpc_pkt.u.call.vers = htonl(2); /* portmapper is version 2 */ }@@ -311,7 +335,7 @@ static void nfs_readlink_req(void) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); } else { /* NFS_V3 */@@ -340,7 +364,7 @@ static void nfs_lookup_req(char *fname) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, dirfh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(fnamelen);@@ -380,7 +404,7 @@ static void nfs_read_req(int offset, int readlen) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(offset);@@ -410,13 +434,13 @@ static void nfs_send(void)
switch (nfs_state) { case STATE_PRCLOOKUP_PROG_MOUNT_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_MOUNT, 1); else /* NFS_V3 */ rpc_lookup_req(PROG_MOUNT, 3); break; case STATE_PRCLOOKUP_PROG_NFS_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_NFS, 2); else /* NFS_V3 */ rpc_lookup_req(PROG_NFS, 3);@@ -457,7 +481,7 @@ static int rpc_handle_error(struct rpc_t *rpc_pkt) const int min = ntohl(rpc_pkt->u.reply.data[0]); const int max = ntohl(rpc_pkt->u.reply.data[1]);
if (max < NFS_V2 || max > NFS_V3 || min > NFS_V3) {
if (max < NFS_V1 || max > NFS_V3 || min > NFS_V3) { puts("*** ERROR: NFS version not supported"); debug(": Requested: V%d, accepted: min V%d - max V%d\n", choosen_nfs_version,@@ -588,7 +612,7 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len) if (ret) return ret;
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + NFS_FHSIZE) > len) return -NFS_RPC_DROP; memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);@@ -712,7 +736,7 @@ static int nfs_read_reply(uchar *pkt, unsigned len) if (!(nfs_offset % ((NFS_READ_SIZE / 2) * 10))) putc('#');
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { rlen = ntohl(rpc_pkt.u.reply.data[18]); data_ptr = (uchar *)&(rpc_pkt.u.reply.data[19]); } else { /* NFS_V3 */-- 2.39.2
And looking at the other changes here, there is really _no_ code which adds NFSv1 support.
So what is this patch doing? The only thing which it does is that for NFSv1 requests it does NFSv2 calls. On every place is just check that choosen_nfs_version is not NFS_V3.
Which just basically duplicates NFSv2 to be used two times.
I would suggest to revisit this patch (who reviewed it at all?) and either fix it or revert it. And of course properly test it. (And I really curious where you find NFSv1 server because Linux has already removed also NFSv2 support from userspace...)
Soo. I had a look at RFC 1094 and this patch adds version one of the mount protocol. I am quite unhappy that we got into this state, but the company I worked for uses the term NFSv1 for this in all their configuration tools etc. What would you suggest to improve this situation?
On Sunday 11 June 2023 16:57:07 Christian Gmeiner wrote:
Hello
Hello! I must admit that this patch is broken and does not add any NFSv1 support. Just look below....
So .. let see what happend here.
On Friday 10 March 2023 10:51:55 Christian Gmeiner wrote:
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
NFSv1 support added by Christian Gmeiner, Thomas Rienoessl, September 27, 2018. As of now, NFSv3 is the default choice. if the server does not support NFSv3, we fall back to versions 2 or 1.
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info
net/nfs.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c index 21cae52f35..7a8887ef23 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -26,6 +26,10 @@
- NFSv2 is still used by default. But if server does not support NFSv2, then
- NFSv3 is used, if available on NFS server. */
+/* NOTE 5: NFSv1 support added by Christian Gmeiner, Thomas Rienoessl,
- September 27, 2018. As of now, NFSv3 is the default choice. If the server
- does not support NFSv3, we fall back to versions 2 or 1. */
#include <common.h> #include <command.h> #include <display_options.h> @@ -78,6 +82,7 @@ static char nfs_path_buff[2048];
enum nfs_version { NFS_UNKOWN = 0,
NFS_V1 = 1, NFS_V2 = 2, NFS_V3 = 3,}; @@ -192,6 +197,7 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) switch (rpc_prog) { case PROG_NFS: switch (choosen_nfs_version) {
case NFS_V1: case NFS_V2: rpc_pkt.u.call.vers = htonl(2);So if NFSv1 is chosen then this code uses NFSv2. This is either rebasing problem or just prove that this patch does not add any NFSv1 support.
break;@@ -205,8 +211,26 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) break; } break;
case PROG_PORTMAP: case PROG_MOUNT:
switch (choosen_nfs_version) {case NFS_V1:rpc_pkt.u.call.vers = htonl(1);break;And later here for NFSv1 we are trying to use Mount Server, which NFSv1 did not use at all. So this patch really does not have to work with old NFSv1 servers.
Instead NFSv1 uses NFSPROC_ROOT RPC call exported by NFS server. (See that this RPC call is deprecated in NFSv2 and MNT server is used in NFSv2 instead.)
MNTv1 is service used by the NFSv2 and it returns NFSv2 file handles (not NFSv1 file handles). MNTv2 is also used by NFSv2 and as addition to MNTv1, it adds DIRPATH rpc call. So if NFSv2 does not need to use DIRPATH then it is fine to use just MNTv1 in NFSv2.
case NFS_V2:rpc_pkt.u.call.vers = htonl(2);break;case NFS_V3:rpc_pkt.u.call.vers = htonl(3);break;case NFS_UNKOWN:/* nothing to do */break;}break;case PROG_PORTMAP: default: rpc_pkt.u.call.vers = htonl(2); /* portmapper is version 2 */ }@@ -311,7 +335,7 @@ static void nfs_readlink_req(void) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); } else { /* NFS_V3 */@@ -340,7 +364,7 @@ static void nfs_lookup_req(char *fname) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, dirfh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(fnamelen);@@ -380,7 +404,7 @@ static void nfs_read_req(int offset, int readlen) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(offset);@@ -410,13 +434,13 @@ static void nfs_send(void)
switch (nfs_state) { case STATE_PRCLOOKUP_PROG_MOUNT_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_MOUNT, 1); else /* NFS_V3 */ rpc_lookup_req(PROG_MOUNT, 3); break; case STATE_PRCLOOKUP_PROG_NFS_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_NFS, 2); else /* NFS_V3 */ rpc_lookup_req(PROG_NFS, 3);@@ -457,7 +481,7 @@ static int rpc_handle_error(struct rpc_t *rpc_pkt) const int min = ntohl(rpc_pkt->u.reply.data[0]); const int max = ntohl(rpc_pkt->u.reply.data[1]);
if (max < NFS_V2 || max > NFS_V3 || min > NFS_V3) {
if (max < NFS_V1 || max > NFS_V3 || min > NFS_V3) { puts("*** ERROR: NFS version not supported"); debug(": Requested: V%d, accepted: min V%d - max V%d\n", choosen_nfs_version,@@ -588,7 +612,7 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len) if (ret) return ret;
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + NFS_FHSIZE) > len) return -NFS_RPC_DROP; memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);@@ -712,7 +736,7 @@ static int nfs_read_reply(uchar *pkt, unsigned len) if (!(nfs_offset % ((NFS_READ_SIZE / 2) * 10))) putc('#');
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { rlen = ntohl(rpc_pkt.u.reply.data[18]); data_ptr = (uchar *)&(rpc_pkt.u.reply.data[19]); } else { /* NFS_V3 */-- 2.39.2
And looking at the other changes here, there is really _no_ code which adds NFSv1 support.
So what is this patch doing? The only thing which it does is that for NFSv1 requests it does NFSv2 calls. On every place is just check that choosen_nfs_version is not NFS_V3.
Which just basically duplicates NFSv2 to be used two times.
I would suggest to revisit this patch (who reviewed it at all?) and either fix it or revert it. And of course properly test it. (And I really curious where you find NFSv1 server because Linux has already removed also NFSv2 support from userspace...)
Soo. I had a look at RFC 1094 and this patch adds version one of the mount protocol. I am quite unhappy that we got into this state, but the company I worked for uses the term NFSv1 for this in all their configuration tools etc. What would you suggest to improve this situation?
Suggestion: Revert this one patch, then figure out what is needed to be supported (describe all details what kind of protocol and what packets needs to be send and received), find a way and discuss how to implement it and prepare patch for the review.
If you, who sent this patch, are unhappy about this patch, and also Peter and me pointed issues, then this patch really should not have land into git master branch in this form. It means that nobody is happy with this patch.
Am So., 11. Juni 2023 um 17:10 Uhr schrieb Pali Rohár pali@kernel.org:
On Sunday 11 June 2023 16:57:07 Christian Gmeiner wrote:
Hello
Hello! I must admit that this patch is broken and does not add any NFSv1 support. Just look below....
So .. let see what happend here.
On Friday 10 March 2023 10:51:55 Christian Gmeiner wrote:
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
NFSv1 support added by Christian Gmeiner, Thomas Rienoessl, September 27, 2018. As of now, NFSv3 is the default choice. if the server does not support NFSv3, we fall back to versions 2 or 1.
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info
net/nfs.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c index 21cae52f35..7a8887ef23 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -26,6 +26,10 @@
- NFSv2 is still used by default. But if server does not support NFSv2, then
- NFSv3 is used, if available on NFS server. */
+/* NOTE 5: NFSv1 support added by Christian Gmeiner, Thomas Rienoessl,
- September 27, 2018. As of now, NFSv3 is the default choice. If the server
- does not support NFSv3, we fall back to versions 2 or 1. */
#include <common.h> #include <command.h> #include <display_options.h> @@ -78,6 +82,7 @@ static char nfs_path_buff[2048];
enum nfs_version { NFS_UNKOWN = 0,
NFS_V1 = 1, NFS_V2 = 2, NFS_V3 = 3,}; @@ -192,6 +197,7 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) switch (rpc_prog) { case PROG_NFS: switch (choosen_nfs_version) {
case NFS_V1: case NFS_V2: rpc_pkt.u.call.vers = htonl(2);So if NFSv1 is chosen then this code uses NFSv2. This is either rebasing problem or just prove that this patch does not add any NFSv1 support.
break;@@ -205,8 +211,26 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) break; } break;
case PROG_PORTMAP: case PROG_MOUNT:
switch (choosen_nfs_version) {case NFS_V1:rpc_pkt.u.call.vers = htonl(1);break;And later here for NFSv1 we are trying to use Mount Server, which NFSv1 did not use at all. So this patch really does not have to work with old NFSv1 servers.
Instead NFSv1 uses NFSPROC_ROOT RPC call exported by NFS server. (See that this RPC call is deprecated in NFSv2 and MNT server is used in NFSv2 instead.)
MNTv1 is service used by the NFSv2 and it returns NFSv2 file handles (not NFSv1 file handles). MNTv2 is also used by NFSv2 and as addition to MNTv1, it adds DIRPATH rpc call. So if NFSv2 does not need to use DIRPATH then it is fine to use just MNTv1 in NFSv2.
case NFS_V2:rpc_pkt.u.call.vers = htonl(2);break;case NFS_V3:rpc_pkt.u.call.vers = htonl(3);break;case NFS_UNKOWN:/* nothing to do */break;}break;case PROG_PORTMAP: default: rpc_pkt.u.call.vers = htonl(2); /* portmapper is version 2 */ }@@ -311,7 +335,7 @@ static void nfs_readlink_req(void) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); } else { /* NFS_V3 */@@ -340,7 +364,7 @@ static void nfs_lookup_req(char *fname) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, dirfh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(fnamelen);@@ -380,7 +404,7 @@ static void nfs_read_req(int offset, int readlen) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(offset);@@ -410,13 +434,13 @@ static void nfs_send(void)
switch (nfs_state) { case STATE_PRCLOOKUP_PROG_MOUNT_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_MOUNT, 1); else /* NFS_V3 */ rpc_lookup_req(PROG_MOUNT, 3); break; case STATE_PRCLOOKUP_PROG_NFS_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_NFS, 2); else /* NFS_V3 */ rpc_lookup_req(PROG_NFS, 3);@@ -457,7 +481,7 @@ static int rpc_handle_error(struct rpc_t *rpc_pkt) const int min = ntohl(rpc_pkt->u.reply.data[0]); const int max = ntohl(rpc_pkt->u.reply.data[1]);
if (max < NFS_V2 || max > NFS_V3 || min > NFS_V3) {
if (max < NFS_V1 || max > NFS_V3 || min > NFS_V3) { puts("*** ERROR: NFS version not supported"); debug(": Requested: V%d, accepted: min V%d - max V%d\n", choosen_nfs_version,@@ -588,7 +612,7 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len) if (ret) return ret;
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + NFS_FHSIZE) > len) return -NFS_RPC_DROP; memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);@@ -712,7 +736,7 @@ static int nfs_read_reply(uchar *pkt, unsigned len) if (!(nfs_offset % ((NFS_READ_SIZE / 2) * 10))) putc('#');
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { rlen = ntohl(rpc_pkt.u.reply.data[18]); data_ptr = (uchar *)&(rpc_pkt.u.reply.data[19]); } else { /* NFS_V3 */-- 2.39.2
And looking at the other changes here, there is really _no_ code which adds NFSv1 support.
So what is this patch doing? The only thing which it does is that for NFSv1 requests it does NFSv2 calls. On every place is just check that choosen_nfs_version is not NFS_V3.
Which just basically duplicates NFSv2 to be used two times.
I would suggest to revisit this patch (who reviewed it at all?) and either fix it or revert it. And of course properly test it. (And I really curious where you find NFSv1 server because Linux has already removed also NFSv2 support from userspace...)
Soo. I had a look at RFC 1094 and this patch adds version one of the mount protocol. I am quite unhappy that we got into this state, but the company I worked for uses the term NFSv1 for this in all their configuration tools etc. What would you suggest to improve this situation?
Suggestion: Revert this one patch, then figure out what is needed to be supported (describe all details what kind of protocol and what packets needs to be send and received), find a way and discuss how to implement it and prepare patch for the review.
Here comes a big problem: "find a way and discuss how to implement it and prepare patch for the review." I am not sure if there is really someone interested in discussion about this topic and it is even harder to find someone that reviews U-Boot patches. And this is not specific to networking stuff.
I can prepare a patch that reworks the current implementation (without a revert) and send it out.
If you, who sent this patch, are unhappy about this patch, and also Peter and me pointed issues, then this patch really should not have land into git master branch in this form. It means that nobody is happy with this patch.
To clarify a fact: I am happy with the patch. I am unhappy with the wrong "naming" that my old employer used and influenced this patch. Also the company is shipping thousands of devices per year where these patches are used.
On Sun, Jun 11, 2023 at 05:24:50PM +0200, Christian Gmeiner wrote:
Am So., 11. Juni 2023 um 17:10 Uhr schrieb Pali Rohár pali@kernel.org:
On Sunday 11 June 2023 16:57:07 Christian Gmeiner wrote:
Hello
Hello! I must admit that this patch is broken and does not add any NFSv1 support. Just look below....
So .. let see what happend here.
On Friday 10 March 2023 10:51:55 Christian Gmeiner wrote:
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
NFSv1 support added by Christian Gmeiner, Thomas Rienoessl, September 27, 2018. As of now, NFSv3 is the default choice. if the server does not support NFSv3, we fall back to versions 2 or 1.
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info
net/nfs.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c index 21cae52f35..7a8887ef23 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -26,6 +26,10 @@
- NFSv2 is still used by default. But if server does not support NFSv2, then
- NFSv3 is used, if available on NFS server. */
+/* NOTE 5: NFSv1 support added by Christian Gmeiner, Thomas Rienoessl,
- September 27, 2018. As of now, NFSv3 is the default choice. If the server
- does not support NFSv3, we fall back to versions 2 or 1. */
#include <common.h> #include <command.h> #include <display_options.h> @@ -78,6 +82,7 @@ static char nfs_path_buff[2048];
enum nfs_version { NFS_UNKOWN = 0,
NFS_V1 = 1, NFS_V2 = 2, NFS_V3 = 3,}; @@ -192,6 +197,7 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) switch (rpc_prog) { case PROG_NFS: switch (choosen_nfs_version) {
case NFS_V1: case NFS_V2: rpc_pkt.u.call.vers = htonl(2);So if NFSv1 is chosen then this code uses NFSv2. This is either rebasing problem or just prove that this patch does not add any NFSv1 support.
break;@@ -205,8 +211,26 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) break; } break;
case PROG_PORTMAP: case PROG_MOUNT:
switch (choosen_nfs_version) {case NFS_V1:rpc_pkt.u.call.vers = htonl(1);break;And later here for NFSv1 we are trying to use Mount Server, which NFSv1 did not use at all. So this patch really does not have to work with old NFSv1 servers.
Instead NFSv1 uses NFSPROC_ROOT RPC call exported by NFS server. (See that this RPC call is deprecated in NFSv2 and MNT server is used in NFSv2 instead.)
MNTv1 is service used by the NFSv2 and it returns NFSv2 file handles (not NFSv1 file handles). MNTv2 is also used by NFSv2 and as addition to MNTv1, it adds DIRPATH rpc call. So if NFSv2 does not need to use DIRPATH then it is fine to use just MNTv1 in NFSv2.
case NFS_V2:rpc_pkt.u.call.vers = htonl(2);break;case NFS_V3:rpc_pkt.u.call.vers = htonl(3);break;case NFS_UNKOWN:/* nothing to do */break;}break;case PROG_PORTMAP: default: rpc_pkt.u.call.vers = htonl(2); /* portmapper is version 2 */ }@@ -311,7 +335,7 @@ static void nfs_readlink_req(void) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); } else { /* NFS_V3 */@@ -340,7 +364,7 @@ static void nfs_lookup_req(char *fname) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, dirfh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(fnamelen);@@ -380,7 +404,7 @@ static void nfs_read_req(int offset, int readlen) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(offset);@@ -410,13 +434,13 @@ static void nfs_send(void)
switch (nfs_state) { case STATE_PRCLOOKUP_PROG_MOUNT_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_MOUNT, 1); else /* NFS_V3 */ rpc_lookup_req(PROG_MOUNT, 3); break; case STATE_PRCLOOKUP_PROG_NFS_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_NFS, 2); else /* NFS_V3 */ rpc_lookup_req(PROG_NFS, 3);@@ -457,7 +481,7 @@ static int rpc_handle_error(struct rpc_t *rpc_pkt) const int min = ntohl(rpc_pkt->u.reply.data[0]); const int max = ntohl(rpc_pkt->u.reply.data[1]);
if (max < NFS_V2 || max > NFS_V3 || min > NFS_V3) {
if (max < NFS_V1 || max > NFS_V3 || min > NFS_V3) { puts("*** ERROR: NFS version not supported"); debug(": Requested: V%d, accepted: min V%d - max V%d\n", choosen_nfs_version,@@ -588,7 +612,7 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len) if (ret) return ret;
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + NFS_FHSIZE) > len) return -NFS_RPC_DROP; memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);@@ -712,7 +736,7 @@ static int nfs_read_reply(uchar *pkt, unsigned len) if (!(nfs_offset % ((NFS_READ_SIZE / 2) * 10))) putc('#');
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { rlen = ntohl(rpc_pkt.u.reply.data[18]); data_ptr = (uchar *)&(rpc_pkt.u.reply.data[19]); } else { /* NFS_V3 */-- 2.39.2
And looking at the other changes here, there is really _no_ code which adds NFSv1 support.
So what is this patch doing? The only thing which it does is that for NFSv1 requests it does NFSv2 calls. On every place is just check that choosen_nfs_version is not NFS_V3.
Which just basically duplicates NFSv2 to be used two times.
I would suggest to revisit this patch (who reviewed it at all?) and either fix it or revert it. And of course properly test it. (And I really curious where you find NFSv1 server because Linux has already removed also NFSv2 support from userspace...)
Soo. I had a look at RFC 1094 and this patch adds version one of the mount protocol. I am quite unhappy that we got into this state, but the company I worked for uses the term NFSv1 for this in all their configuration tools etc. What would you suggest to improve this situation?
Suggestion: Revert this one patch, then figure out what is needed to be supported (describe all details what kind of protocol and what packets needs to be send and received), find a way and discuss how to implement it and prepare patch for the review.
Here comes a big problem: "find a way and discuss how to implement it and prepare patch for the review." I am not sure if there is really someone interested in discussion about this topic and it is even harder to find someone that reviews U-Boot patches. And this is not specific to networking stuff.
I can prepare a patch that reworks the current implementation (without a revert) and send it out.
If you, who sent this patch, are unhappy about this patch, and also Peter and me pointed issues, then this patch really should not have land into git master branch in this form. It means that nobody is happy with this patch.
To clarify a fact: I am happy with the patch. I am unhappy with the wrong "naming" that my old employer used and influenced this patch. Also the company is shipping thousands of devices per year where these patches are used.
Further clean-ups and clarifications to the support here in terms of what it does and doesn't provide are good. But to the point on reviews, yes, I do wish we had more people interested in various areas, and with time to devote to reviewing code as well. Sadly, we don't always, and I took this particular set of patches as being small enough of a global impact while (hopefully!) making future contributions both in this area and the related platforms using it more likely.
On Monday 12 June 2023 12:29:46 Tom Rini wrote:
On Sun, Jun 11, 2023 at 05:24:50PM +0200, Christian Gmeiner wrote:
Am So., 11. Juni 2023 um 17:10 Uhr schrieb Pali Rohár pali@kernel.org:
On Sunday 11 June 2023 16:57:07 Christian Gmeiner wrote:
Hello
Hello! I must admit that this patch is broken and does not add any NFSv1 support. Just look below....
So .. let see what happend here.
On Friday 10 March 2023 10:51:55 Christian Gmeiner wrote:
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
NFSv1 support added by Christian Gmeiner, Thomas Rienoessl, September 27, 2018. As of now, NFSv3 is the default choice. if the server does not support NFSv3, we fall back to versions 2 or 1.
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info
net/nfs.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c index 21cae52f35..7a8887ef23 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -26,6 +26,10 @@
- NFSv2 is still used by default. But if server does not support NFSv2, then
- NFSv3 is used, if available on NFS server. */
+/* NOTE 5: NFSv1 support added by Christian Gmeiner, Thomas Rienoessl,
- September 27, 2018. As of now, NFSv3 is the default choice. If the server
- does not support NFSv3, we fall back to versions 2 or 1. */
#include <common.h> #include <command.h> #include <display_options.h> @@ -78,6 +82,7 @@ static char nfs_path_buff[2048];
enum nfs_version { NFS_UNKOWN = 0,
NFS_V1 = 1, NFS_V2 = 2, NFS_V3 = 3,}; @@ -192,6 +197,7 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) switch (rpc_prog) { case PROG_NFS: switch (choosen_nfs_version) {
case NFS_V1: case NFS_V2: rpc_pkt.u.call.vers = htonl(2);So if NFSv1 is chosen then this code uses NFSv2. This is either rebasing problem or just prove that this patch does not add any NFSv1 support.
break;@@ -205,8 +211,26 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) break; } break;
case PROG_PORTMAP: case PROG_MOUNT:
switch (choosen_nfs_version) {case NFS_V1:rpc_pkt.u.call.vers = htonl(1);break;And later here for NFSv1 we are trying to use Mount Server, which NFSv1 did not use at all. So this patch really does not have to work with old NFSv1 servers.
Instead NFSv1 uses NFSPROC_ROOT RPC call exported by NFS server. (See that this RPC call is deprecated in NFSv2 and MNT server is used in NFSv2 instead.)
MNTv1 is service used by the NFSv2 and it returns NFSv2 file handles (not NFSv1 file handles). MNTv2 is also used by NFSv2 and as addition to MNTv1, it adds DIRPATH rpc call. So if NFSv2 does not need to use DIRPATH then it is fine to use just MNTv1 in NFSv2.
case NFS_V2:rpc_pkt.u.call.vers = htonl(2);break;case NFS_V3:rpc_pkt.u.call.vers = htonl(3);break;case NFS_UNKOWN:/* nothing to do */break;}break;case PROG_PORTMAP: default: rpc_pkt.u.call.vers = htonl(2); /* portmapper is version 2 */ }@@ -311,7 +335,7 @@ static void nfs_readlink_req(void) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); } else { /* NFS_V3 */@@ -340,7 +364,7 @@ static void nfs_lookup_req(char *fname) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, dirfh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(fnamelen);@@ -380,7 +404,7 @@ static void nfs_read_req(int offset, int readlen) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(offset);@@ -410,13 +434,13 @@ static void nfs_send(void)
switch (nfs_state) { case STATE_PRCLOOKUP_PROG_MOUNT_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_MOUNT, 1); else /* NFS_V3 */ rpc_lookup_req(PROG_MOUNT, 3); break; case STATE_PRCLOOKUP_PROG_NFS_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_NFS, 2); else /* NFS_V3 */ rpc_lookup_req(PROG_NFS, 3);@@ -457,7 +481,7 @@ static int rpc_handle_error(struct rpc_t *rpc_pkt) const int min = ntohl(rpc_pkt->u.reply.data[0]); const int max = ntohl(rpc_pkt->u.reply.data[1]);
if (max < NFS_V2 || max > NFS_V3 || min > NFS_V3) {
if (max < NFS_V1 || max > NFS_V3 || min > NFS_V3) { puts("*** ERROR: NFS version not supported"); debug(": Requested: V%d, accepted: min V%d - max V%d\n", choosen_nfs_version,@@ -588,7 +612,7 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len) if (ret) return ret;
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + NFS_FHSIZE) > len) return -NFS_RPC_DROP; memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);@@ -712,7 +736,7 @@ static int nfs_read_reply(uchar *pkt, unsigned len) if (!(nfs_offset % ((NFS_READ_SIZE / 2) * 10))) putc('#');
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { rlen = ntohl(rpc_pkt.u.reply.data[18]); data_ptr = (uchar *)&(rpc_pkt.u.reply.data[19]); } else { /* NFS_V3 */-- 2.39.2
And looking at the other changes here, there is really _no_ code which adds NFSv1 support.
So what is this patch doing? The only thing which it does is that for NFSv1 requests it does NFSv2 calls. On every place is just check that choosen_nfs_version is not NFS_V3.
Which just basically duplicates NFSv2 to be used two times.
I would suggest to revisit this patch (who reviewed it at all?) and either fix it or revert it. And of course properly test it. (And I really curious where you find NFSv1 server because Linux has already removed also NFSv2 support from userspace...)
Soo. I had a look at RFC 1094 and this patch adds version one of the mount protocol. I am quite unhappy that we got into this state, but the company I worked for uses the term NFSv1 for this in all their configuration tools etc. What would you suggest to improve this situation?
Suggestion: Revert this one patch, then figure out what is needed to be supported (describe all details what kind of protocol and what packets needs to be send and received), find a way and discuss how to implement it and prepare patch for the review.
Here comes a big problem: "find a way and discuss how to implement it and prepare patch for the review." I am not sure if there is really someone interested in discussion about this topic and it is even harder to find someone that reviews U-Boot patches. And this is not specific to networking stuff.
I can prepare a patch that reworks the current implementation (without a revert) and send it out.
If you, who sent this patch, are unhappy about this patch, and also Peter and me pointed issues, then this patch really should not have land into git master branch in this form. It means that nobody is happy with this patch.
To clarify a fact: I am happy with the patch. I am unhappy with the wrong "naming" that my old employer used and influenced this patch. Also the company is shipping thousands of devices per year where these patches are used.
Further clean-ups and clarifications to the support here in terms of what it does and doesn't provide are good. But to the point on reviews, yes, I do wish we had more people interested in various areas, and with time to devote to reviewing code as well. Sadly, we don't always, and I took this particular set of patches as being small enough of a global impact while (hopefully!) making future contributions both in this area and the related platforms using it more likely.
-- Tom
I can help with reviews in this area. In this case, please CC me.
On Tuesday 13 June 2023 09:55:26 Peter Robinson wrote:
I can help with reviews in this area. In this case, please CC me.
I suggest adding yourself to the MAINTAINERS file as a reviewer, people won't know you've offered and in a year Tom may not remember.
Once people stop sending me unrelated u-boot emails then I can add myself to another reviewer area in u-boot files.
On Sat, Jun 24, 2023 at 11:08:59AM +0200, Pali Rohár wrote:
On Tuesday 13 June 2023 09:55:26 Peter Robinson wrote:
I can help with reviews in this area. In this case, please CC me.
I suggest adding yourself to the MAINTAINERS file as a reviewer, people won't know you've offered and in a year Tom may not remember.
Once people stop sending me unrelated u-boot emails then I can add myself to another reviewer area in u-boot files.
Were you going to follow up on my request for a patch to .get_maintainers.conf that tweaks the cut-off for git commits to your liking? I think that's where that particular issue stalled out.
On Saturday 24 June 2023 12:57:58 Tom Rini wrote:
On Sat, Jun 24, 2023 at 11:08:59AM +0200, Pali Rohár wrote:
On Tuesday 13 June 2023 09:55:26 Peter Robinson wrote:
I can help with reviews in this area. In this case, please CC me.
I suggest adding yourself to the MAINTAINERS file as a reviewer, people won't know you've offered and in a year Tom may not remember.
Once people stop sending me unrelated u-boot emails then I can add myself to another reviewer area in u-boot files.
Were you going to follow up on my request for a patch to .get_maintainers.conf that tweaks the cut-off for git commits to your liking? I think that's where that particular issue stalled out.
Me? Not, I'm not maintainer here.
Do you really think that I'm willing to do any fix in this area now if are writing me that locating u-boot commits which are breaking stuff and proposing their revert until proper fix is ready, is not something you would accept from me?
Well, in this case you know the best how to fix this particular issue, so do it yourself. It is clear that you would not accept any my change here, so I'm not going to spend time on this issue as it would be just waste of my free time.
On Sun, Jun 25, 2023 at 05:23:34PM +0200, Pali Rohár wrote:
On Saturday 24 June 2023 12:57:58 Tom Rini wrote:
On Sat, Jun 24, 2023 at 11:08:59AM +0200, Pali Rohár wrote:
On Tuesday 13 June 2023 09:55:26 Peter Robinson wrote:
I can help with reviews in this area. In this case, please CC me.
I suggest adding yourself to the MAINTAINERS file as a reviewer, people won't know you've offered and in a year Tom may not remember.
Once people stop sending me unrelated u-boot emails then I can add myself to another reviewer area in u-boot files.
Were you going to follow up on my request for a patch to .get_maintainers.conf that tweaks the cut-off for git commits to your liking? I think that's where that particular issue stalled out.
Me? Not, I'm not maintainer here.
Do you really think that I'm willing to do any fix in this area now if are writing me that locating u-boot commits which are breaking stuff and proposing their revert until proper fix is ready, is not something you would accept from me?
Well, in this case you know the best how to fix this particular issue, so do it yourself. It is clear that you would not accept any my change here, so I'm not going to spend time on this issue as it would be just waste of my free time.
I have literally no idea what your cut-off point is for "git log says I care about this file, but it's wrong" is. So I can't come up with --git-min-signatures or --git-since that would make you happy, and in turn the one or two lines to add to the existing .git_maintainer.conf file.
To clarify a fact: I am happy with the patch. I am unhappy with the wrong "naming" that my old employer used and influenced this patch. Also the company is shipping thousands of devices per year where these patches are used.
Further clean-ups and clarifications to the support here in terms of what it does and doesn't provide are good. But to the point on reviews, yes, I do wish we had more people interested in various areas, and with time to devote to reviewing code as well. Sadly, we don't always, and I took this particular set of patches as being small enough of a global impact while (hopefully!) making future contributions both in this area and the related platforms using it more likely.
I understand your sentiment but even feedback for things like having it behind a Kconfig option to opt into, especially for a default on option, and CI weren't even addressed, it was just landed without additional revisions. I feel some of those negate the "just land it" sentiment.
Peter
Am Di., 13. Juni 2023 um 13:24 Uhr schrieb Peter Robinson pbrobinson@gmail.com:
To clarify a fact: I am happy with the patch. I am unhappy with the wrong "naming" that my old employer used and influenced this patch. Also the company is shipping thousands of devices per year where these patches are used.
Further clean-ups and clarifications to the support here in terms of what it does and doesn't provide are good. But to the point on reviews, yes, I do wish we had more people interested in various areas, and with time to devote to reviewing code as well. Sadly, we don't always, and I took this particular set of patches as being small enough of a global impact while (hopefully!) making future contributions both in this area and the related platforms using it more likely.
I understand your sentiment but even feedback for things like having it behind a Kconfig option to opt into, especially for a default on option, and CI weren't even addressed, it was just landed without additional revisions. I feel some of those negate the "just land it" sentiment.
Let's move a little bit back in time when I send out V2 of this series. When reading the cover letter thread again https://lore.kernel.org/u-boot/20230310095155.29832-1-christian.gmeiner@gmai... you had a very strong opinion against it Peter.
After that I lost motivation on working on this series as I had the feeling you will always be against adding legacy stuff that is still used today. Tom had taken it and committed it to the master branch.
Peter, If you feel better let's revert all the changes.
On Tue, Jun 13, 2023 at 12:24:44PM +0100, Peter Robinson wrote:
To clarify a fact: I am happy with the patch. I am unhappy with the wrong "naming" that my old employer used and influenced this patch. Also the company is shipping thousands of devices per year where these patches are used.
Further clean-ups and clarifications to the support here in terms of what it does and doesn't provide are good. But to the point on reviews, yes, I do wish we had more people interested in various areas, and with time to devote to reviewing code as well. Sadly, we don't always, and I took this particular set of patches as being small enough of a global impact while (hopefully!) making future contributions both in this area and the related platforms using it more likely.
I understand your sentiment but even feedback for things like having it behind a Kconfig option to opt into, especially for a default on option, and CI weren't even addressed, it was just landed without additional revisions. I feel some of those negate the "just land it" sentiment.
I went back and forth on if it should get a Kconfig option, but the size increase was very small (200-300 bytes) which is below the "gate this for size" reasons. I have a harder time with "gate this for security reasons" when "go modify memory however you want" is a default enabled command, and if you're already using NFS in your bootloader you had better have already taken some precautions. But I'm also agreeable with your RFC to disable NFS command by default.
Peter
On Sunday 11 June 2023 17:24:50 Christian Gmeiner wrote:
Am So., 11. Juni 2023 um 17:10 Uhr schrieb Pali Rohár pali@kernel.org:
On Sunday 11 June 2023 16:57:07 Christian Gmeiner wrote:
Hello
Hello! I must admit that this patch is broken and does not add any NFSv1 support. Just look below....
So .. let see what happend here.
On Friday 10 March 2023 10:51:55 Christian Gmeiner wrote:
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
NFSv1 support added by Christian Gmeiner, Thomas Rienoessl, September 27, 2018. As of now, NFSv3 is the default choice. if the server does not support NFSv3, we fall back to versions 2 or 1.
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info
net/nfs.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c index 21cae52f35..7a8887ef23 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -26,6 +26,10 @@
- NFSv2 is still used by default. But if server does not support NFSv2, then
- NFSv3 is used, if available on NFS server. */
+/* NOTE 5: NFSv1 support added by Christian Gmeiner, Thomas Rienoessl,
- September 27, 2018. As of now, NFSv3 is the default choice. If the server
- does not support NFSv3, we fall back to versions 2 or 1. */
#include <common.h> #include <command.h> #include <display_options.h> @@ -78,6 +82,7 @@ static char nfs_path_buff[2048];
enum nfs_version { NFS_UNKOWN = 0,
NFS_V1 = 1, NFS_V2 = 2, NFS_V3 = 3,}; @@ -192,6 +197,7 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) switch (rpc_prog) { case PROG_NFS: switch (choosen_nfs_version) {
case NFS_V1: case NFS_V2: rpc_pkt.u.call.vers = htonl(2);So if NFSv1 is chosen then this code uses NFSv2. This is either rebasing problem or just prove that this patch does not add any NFSv1 support.
break;@@ -205,8 +211,26 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) break; } break;
case PROG_PORTMAP: case PROG_MOUNT:
switch (choosen_nfs_version) {case NFS_V1:rpc_pkt.u.call.vers = htonl(1);break;And later here for NFSv1 we are trying to use Mount Server, which NFSv1 did not use at all. So this patch really does not have to work with old NFSv1 servers.
Instead NFSv1 uses NFSPROC_ROOT RPC call exported by NFS server. (See that this RPC call is deprecated in NFSv2 and MNT server is used in NFSv2 instead.)
MNTv1 is service used by the NFSv2 and it returns NFSv2 file handles (not NFSv1 file handles). MNTv2 is also used by NFSv2 and as addition to MNTv1, it adds DIRPATH rpc call. So if NFSv2 does not need to use DIRPATH then it is fine to use just MNTv1 in NFSv2.
case NFS_V2:rpc_pkt.u.call.vers = htonl(2);break;case NFS_V3:rpc_pkt.u.call.vers = htonl(3);break;case NFS_UNKOWN:/* nothing to do */break;}break;case PROG_PORTMAP: default: rpc_pkt.u.call.vers = htonl(2); /* portmapper is version 2 */ }@@ -311,7 +335,7 @@ static void nfs_readlink_req(void) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); } else { /* NFS_V3 */@@ -340,7 +364,7 @@ static void nfs_lookup_req(char *fname) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, dirfh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(fnamelen);@@ -380,7 +404,7 @@ static void nfs_read_req(int offset, int readlen) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(offset);@@ -410,13 +434,13 @@ static void nfs_send(void)
switch (nfs_state) { case STATE_PRCLOOKUP_PROG_MOUNT_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_MOUNT, 1); else /* NFS_V3 */ rpc_lookup_req(PROG_MOUNT, 3); break; case STATE_PRCLOOKUP_PROG_NFS_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_NFS, 2); else /* NFS_V3 */ rpc_lookup_req(PROG_NFS, 3);@@ -457,7 +481,7 @@ static int rpc_handle_error(struct rpc_t *rpc_pkt) const int min = ntohl(rpc_pkt->u.reply.data[0]); const int max = ntohl(rpc_pkt->u.reply.data[1]);
if (max < NFS_V2 || max > NFS_V3 || min > NFS_V3) {
if (max < NFS_V1 || max > NFS_V3 || min > NFS_V3) { puts("*** ERROR: NFS version not supported"); debug(": Requested: V%d, accepted: min V%d - max V%d\n", choosen_nfs_version,@@ -588,7 +612,7 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len) if (ret) return ret;
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + NFS_FHSIZE) > len) return -NFS_RPC_DROP; memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);@@ -712,7 +736,7 @@ static int nfs_read_reply(uchar *pkt, unsigned len) if (!(nfs_offset % ((NFS_READ_SIZE / 2) * 10))) putc('#');
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { rlen = ntohl(rpc_pkt.u.reply.data[18]); data_ptr = (uchar *)&(rpc_pkt.u.reply.data[19]); } else { /* NFS_V3 */-- 2.39.2
And looking at the other changes here, there is really _no_ code which adds NFSv1 support.
So what is this patch doing? The only thing which it does is that for NFSv1 requests it does NFSv2 calls. On every place is just check that choosen_nfs_version is not NFS_V3.
Which just basically duplicates NFSv2 to be used two times.
I would suggest to revisit this patch (who reviewed it at all?) and either fix it or revert it. And of course properly test it. (And I really curious where you find NFSv1 server because Linux has already removed also NFSv2 support from userspace...)
Soo. I had a look at RFC 1094 and this patch adds version one of the mount protocol. I am quite unhappy that we got into this state, but the company I worked for uses the term NFSv1 for this in all their configuration tools etc. What would you suggest to improve this situation?
Suggestion: Revert this one patch, then figure out what is needed to be supported (describe all details what kind of protocol and what packets needs to be send and received), find a way and discuss how to implement it and prepare patch for the review.
Here comes a big problem: "find a way and discuss how to implement it and prepare patch for the review." I am not sure if there is really someone interested in discussion about this topic and it is even harder to find someone that reviews U-Boot patches. And this is not specific to networking stuff.
I can prepare a patch that reworks the current implementation (without a revert) and send it out.
So how to handle this issue? Release date of u-boot is coming and this issue have not been addressed or discussed yet.
As I wrote I can help in this area, but I doubt that we will be able to everything before release.
Tom, could you do something here?
If you, who sent this patch, are unhappy about this patch, and also Peter and me pointed issues, then this patch really should not have land into git master branch in this form. It means that nobody is happy with this patch.
To clarify a fact: I am happy with the patch. I am unhappy with the wrong "naming" that my old employer used and influenced this patch. Also the company is shipping thousands of devices per year where these patches are used.
-- greets -- Christian Gmeiner, MSc
On Sat, Jun 24, 2023 at 11:05:11AM +0200, Pali Rohár wrote:
On Sunday 11 June 2023 17:24:50 Christian Gmeiner wrote:
Am So., 11. Juni 2023 um 17:10 Uhr schrieb Pali Rohár pali@kernel.org:
On Sunday 11 June 2023 16:57:07 Christian Gmeiner wrote:
Hello
Hello! I must admit that this patch is broken and does not add any NFSv1 support. Just look below....
So .. let see what happend here.
On Friday 10 March 2023 10:51:55 Christian Gmeiner wrote:
From: Thomas RIENOESSL thomas.rienoessl@bachmann.info
NFSv1 support added by Christian Gmeiner, Thomas Rienoessl, September 27, 2018. As of now, NFSv3 is the default choice. if the server does not support NFSv3, we fall back to versions 2 or 1.
Signed-off-by: Thomas RIENOESSL thomas.rienoessl@bachmann.info
net/nfs.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c index 21cae52f35..7a8887ef23 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -26,6 +26,10 @@
- NFSv2 is still used by default. But if server does not support NFSv2, then
- NFSv3 is used, if available on NFS server. */
+/* NOTE 5: NFSv1 support added by Christian Gmeiner, Thomas Rienoessl,
- September 27, 2018. As of now, NFSv3 is the default choice. If the server
- does not support NFSv3, we fall back to versions 2 or 1. */
#include <common.h> #include <command.h> #include <display_options.h> @@ -78,6 +82,7 @@ static char nfs_path_buff[2048];
enum nfs_version { NFS_UNKOWN = 0,
NFS_V1 = 1, NFS_V2 = 2, NFS_V3 = 3,}; @@ -192,6 +197,7 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) switch (rpc_prog) { case PROG_NFS: switch (choosen_nfs_version) {
case NFS_V1: case NFS_V2: rpc_pkt.u.call.vers = htonl(2);So if NFSv1 is chosen then this code uses NFSv2. This is either rebasing problem or just prove that this patch does not add any NFSv1 support.
break;@@ -205,8 +211,26 @@ static void rpc_req(int rpc_prog, int rpc_proc, uint32_t *data, int datalen) break; } break;
case PROG_PORTMAP: case PROG_MOUNT:
switch (choosen_nfs_version) {case NFS_V1:rpc_pkt.u.call.vers = htonl(1);break;And later here for NFSv1 we are trying to use Mount Server, which NFSv1 did not use at all. So this patch really does not have to work with old NFSv1 servers.
Instead NFSv1 uses NFSPROC_ROOT RPC call exported by NFS server. (See that this RPC call is deprecated in NFSv2 and MNT server is used in NFSv2 instead.)
MNTv1 is service used by the NFSv2 and it returns NFSv2 file handles (not NFSv1 file handles). MNTv2 is also used by NFSv2 and as addition to MNTv1, it adds DIRPATH rpc call. So if NFSv2 does not need to use DIRPATH then it is fine to use just MNTv1 in NFSv2.
case NFS_V2:rpc_pkt.u.call.vers = htonl(2);break;case NFS_V3:rpc_pkt.u.call.vers = htonl(3);break;case NFS_UNKOWN:/* nothing to do */break;}break;case PROG_PORTMAP: default: rpc_pkt.u.call.vers = htonl(2); /* portmapper is version 2 */ }@@ -311,7 +335,7 @@ static void nfs_readlink_req(void) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); } else { /* NFS_V3 */@@ -340,7 +364,7 @@ static void nfs_lookup_req(char *fname) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, dirfh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(fnamelen);@@ -380,7 +404,7 @@ static void nfs_read_req(int offset, int readlen) p = &(data[0]); p = rpc_add_credentials(p);
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { memcpy(p, filefh, NFS_FHSIZE); p += (NFS_FHSIZE / 4); *p++ = htonl(offset);@@ -410,13 +434,13 @@ static void nfs_send(void)
switch (nfs_state) { case STATE_PRCLOOKUP_PROG_MOUNT_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_MOUNT, 1); else /* NFS_V3 */ rpc_lookup_req(PROG_MOUNT, 3); break; case STATE_PRCLOOKUP_PROG_NFS_REQ:
if (choosen_nfs_version == NFS_V2)
if (choosen_nfs_version != NFS_V3) rpc_lookup_req(PROG_NFS, 2); else /* NFS_V3 */ rpc_lookup_req(PROG_NFS, 3);@@ -457,7 +481,7 @@ static int rpc_handle_error(struct rpc_t *rpc_pkt) const int min = ntohl(rpc_pkt->u.reply.data[0]); const int max = ntohl(rpc_pkt->u.reply.data[1]);
if (max < NFS_V2 || max > NFS_V3 || min > NFS_V3) {
if (max < NFS_V1 || max > NFS_V3 || min > NFS_V3) { puts("*** ERROR: NFS version not supported"); debug(": Requested: V%d, accepted: min V%d - max V%d\n", choosen_nfs_version,@@ -588,7 +612,7 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len) if (ret) return ret;
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + NFS_FHSIZE) > len) return -NFS_RPC_DROP; memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);@@ -712,7 +736,7 @@ static int nfs_read_reply(uchar *pkt, unsigned len) if (!(nfs_offset % ((NFS_READ_SIZE / 2) * 10))) putc('#');
if (choosen_nfs_version == NFS_V2) {
if (choosen_nfs_version != NFS_V3) { rlen = ntohl(rpc_pkt.u.reply.data[18]); data_ptr = (uchar *)&(rpc_pkt.u.reply.data[19]); } else { /* NFS_V3 */-- 2.39.2
And looking at the other changes here, there is really _no_ code which adds NFSv1 support.
So what is this patch doing? The only thing which it does is that for NFSv1 requests it does NFSv2 calls. On every place is just check that choosen_nfs_version is not NFS_V3.
Which just basically duplicates NFSv2 to be used two times.
I would suggest to revisit this patch (who reviewed it at all?) and either fix it or revert it. And of course properly test it. (And I really curious where you find NFSv1 server because Linux has already removed also NFSv2 support from userspace...)
Soo. I had a look at RFC 1094 and this patch adds version one of the mount protocol. I am quite unhappy that we got into this state, but the company I worked for uses the term NFSv1 for this in all their configuration tools etc. What would you suggest to improve this situation?
Suggestion: Revert this one patch, then figure out what is needed to be supported (describe all details what kind of protocol and what packets needs to be send and received), find a way and discuss how to implement it and prepare patch for the review.
Here comes a big problem: "find a way and discuss how to implement it and prepare patch for the review." I am not sure if there is really someone interested in discussion about this topic and it is even harder to find someone that reviews U-Boot patches. And this is not specific to networking stuff.
I can prepare a patch that reworks the current implementation (without a revert) and send it out.
So how to handle this issue? Release date of u-boot is coming and this issue have not been addressed or discussed yet.
As I wrote I can help in this area, but I doubt that we will be able to everything before release.
Tom, could you do something here?
I don't see a reason to revert this patch as Christian has promised a follow-up to address things. Using the nfs command (not to be confused with root=/dev/nfs) isn't part of any in-tree default environment so I don't have a concern about this exposing some security issue.
This patch series adds support for NFSv1 and is more or less a rebased version of an older series.
During V1 there was a discussion if it really makes sense to bring more features into the network stack of U-Boot as it is just a bootloader. As TCP support landed I thought I might give this patch series another try.
gentle ping
This patch series adds support for NFSv1 and is more or less a rebased version of an older series.
During V1 there was a discussion if it really makes sense to bring more features into the network stack of U-Boot as it is just a bootloader. As TCP support landed I thought I might give this patch series another try.
gentle ping
Adding some more people to CC - maybe this helps.
On Fri, Mar 10, 2023 at 9:52 AM Christian Gmeiner christian.gmeiner@gmail.com wrote:
This patch series adds support for NFSv1 and is more or less a rebased version of an older series.
During V1 there was a discussion if it really makes sense to bring more features into the network stack of U-Boot as it is just a bootloader. As TCP support landed I thought I might give this patch series another try.
So the real question is what use does this bring? TCP is useful because it can enable support for modern features like UEFI HTTP boot. What is the use of NFSc1? The Linux kernel is removing support for NFSv2 and according to wikipedia v1 was only ever used internally to Sun so I'm not sure what the wider use of this functionality would be?
Peter
[1] https://www.phoronix.com/news/Linux-6.2-NFSD [2] https://en.wikipedia.org/wiki/Network_File_System#Versions_and_variations
Hi Peter,
This patch series adds support for NFSv1 and is more or less a rebased version of an older series.
During V1 there was a discussion if it really makes sense to bring more features into the network stack of U-Boot as it is just a bootloader. As TCP support landed I thought I might give this patch series another try.
So the real question is what use does this bring? TCP is useful because it can enable support for modern features like UEFI HTTP boot. What is the use of NFSc1? The Linux kernel is removing support for NFSv2 and according to wikipedia v1 was only ever used internally to Sun so I'm not sure what the wider use of this functionality would be?
You know there are other operating systems used in the wild. One of them is vxworks. The company I work for supports software versions for more than 10 years and customers do not want to update to newer software versions as the process of approval for wind turbines/parks is very costly and time intensive. For security issues we provide patches for older versions that the customers are switching to.
We are shipping devices right now which are based on vxworks 5. Some customers are using our "processor modules" as NFS server for e.g. other "processor modules" to boot from. If you want/need I can go into further details about our use case.
So I know It is a quite specific use case but we want to upstream most of our downstream patches.
Btw. if this patch series will not land does this mean NFSv2 will die too soon?
On Fri, Mar 24, 2023 at 9:35 AM Christian Gmeiner christian.gmeiner@gmail.com wrote:
Hi Peter,
This patch series adds support for NFSv1 and is more or less a rebased version of an older series.
During V1 there was a discussion if it really makes sense to bring more features into the network stack of U-Boot as it is just a bootloader. As TCP support landed I thought I might give this patch series another try.
So the real question is what use does this bring? TCP is useful because it can enable support for modern features like UEFI HTTP boot. What is the use of NFSc1? The Linux kernel is removing support for NFSv2 and according to wikipedia v1 was only ever used internally to Sun so I'm not sure what the wider use of this functionality would be?
You know there are other operating systems used in the wild. One of them is vxworks. The company I work for supports software versions for more than 10 years and customers do not want to update to newer software versions as the process of approval for wind turbines/parks is very costly and time intensive. For security issues we provide patches for older versions that the customers are switching to.
Are there deployments that are updating to new versions of U-Boot that require NFSv1 though?
We are shipping devices right now which are based on vxworks 5. Some customers are using our "processor modules" as NFS server for e.g. other "processor modules" to boot from. If you want/need I can go into further details about our use case.
So they're using a 25 year old version of vxworks with the latest version of U-Boot? And vxworks supports only NFSv1, and not NFSv2, which has been out since 1989 and was the first version that even Sun supported publicly?
So I know It is a quite specific use case but we want to upstream most of our downstream patches.
Btw. if this patch series will not land does this mean NFSv2 will die too soon?
At least NFSv2 is supported to some degree whereas it seems NFSv1 is something that's special to vxworks, how are people supposed to test it? I don't see any tests in your patchset and I'm not sure how it's useful outside of the legacy vxmworks use case.
At the very least this should be behind a Kconfig option.
Peter
Hi Peter,
Am Fr., 24. März 2023 um 11:10 Uhr schrieb Peter Robinson pbrobinson@gmail.com:
On Fri, Mar 24, 2023 at 9:35 AM Christian Gmeiner christian.gmeiner@gmail.com wrote:
Hi Peter,
This patch series adds support for NFSv1 and is more or less a rebased version of an older series.
During V1 there was a discussion if it really makes sense to bring more features into the network stack of U-Boot as it is just a bootloader. As TCP support landed I thought I might give this patch series another try.
So the real question is what use does this bring? TCP is useful because it can enable support for modern features like UEFI HTTP boot. What is the use of NFSc1? The Linux kernel is removing support for NFSv2 and according to wikipedia v1 was only ever used internally to Sun so I'm not sure what the wider use of this functionality would be?
You know there are other operating systems used in the wild. One of them is vxworks. The company I work for supports software versions for more than 10 years and customers do not want to update to newer software versions as the process of approval for wind turbines/parks is very costly and time intensive. For security issues we provide patches for older versions that the customers are switching to.
Are there deployments that are updating to new versions of U-Boot that require NFSv1 though?
Sadly, yes .. There are so called drop-in replacements for "processor modules" where we update the old design (e.g. AMD LX800 based 26G MHz one) to a new recent Intel SoC. Our customer can order a replacement HW for the old LX800 based design and they get the new one with fully compatible software.
We are shipping devices right now which are based on vxworks 5. Some customers are using our "processor modules" as NFS server for e.g. other "processor modules" to boot from. If you want/need I can go into further details about our use case.
So they're using a 25 year old version of vxworks with the latest version of U-Boot? And vxworks supports only NFSv1, and not NFSv2, which has been out since 1989 and was the first version that even Sun supported publicly?
There are newer versions of NFS supported on more recent vxworks versions but as our customers do not want to change software of an approved wind turbine we need to support the old feature.
Also we are allowed to include only critical fixes in patch version, which can be used without the whole approval process. For any new feature our customer would need to redo the whole approval process.
So I know It is a quite specific use case but we want to upstream most of our downstream patches.
Btw. if this patch series will not land does this mean NFSv2 will die too soon?
At least NFSv2 is supported to some degree whereas it seems NFSv1 is something that's special to vxworks, how are people supposed to test it? I don't see any tests in your patchset and I'm not sure how it's useful outside of the legacy vxmworks use case.
I thought that this unit test topic will pop up and we have written some last week. I will include them in the next version of this patchset.
Btw why are there no NFSv2, NFSv3 unit tests in U-Boot? How are people supposed to test it?
At the very least this should be behind a Kconfig option.
Works for me.
On Fri, Mar 24, 2023 at 10:42 AM Christian Gmeiner christian.gmeiner@gmail.com wrote:
Hi Peter,
Am Fr., 24. März 2023 um 11:10 Uhr schrieb Peter Robinson pbrobinson@gmail.com:
On Fri, Mar 24, 2023 at 9:35 AM Christian Gmeiner christian.gmeiner@gmail.com wrote:
Hi Peter,
This patch series adds support for NFSv1 and is more or less a rebased version of an older series.
During V1 there was a discussion if it really makes sense to bring more features into the network stack of U-Boot as it is just a bootloader. As TCP support landed I thought I might give this patch series another try.
So the real question is what use does this bring? TCP is useful because it can enable support for modern features like UEFI HTTP boot. What is the use of NFSc1? The Linux kernel is removing support for NFSv2 and according to wikipedia v1 was only ever used internally to Sun so I'm not sure what the wider use of this functionality would be?
You know there are other operating systems used in the wild. One of them is vxworks. The company I work for supports software versions for more than 10 years and customers do not want to update to newer software versions as the process of approval for wind turbines/parks is very costly and time intensive. For security issues we provide patches for older versions that the customers are switching to.
Are there deployments that are updating to new versions of U-Boot that require NFSv1 though?
Sadly, yes .. There are so called drop-in replacements for "processor modules" where we update the old design (e.g. AMD LX800 based 26G MHz one) to a new recent Intel SoC. Our customer can order a replacement HW for the old LX800 based design and they get the new one with fully compatible software.
We are shipping devices right now which are based on vxworks 5. Some customers are using our "processor modules" as NFS server for e.g. other "processor modules" to boot from. If you want/need I can go into further details about our use case.
So they're using a 25 year old version of vxworks with the latest version of U-Boot? And vxworks supports only NFSv1, and not NFSv2, which has been out since 1989 and was the first version that even Sun supported publicly?
There are newer versions of NFS supported on more recent vxworks versions but as our customers do not want to change software of an approved wind turbine we need to support the old feature.
Also we are allowed to include only critical fixes in patch version, which can be used without the whole approval process. For any new feature our customer would need to redo the whole approval process.
Yet they allow you to update the entire version of U-Boot to the latest one?
I understand how the process works when they don't want to change things, but it doesn't make sense to update U-Boot to the latest version yet not other pieces of the infrastructure. NFSv1 was basically obsolete from the outset because Sun's first public release was v2.
It doesn't make sense to me to upstream a long obsolete version, and no doubt insecure, version of a protocol which is seemingly used by a single customer of yours.
So I know It is a quite specific use case but we want to upstream most of our downstream patches.
Btw. if this patch series will not land does this mean NFSv2 will die too soon?
At least NFSv2 is supported to some degree whereas it seems NFSv1 is something that's special to vxworks, how are people supposed to test it? I don't see any tests in your patchset and I'm not sure how it's useful outside of the legacy vxmworks use case.
I thought that this unit test topic will pop up and we have written some last week. I will include them in the next version of this patchset.
Btw why are there no NFSv2, NFSv3 unit tests in U-Boot? How are people supposed to test it?
The NFSv2 code was contributed before it was decided that new functionality should come with tests that can run in CI.
Where would the NFSv1 server come from for this for testing in CI?
At the very least this should be behind a Kconfig option.
Works for me.
-- greets -- Christian Gmeiner, MSc
Hi Peter
Hi Peter,
Am Fr., 24. März 2023 um 11:10 Uhr schrieb Peter Robinson pbrobinson@gmail.com:
On Fri, Mar 24, 2023 at 9:35 AM Christian Gmeiner christian.gmeiner@gmail.com wrote:
Hi Peter,
This patch series adds support for NFSv1 and is more or less a rebased version of an older series.
During V1 there was a discussion if it really makes sense to bring more features into the network stack of U-Boot as it is just a bootloader. As TCP support landed I thought I might give this patch series another try.
So the real question is what use does this bring? TCP is useful because it can enable support for modern features like UEFI HTTP boot. What is the use of NFSc1? The Linux kernel is removing support for NFSv2 and according to wikipedia v1 was only ever used internally to Sun so I'm not sure what the wider use of this functionality would be?
You know there are other operating systems used in the wild. One of them is vxworks. The company I work for supports software versions for more than 10 years and customers do not want to update to newer software versions as the process of approval for wind turbines/parks is very costly and time intensive. For security issues we provide patches for older versions that the customers are switching to.
Are there deployments that are updating to new versions of U-Boot that require NFSv1 though?
Sadly, yes .. There are so called drop-in replacements for "processor modules" where we update the old design (e.g. AMD LX800 based 26G MHz one) to a new recent Intel SoC. Our customer can order a replacement HW for the old LX800 based design and they get the new one with fully compatible software.
We are shipping devices right now which are based on vxworks 5. Some customers are using our "processor modules" as NFS server for e.g. other "processor modules" to boot from. If you want/need I can go into further details about our use case.
So they're using a 25 year old version of vxworks with the latest version of U-Boot? And vxworks supports only NFSv1, and not NFSv2, which has been out since 1989 and was the first version that even Sun supported publicly?
There are newer versions of NFS supported on more recent vxworks versions but as our customers do not want to change software of an approved wind turbine we need to support the old feature.
Also we are allowed to include only critical fixes in patch version, which can be used without the whole approval process. For any new feature our customer would need to redo the whole approval process.
Yet they allow you to update the entire version of U-Boot to the latest one?
U-Boot is used as coreboot payload and both together are the bios. None of our customers do bios updates in the field as the risk of bricking the device is too high.
I understand how the process works when they don't want to change things, but it doesn't make sense to update U-Boot to the latest version yet not other pieces of the infrastructure. NFSv1 was basically obsolete from the outset because Sun's first public release was v2.
It doesn't make sense to me to upstream a long obsolete version, and no doubt insecure, version of a protocol which is seemingly used by a single customer of yours.
I am fine with that decision .. but I hope you know that NFSv2 is an insecure version too!
So I know It is a quite specific use case but we want to upstream most of our downstream patches.
Btw. if this patch series will not land does this mean NFSv2 will die too soon?
At least NFSv2 is supported to some degree whereas it seems NFSv1 is something that's special to vxworks, how are people supposed to test it? I don't see any tests in your patchset and I'm not sure how it's useful outside of the legacy vxmworks use case.
I thought that this unit test topic will pop up and we have written some last week. I will include them in the next version of this patchset.
Btw why are there no NFSv2, NFSv3 unit tests in U-Boot? How are people supposed to test it?
The NFSv2 code was contributed before it was decided that new functionality should come with tests that can run in CI.
Where would the NFSv1 server come from for this for testing in CI?
You convinced me to stop thinking about upstreaming NFSv1 support.
On Fri, Mar 24, 2023 at 1:05 PM Christian Gmeiner christian.gmeiner@gmail.com wrote:
Hi Peter
Hi Peter,
Am Fr., 24. März 2023 um 11:10 Uhr schrieb Peter Robinson pbrobinson@gmail.com:
On Fri, Mar 24, 2023 at 9:35 AM Christian Gmeiner christian.gmeiner@gmail.com wrote:
Hi Peter,
> > This patch series adds support for NFSv1 and is more > or less a rebased version of an older series. > > During V1 there was a discussion if it really makes sense > to bring more features into the network stack of U-Boot as it > is just a bootloader. As TCP support landed I thought I might > give this patch series another try.
So the real question is what use does this bring? TCP is useful because it can enable support for modern features like UEFI HTTP boot. What is the use of NFSc1? The Linux kernel is removing support for NFSv2 and according to wikipedia v1 was only ever used internally to Sun so I'm not sure what the wider use of this functionality would be?
You know there are other operating systems used in the wild. One of them is vxworks. The company I work for supports software versions for more than 10 years and customers do not want to update to newer software versions as the process of approval for wind turbines/parks is very costly and time intensive. For security issues we provide patches for older versions that the customers are switching to.
Are there deployments that are updating to new versions of U-Boot that require NFSv1 though?
Sadly, yes .. There are so called drop-in replacements for "processor modules" where we update the old design (e.g. AMD LX800 based 26G MHz one) to a new recent Intel SoC. Our customer can order a replacement HW for the old LX800 based design and they get the new one with fully compatible software.
We are shipping devices right now which are based on vxworks 5. Some customers are using our "processor modules" as NFS server for e.g. other "processor modules" to boot from. If you want/need I can go into further details about our use case.
So they're using a 25 year old version of vxworks with the latest version of U-Boot? And vxworks supports only NFSv1, and not NFSv2, which has been out since 1989 and was the first version that even Sun supported publicly?
There are newer versions of NFS supported on more recent vxworks versions but as our customers do not want to change software of an approved wind turbine we need to support the old feature.
Also we are allowed to include only critical fixes in patch version, which can be used without the whole approval process. For any new feature our customer would need to redo the whole approval process.
Yet they allow you to update the entire version of U-Boot to the latest one?
U-Boot is used as coreboot payload and both together are the bios. None of our customers do bios updates in the field as the risk of bricking the device is too high.
I understand how the process works when they don't want to change things, but it doesn't make sense to update U-Boot to the latest version yet not other pieces of the infrastructure. NFSv1 was basically obsolete from the outset because Sun's first public release was v2.
It doesn't make sense to me to upstream a long obsolete version, and no doubt insecure, version of a protocol which is seemingly used by a single customer of yours.
I am fine with that decision .. but I hope you know that NFSv2 is an insecure version too!
Oh I'm well aware of that, it's at least been more widely reviewed and the issues are better known. Personally I've never particularly seen the point of NFS in a firmware at all. These days I think HTTP would provide equivalence of functionality with the better support for things like proxy/caching/CDN and other such functionality.
So I know It is a quite specific use case but we want to upstream most of our downstream patches.
Btw. if this patch series will not land does this mean NFSv2 will die too soon?
At least NFSv2 is supported to some degree whereas it seems NFSv1 is something that's special to vxworks, how are people supposed to test it? I don't see any tests in your patchset and I'm not sure how it's useful outside of the legacy vxmworks use case.
I thought that this unit test topic will pop up and we have written some last week. I will include them in the next version of this patchset.
Btw why are there no NFSv2, NFSv3 unit tests in U-Boot? How are people supposed to test it?
The NFSv2 code was contributed before it was decided that new functionality should come with tests that can run in CI.
Where would the NFSv1 server come from for this for testing in CI?
You convinced me to stop thinking about upstreaming NFSv1 support.
-- thanks -- Christian Gmeiner, MSc
Hi Peter,
On Sat, 25 Mar 2023 at 02:14, Peter Robinson pbrobinson@gmail.com wrote:
On Fri, Mar 24, 2023 at 1:05 PM Christian Gmeiner christian.gmeiner@gmail.com wrote:
Hi Peter
Hi Peter,
Am Fr., 24. März 2023 um 11:10 Uhr schrieb Peter Robinson pbrobinson@gmail.com:
On Fri, Mar 24, 2023 at 9:35 AM Christian Gmeiner christian.gmeiner@gmail.com wrote:
Hi Peter,
> > > > This patch series adds support for NFSv1 and is more > > or less a rebased version of an older series. > > > > During V1 there was a discussion if it really makes sense > > to bring more features into the network stack of U-Boot as it > > is just a bootloader. As TCP support landed I thought I might > > give this patch series another try. > > So the real question is what use does this bring? TCP is useful > because it can enable support for modern features like UEFI HTTP boot. > What is the use of NFSc1? The Linux kernel is removing support for > NFSv2 and according to wikipedia v1 was only ever used internally to > Sun so I'm not sure what the wider use of this functionality would be? >
You know there are other operating systems used in the wild. One of them is vxworks. The company I work for supports software versions for more than 10 years and customers do not want to update to newer software versions as the process of approval for wind turbines/parks is very costly and time intensive. For security issues we provide patches for older versions that the customers are switching to.
Are there deployments that are updating to new versions of U-Boot that require NFSv1 though?
Sadly, yes .. There are so called drop-in replacements for "processor modules" where we update the old design (e.g. AMD LX800 based 26G MHz one) to a new recent Intel SoC. Our customer can order a replacement HW for the old LX800 based design and they get the new one with fully compatible software.
We are shipping devices right now which are based on vxworks 5. Some customers are using our "processor modules" as NFS server for e.g. other "processor modules" to boot from. If you want/need I can go into further details about our use case.
So they're using a 25 year old version of vxworks with the latest version of U-Boot? And vxworks supports only NFSv1, and not NFSv2, which has been out since 1989 and was the first version that even Sun supported publicly?
There are newer versions of NFS supported on more recent vxworks versions but as our customers do not want to change software of an approved wind turbine we need to support the old feature.
Also we are allowed to include only critical fixes in patch version, which can be used without the whole approval process. For any new feature our customer would need to redo the whole approval process.
Yet they allow you to update the entire version of U-Boot to the latest one?
U-Boot is used as coreboot payload and both together are the bios. None of our customers do bios updates in the field as the risk of bricking the device is too high.
I understand how the process works when they don't want to change things, but it doesn't make sense to update U-Boot to the latest version yet not other pieces of the infrastructure. NFSv1 was basically obsolete from the outset because Sun's first public release was v2.
It doesn't make sense to me to upstream a long obsolete version, and no doubt insecure, version of a protocol which is seemingly used by a single customer of yours.
I am fine with that decision .. but I hope you know that NFSv2 is an insecure version too!
Oh I'm well aware of that, it's at least been more widely reviewed and the issues are better known. Personally I've never particularly seen the point of NFS in a firmware at all. These days I think HTTP would provide equivalence of functionality with the better support for things like proxy/caching/CDN and other such functionality.
But here we are talking about supporting old software. So it doesn't much matter that HTTP is now available.
Regards, Simon
So I know It is a quite specific use case but we want to upstream most of our downstream patches.
Btw. if this patch series will not land does this mean NFSv2 will die too soon?
At least NFSv2 is supported to some degree whereas it seems NFSv1 is something that's special to vxworks, how are people supposed to test it? I don't see any tests in your patchset and I'm not sure how it's useful outside of the legacy vxmworks use case.
I thought that this unit test topic will pop up and we have written some last week. I will include them in the next version of this patchset.
Btw why are there no NFSv2, NFSv3 unit tests in U-Boot? How are people supposed to test it?
The NFSv2 code was contributed before it was decided that new functionality should come with tests that can run in CI.
Where would the NFSv1 server come from for this for testing in CI?
You convinced me to stop thinking about upstreaming NFSv1 support.
-- thanks -- Christian Gmeiner, MSc
participants (5)
-
Christian Gmeiner -
Pali Rohár -
Peter Robinson -
Simon Glass -
Tom Rini