Re: Hurd NFS translator

Michael Kelly <[email protected]>
Newsgroups gmane.linux.debian.ports.hurd
Message-ID <[email protected]> 
On 21/06/2025 18:18, Samuel Thibault wrote:
> Better validate progressively.

Here are two further patches to add more NFSv3 functionality. They need 
to be applied in sequence 3_3 then 3_4.

Patch v3_3 includes:

1) New code to process the reply from a file creation operation that 
applies equally to CREATE3, MKDIR3, SYMLINK3 and MKNOD3. RFC1813 permits 
a reply without the created file's handle and it might be necessary to 
perform a LOOKUP3 on the created file post-creation. Debian Linux NFSV3 
server actually always returns the handle when compiling gnumach however 
I have tested the case where it might not by adding code (now removed) 
to pretend the handle wasn't present in the reply. I expect therefore 
that the 'skip_returned_stat' function that is added is rarely called.

2) Fixed netfs_attempt_mkdir() to process the NFSv3 reply properly.

Patch v3_4 includes:

1) Fixes netfs_attempt_read(): use the 64 bit value required for v3 
'offset' and process opaque_data part of the reply properly.

2) Fixes netfs_attempt_create_file(): mutex_unlock for unlikely case of 
RPC init failure and processing of v3 reply similarly to mkdir in v3_3 patch

3) The v3 netfs_attempt_create_file() uses the 'exclusive' creation 
protocol which cannot specify the file permission bits or uid/gid. These 
need to be applied post-creation using SETATTR3. There was a missing 
call to do the chmod part  which I've added. Previously the call to 
chown that was present did not consider the return value. I have done so 
but am now wondering if there was a specific reason to discard that error ?

Regards,

Mike.
v3_3.patch (text/x-patch, 2.5 KB) 
diff -ur nfs.submitted/ops.c nfs.new/ops.c
--- nfs.submitted/ops.c	2025-07-13 07:01:38.000000000 +0100
+++ nfs.new/ops.c	2025-07-13 07:02:42.000000000 +0100
@@ -134,6 +134,70 @@
     }
 }
 
+/* advance the reply buffer beyond the 'post_op_attr' (v3) or
+   'fattr' (v2). Populating 'struct stat' for the purposes of
+   advancing the correct number of bytes might seem wasteful
+   but it is not expected that this function will be called
+   often. */
+static int *
+skip_returned_stat (int *p)
+{
+  struct stat st;
+
+  if (protocol_version == 2)
+    return xdr_decode_fattr (p, &st);
+
+  int attrs_exist = ntohl (*p);
+  p++;
+
+  return (attrs_exist ? xdr_decode_fattr (p, &st) : p);
+}
+
+/* The reply to CREATE, MKDIR, SYMLINK and MKNOD in v3 all have
+   the same reply content. It is expected that 'np' is supplied
+   locked and any 'newnp' is also returned locked. */
+static error_t
+process_create_reply (struct iouser *cred,
+		      struct node *np,
+		      const char* name,
+		      struct node **newnp,
+		      int *p)
+{
+  assert_backtrace (protocol_version == 3);
+
+  error_t err = nfs_error_trans (ntohl (*p));
+  p++;
+
+  if (!err)
+    {
+      int handle_follows = ntohl (*p);
+      p++;
+
+      if (handle_follows)
+	{
+	  p = xdr_decode_fhandle (p, newnp);
+	  p = process_returned_stat (*newnp, p, 1);
+	}
+      else
+	/* These will be refetched in LOOKUP */
+	p = skip_returned_stat (p);
+
+      p = process_wcc_stat (np, p, 1);
+
+      if (!handle_follows)
+      {
+	err = netfs_attempt_lookup (cred, np, name, newnp);
+	/* netfs_attempt_lookup always unlocks 'np' so... */
+	pthread_mutex_lock (&np->lock);
+      }
+    }
+  else
+    {
+      p = process_wcc_stat (np, p, 1);
+    }
+
+  return err;
+}
 
 /* Implement the netfs_validate_stat callback as described in
    <hurd/netfs.h>.  */
