[PATCH v2 4/4] net: add NFSv1 support
Pali Rohár
pali at kernel.org
Sun Jun 11 17:10:50 CEST 2023
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.
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.
More information about the U-Boot
mailing list