[PATCH v2 4/4] net: add NFSv1 support
Pali Rohár
pali at kernel.org
Mon Jun 12 19:07:01 CEST 2023
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 at 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 at 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 at 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.
More information about the U-Boot
mailing list