@@ -740,15 +804,23 @@
   err = conduct_rpc (&rpcbuf, &p);
   if (!err)
     {
-      err = nfs_error_trans (ntohl (*p));
-      p++;
+      if (protocol_version == 2)
+	{
+	  err = nfs_error_trans (ntohl (*p));
+	  p++;
+
+	  if (!err)
+	    {
+	      p = xdr_decode_fhandle (p, &newnp);
+	      p = process_returned_stat (newnp, p, 1);
+	    }
+	}
+      else
+	err = process_create_reply (cred, np, name, &newnp, p);
     }
 
   if (!err)
     {
-      p = xdr_decode_fhandle (p, &newnp);
-      p = process_returned_stat (newnp, p, 1);
-
       /* Did we set the owner correctly?  If not, try, but ignore failures. */
       if (!netfs_validate_stat (newnp, (struct iouser *) -1)
           && newnp->nn_stat.st_uid != owner)
v3_4.patch (text/x-patch, 2.8 KB) 
diff -ur nfs.submitted/ops.c nfs.new/ops.c
--- nfs.submitted/ops.c	2025-07-13 07:02:42.000000000 +0100
+++ nfs.new/ops.c	2025-07-13 07:08:08.000000000 +0100
@@ -526,10 +526,17 @@
         return errno;
 
       p = xdr_encode_fhandle (p, &np->nn->handle);
-      *(p++) = htonl (offset);
-      *(p++) = htonl (thisamt);
       if (protocol_version == 2)
-	*(p++) = 0;
+	{
+	  *(p++) = htonl (offset);
+	  *(p++) = htonl (thisamt);
+	  *(p++) = 0;
+	}
+      else
+	{
+	  p = xdr_encode_64bit (p, offset);
+	  *(p++) = htonl (thisamt);
+	}
 
       err = conduct_rpc (&rpcbuf, &p);
       if (!err)
@@ -553,8 +560,15 @@
 
 	  if (protocol_version == 3)
 	    {
+	      size_t opaque_data_len;
+
 	      eof = ntohl (*p);
 	      p++;
+	      opaque_data_len = ntohl (*p++);
+
+	      /* opaque_len should surely equal trans_len, however... */
+	      if (opaque_data_len < trans_len)
+		trans_len = opaque_data_len;
 	    }
 	  else
 	    eof = (trans_len < thisamt);
@@ -1251,8 +1265,10 @@
 
   p = nfs_initialize_rpc (NFSPROC_CREATE (protocol_version),
 			  cred, 0, &rpcbuf, np, -1);
-  if (! p)
+  if (! p) {
+    pthread_mutex_unlock (&np->lock);
     return errno;
+  }
 
   p = xdr_encode_fhandle (p, &np->nn->handle);
   p = xdr_encode_string (p, name);
@@ -1270,37 +1286,36 @@
     p = xdr_encode_create_state (p, mode, owner);
 
   err = conduct_rpc (&rpcbuf, &p);
-
-  pthread_mutex_unlock (&np->lock);
+  *newnp = 0;
 
   if (!err)
     {
-      err = nfs_error_trans (ntohl (*p));
-      p++;
-      if (!err)
+      if (protocol_version == 2)
 	{
-	  p = xdr_decode_fhandle (p, newnp);
-	  p = process_returned_stat (*newnp, p, 1);
+	  err = nfs_error_trans (ntohl (*p));
+	  p++;
+	  if (!err)
+	    {
+	      p = xdr_decode_fhandle (p, newnp);
+	      p = process_returned_stat (*newnp, p, 1);
+	    }
 	}
-      if (err)
-	*newnp = 0;
-      if (protocol_version == 3)
+      else
+	err = process_create_reply (cred, np, name, newnp, p);
+
+      pthread_mutex_unlock (&np->lock);
+
+      if (*newnp && !netfs_validate_stat (*newnp, (struct iouser *) -1))
 	{
-	  if (*newnp)
-	    pthread_mutex_unlock (&(*newnp)->lock);
-	  pthread_mutex_lock (&np->lock);
-	  p = process_wcc_stat (np, p, 1);
-	  pthread_mutex_unlock (&np->lock);
-	  if (*newnp)
-	    pthread_mutex_lock (&(*newnp)->lock);
-	}
+	  if ((*newnp)->nn_stat.st_uid != owner)
+	    err = netfs_attempt_chown ((struct iouser *) -1, *newnp, owner, (*newnp)->nn_stat.st_gid);
 
-      if (*newnp && !netfs_validate_stat (*newnp, (struct iouser *) -1)
-	  && (*newnp)->nn_stat.st_uid != owner)
-	netfs_attempt_chown ((struct iouser *) -1, *newnp, owner, (*newnp)->nn_stat.st_gid);
+	  if (!err && (*newnp)->nn_stat.st_mode != mode)
+	    err = netfs_attempt_chmod (cred, *newnp, mode);
+	}
     }
   else
-    *newnp = 0;
+    pthread_mutex_unlock (&np->lock);
 
   free (rpcbuf);
   return err;
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.