[PATCH v2 4/4] net: add NFSv1 support

[Peter Robinson]

On Sun, Jun 11, 2023 at 1:54 PM Pali Rohár <pali at 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 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...)

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.