[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Hurd NFS translator



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.
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)
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;

Reply to: