Re: Hurd NFS translator
On 21/06/2025 18:18, Samuel Thibault wrote:
Better validate progressively.
The fix for processing of SYMLINK and MKNOD involved more change than I
expected. I think that it is now fixed properly with this series of
patches that completes what I intended to add for NFSv3 functionality:
patch 8: To fix the processing of replies to attempt_create_file() I
needed to add another rpc call to SETATTR. I couldn't bring myself to
cut and paste the same code which had already 4 copies so I generalised
what was there. It's quite a large patch but essentially much of it is a
repeated alteration.
patch 9: I finally got to summarise the mutex locking rules for the
module. This patch also generalises the code for making a LOOKUP rpc
call so that it can be used in a slightly different context in a
following patch. I also removed the caching of rpc call results that
were failed for errors other than ENOENT. For example, EPERM doesn't say
anything about the lookup validity for other credentials that might be used.
patch 10: This is a generalisation of process_create_reply() to allow it
to be used for MKNOD and SYMLINK rpc calls. These file types work
differently to others in that a temporary file is created earlier and
its 'struct node' is migrated to the new file later. A few adjustments
to the mutex locking to comply with the locking rules. A couple of mutex
unlock errors spotted: not being unlocked on malloc failure and an
unnecessary unlock in netfs_attempt_rename().
patch 11: The final patch fixes the handling of replies to MKNOD and
SYMLINK. The Linux NFS server does not accept a setting of file size to
0 for sockets and FIFOs so there is code to strip that for those cases.
I also spotted that the file modification times were not being applied
to a created file so there is now just one rpc to set all file
attributes using xdr_encode_create_state(). This sets the times to be
server time which is appropriate for a new file (even though it was
created in the previous RPC call). The previous code was supplying the
'gid' based on that returned by the 'stat' but I don't think that can be
relied on since the V3 protocol states that CREATE3 using EXCLUSIVE
cannot guarantee any file attributes until after the SETATTR3.
I have tested mknod FIFO but I haven't found a method of testing mknod
block and character devices. Hurd does not support Unix domain socket on
NFS (no ifsock_getsockaddr in libnetfs) but I did test it artificially
with success so should work if that gets altered.
I still have to think about what should happen in the case where the
CREATE3 succeeds but the SETATTR3 does not. For the time being there is
a comment in the code as a reminder.
Regards,
Mike.
diff -ur nfs.submitted/ops.c nfs.new/ops.c
--- nfs.submitted/ops.c 2025-07-13 20:30:34.000000000 +0100
+++ nfs.new/ops.c 2025-07-18 07:31:48.000000000 +0100
@@ -231,11 +231,11 @@
return err;
}
-/* Implement the netfs_attempt_chown callback as described in
- <hurd/netfs.h>. */
-error_t
-netfs_attempt_chown (struct iouser *cred, struct node *np,
- uid_t uid, gid_t gid)
+/* Generalised implementation of PROC_SETATTR rpc call requiring
+ only a localised sattr encoding function for each caller. */
+static error_t
+nfs_setattr_rpc (struct iouser *cred, struct node *np, gid_t gid,
+ int *(sattr_encoder) (int *))
{
int *p;
void *rpcbuf;
@@ -247,7 +247,7 @@
return errno;
p = xdr_encode_fhandle (p, &np->nn->handle);
- p = xdr_encode_sattr_ids (p, uid, gid);
+ p = (sattr_encoder) (p);
if (protocol_version == 3)
*(p++) = 0; /* guard_check == 0 */
@@ -265,6 +265,20 @@
return err;
}
+/* Implement the netfs_attempt_chown callback as described in
+ <hurd/netfs.h>. */
+error_t
+netfs_attempt_chown (struct iouser *cred, struct node *np,
+ uid_t uid, gid_t gid)
+{
+ int *_chown_sattr_encoder (int *p)
+ {
+ return xdr_encode_sattr_ids (p, uid, gid);
+ }
+
+ return nfs_setattr_rpc (cred, np, gid, _chown_sattr_encoder);
+}
+
/* Implement the netfs_attempt_chauthor callback as described in
<hurd/netfs.h>. */
error_t
@@ -280,13 +294,9 @@
netfs_attempt_chmod (struct iouser *cred, struct node *np,
mode_t mode)
{
- int *p;
- void *rpcbuf;
- error_t err;
-
if ((mode & S_IFMT) != 0)
{
- err = netfs_validate_stat (np, cred);
+ error_t err = netfs_validate_stat (np, cred);
if (err)
return err;
@@ -321,27 +331,12 @@
}
}
- p = nfs_initialize_rpc (NFSPROC_SETATTR (protocol_version),
- cred, 0, &rpcbuf, np, -1);
- if (! p)
- return errno;
-
- p = xdr_encode_fhandle (p, &np->nn->handle);
- p = xdr_encode_sattr_mode (p, mode);
- if (protocol_version == 3)
- *(p++) = 0; /* guard check == 0 */
-
- err = conduct_rpc (&rpcbuf, &p);
- if (!err)
- {
- err = nfs_error_trans (ntohl (*p));
- p++;
- if (!err || protocol_version == 3)
- p = process_wcc_stat (np, p, !err);
- }
+ int *_chmod_sattr_encoder (int *p)
+ {
+ return xdr_encode_sattr_mode (p, mode);
+ }
- free (rpcbuf);
- return err;
+ return nfs_setattr_rpc (cred, np, -1, _chmod_sattr_encoder);
}
/* Implement the netfs_attempt_chflags callback as described in
@@ -359,37 +354,18 @@
netfs_attempt_utimes (struct iouser *cred, struct node *np,
struct timespec *atime, struct timespec *mtime)
{
- int *p;
- void *rpcbuf;
- error_t err;
-
if (!atime && !mtime)
return 0; /* nothing to update */
/* XXX For version 3 we can actually do this right, but we don't
just yet. */
- p = nfs_initialize_rpc (NFSPROC_SETATTR (protocol_version),
- cred, 0, &rpcbuf, np, -1);
- if (! p)
- return errno;
-
- p = xdr_encode_fhandle (p, &np->nn->handle);
- p = xdr_encode_sattr_times (p, atime, mtime);
- if (protocol_version == 3)
- *(p++) = 0; /* guard check == 0 */
+ int *_utimes_sattr_encoder (int *p)
+ {
+ return xdr_encode_sattr_times (p, atime, mtime);
+ }
- err = conduct_rpc (&rpcbuf, &p);
- if (!err)
- {
- err = nfs_error_trans (ntohl (*p));
- p++;
- if (!err || protocol_version == 3)
- p = process_wcc_stat (np, p, !err);
- }
-
- free (rpcbuf);
- return err;
+ return nfs_setattr_rpc (cred, np, -1, _utimes_sattr_encoder);
}
/* Implement the netfs_attempt_set_size callback as described in
@@ -398,28 +374,14 @@
netfs_attempt_set_size (struct iouser *cred, struct node *np,
off_t size)
{
- int *p;
- void *rpcbuf;
error_t err;
- p = nfs_initialize_rpc (NFSPROC_SETATTR (protocol_version),
- cred, 0, &rpcbuf, np, -1);
- if (! p)
- return errno;
-
- p = xdr_encode_fhandle (p, &np->nn->handle);
- p = xdr_encode_sattr_size (p, size);
- if (protocol_version == 3)
- *(p++) = 0; /* guard_check == 0 */
+ int *_size_sattr_encoder (int *p)
+ {
+ return xdr_encode_sattr_size (p, size);
+ }
- err = conduct_rpc (&rpcbuf, &p);
- if (!err)
- {
- err = nfs_error_trans (ntohl (*p));
- p++;
- if (!err || protocol_version == 3)
- p = process_wcc_stat (np, p, !err);
- }
+ err = nfs_setattr_rpc (cred, np, -1, _size_sattr_encoder);
/* If we got EACCES, but the user has the file open for writing,
then the NFS protocol has screwed us. There's nothing we can do,
@@ -435,7 +397,6 @@
err = 0;
}
- free (rpcbuf);
return err;
}
diff -ur nfs.submitted/ops.c nfs.new/ops.c
--- nfs.submitted/ops.c 2025-07-18 07:31:48.000000000 +0100
+++ nfs.new/ops.c 2025-07-18 09:00:21.000000000 +0100
@@ -27,6 +27,19 @@
#include <maptime.h>
#include <sys/sysmacros.h>
+/* Rules for locking/unlocking of 'struct node'->lock
+ applicable throughout the whole module:
+ 1) Only lock a single node at a time except when
+ 2) A new node is created beneath a directory when the
+ directory node->lock is held but
+ 3) Once the directory lock is released then 2) no
+ longer applies and rule 1) applies to the new node.
+*/
+
+static error_t
+nfs_lookup_rpc (struct iouser *cred, struct node *np,
+ const char *name, struct node **newnp);
+
/* We have fresh stat information for NP; the file attribute (fattr)
structure is at P. Update our entry. Return the address of the next
int after the fattr structure. */
@@ -736,12 +749,6 @@
netfs_attempt_lookup (struct iouser *cred, struct node *np,
const char *name, struct node **newnp)
{
- int *p;
- void *rpcbuf;
- error_t err;
- char dirhandle[NFS3_FHSIZE];
- size_t dirlen;
-
/* Check the cache first. */
*newnp = check_lookup_cache (np, name);
if (*newnp)
@@ -755,6 +762,23 @@
return 0;
}
+ return nfs_lookup_rpc (cred, np, name, newnp);
+}
+
+/* LOOKUP rpc call to refetch the status of 'name' in directory
+ 'np' which is supplied locked. Any '*newnp' supplied should be
+ unlocked and will be recached with the returned handle. On
+ return 'np' is unlocked and any '*newnp' is locked. */
+static error_t
+nfs_lookup_rpc (struct iouser *cred, struct node *np,
+ const char *name, struct node **newnp)
+{
+ int *p;
+ void *rpcbuf;
+ error_t err;
+ char dirhandle[NFS3_FHSIZE];
+ size_t dirlen;
+
p = nfs_initialize_rpc (NFSPROC_LOOKUP (protocol_version),
cred, 0, &rpcbuf, np, -1);
if (! p)
@@ -780,27 +804,33 @@
p++;
if (!err)
{
- p = xdr_decode_fhandle (p, newnp);
+ if (*newnp != NULL)
+ {
+ pthread_mutex_lock (&(*newnp)->lock);
+ p = recache_handle (p, *newnp);
+ }
+ else
+ p = xdr_decode_fhandle (p, newnp);
+
p = process_returned_stat (*newnp, p, 1);
}
- if (err)
- *newnp = 0;
if (protocol_version == 3)
{
if (*newnp)
pthread_mutex_unlock (&(*newnp)->lock);
pthread_mutex_lock (&np->lock);
- p = process_returned_stat (np, p, 0); /* XXX Do we have to lock np? */
+ p = process_returned_stat (np, p, 0);
pthread_mutex_unlock (&np->lock);
if (*newnp)
pthread_mutex_lock (&(*newnp)->lock);
}
}
- else
- *newnp = 0;
- /* Notify the cache of the hit or miss. */
- enter_lookup_cache (dirhandle, dirlen, *newnp, name);
+ if (!err || err == ENOENT)
+ {
+ /* Notify the cache of the hit or miss. */
+ enter_lookup_cache (dirhandle, dirlen, *newnp, name);
+ }
free (rpcbuf);
diff -ur nfs.submitted/ops.c nfs.new/ops.c
--- nfs.submitted/ops.c 2025-07-18 09:13:46.000000000 +0100
+++ nfs.new/ops.c 2025-07-18 10:01:28.000000000 +0100
@@ -167,8 +167,10 @@
}
/* 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. */
+ the same reply content. 'np' should be supplied unlocked.
+ SYMLINK and MKNOD creation involve the translation of an
+ existing node '*newnp' that is supplied locked. On return
+ any '*newnp' is locked and 'np' is returned unlocked. */
static error_t
process_create_reply (struct iouser *cred,
struct node *np,
@@ -188,25 +190,37 @@
if (handle_follows)
{
- p = xdr_decode_fhandle (p, newnp);
+ p = (*newnp != NULL
+ ? recache_handle (p, *newnp)
+ : xdr_decode_fhandle (p, newnp));
p = process_returned_stat (*newnp, p, 1);
}
else
- /* These will be refetched in LOOKUP */
+ /* These will be refetched by nfs_lookup_rpc () */
p = skip_returned_stat (p);
+ if (*newnp)
+ pthread_mutex_unlock (&(*newnp)->lock);
+
+ pthread_mutex_lock (&np->lock);
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);
+ err = nfs_lookup_rpc (cred, np, name, newnp);
}
+ else
+ pthread_mutex_unlock (&np->lock);
}
else
{
+ 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);
}
return err;
@@ -869,6 +883,8 @@
p = xdr_encode_string (p, name);
p = xdr_encode_create_state (p, mode, owner);
+ pthread_mutex_unlock (&np->lock);
+
err = conduct_rpc (&rpcbuf, &p);
if (!err)
{
@@ -884,7 +900,10 @@
}
}
else
- err = process_create_reply (cred, np, name, &newnp, p);
+ {
+ newnp = NULL;
+ err = process_create_reply (cred, np, name, &newnp, p);
+ }
}
if (!err)
@@ -899,6 +918,8 @@
netfs_nput (newnp);
}
+ pthread_mutex_lock (&np->lock);
+
free (rpcbuf);
return err;
}
@@ -1253,16 +1274,17 @@
name = malloc (50);
if (! name)
- return ENOMEM;
+ {
+ pthread_mutex_unlock (&dir->lock);
+ return ENOMEM;
+ }
do
{
sprintf (name, ".nfstmpgnu.%d", n++);
err = netfs_attempt_create_file (cred, dir, name, mode, newnp);
if (err == EEXIST)
- pthread_mutex_lock (&dir->lock); /* XXX is this right? does create need this
- and drop this on error? Doesn't look
- like it. */
+ pthread_mutex_lock (&dir->lock);
}
while (err == EEXIST);
@@ -1342,6 +1364,8 @@
err = conduct_rpc (&rpcbuf, &p);
*newnp = 0;
+ pthread_mutex_unlock (&np->lock);
+
if (!err)
{
if (protocol_version == 2)
@@ -1357,8 +1381,6 @@
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)->nn_stat.st_uid != owner)
@@ -1368,8 +1390,6 @@
err = netfs_attempt_chmod (cred, *newnp, mode);
}
}
- else
- pthread_mutex_unlock (&np->lock);
free (rpcbuf);
return err;
@@ -1500,7 +1520,6 @@
pthread_mutex_lock (&fromdir->lock);
err = netfs_attempt_lookup (cred, fromdir, fromname, &np);
- pthread_mutex_unlock (&fromdir->lock);
if (err)
return err;
diff -ur nfs.submitted/nfs.c nfs.new/nfs.c
--- nfs.submitted/nfs.c 2025-07-18 09:12:50.000000000 +0100
+++ nfs.new/nfs.c 2025-07-18 10:17:50.000000000 +0100
@@ -25,6 +25,7 @@
#include <string.h>
#include <netinet/in.h>
+#include <stdbool.h>
#include <stdio.h>
#include <sys/sysmacros.h>
@@ -179,6 +180,30 @@
return xdr_encode_data (p, string, strlen (string));
}
+/* Some NFSv3 rpcs can fail if the file size field
+ is set within the set attributes unnecessarily. */
+static inline bool
+nfs_sattr3_size_needed(mode_t mode, size_t sz)
+{
+ if (sz == 0)
+ {
+ switch (hurd_mode_to_nfs_type (mode))
+ {
+ case NFSOCK:
+ case NF3FIFO:
+ /* NFCHR and NFBLK probaly also fall into this
+ category but as unable to check then leave
+ those alone. */
+ return false;
+
+ default:
+ break;
+ }
+ }
+
+ return true;
+}
+
/* Encode a MODE into an otherwise empty sattr. */
int *
xdr_encode_sattr_mode (int *p, mode_t mode)
@@ -372,14 +397,17 @@
}
else
{
+ bool needs_size = nfs_sattr3_size_needed (st->st_mode, st->st_size);
+
*(p++) = htonl (1); /* set mode */
*(p++) = htonl (hurd_mode_to_nfs_mode (st->st_mode));
*(p++) = htonl (1); /* set uid */
*(p++) = htonl (st->st_uid);
*(p++) = htonl (1); /* set gid */
*(p++) = htonl (st->st_gid);
- *(p++) = htonl (1); /* set size */
- p = xdr_encode_64bit (p, st->st_size);
+ *(p++) = htonl (needs_size); /* set size */
+ if (needs_size)
+ p = xdr_encode_64bit (p, st->st_size);
*(p++) = htonl (SET_TO_CLIENT_TIME); /* set atime */
*(p++) = htonl (st->st_atim.tv_sec);
*(p++) = htonl (st->st_atim.tv_nsec);
diff -ur nfs.submitted/ops.c nfs.new/ops.c
--- nfs.submitted/ops.c 2025-07-18 10:12:31.000000000 +0100
+++ nfs.new/ops.c 2025-07-18 10:48:34.000000000 +0100
@@ -1056,8 +1056,6 @@
}
pthread_mutex_unlock (&np->lock);
- pthread_mutex_lock (&dir->lock);
-
purge_lookup_cache (dir, name, strlen (name));
err = conduct_rpc (&rpcbuf, &p);
if (!err)
@@ -1068,6 +1066,7 @@
if (protocol_version == 2 && !err)
{
free (rpcbuf);
+ pthread_mutex_lock (&dir->lock);
/* NFSPROC_SYMLINK stupidly does not pass back an
fhandle, so we have to fetch one now. */
@@ -1101,23 +1100,14 @@
}
else if (protocol_version == 3)
{
- if (!err)
- {
- pthread_mutex_unlock (&dir->lock);
- pthread_mutex_lock (&np->lock);
- p = recache_handle (p, np);
- p = process_returned_stat (np, p, 1);
- pthread_mutex_unlock (&np->lock);
- pthread_mutex_lock (&dir->lock);
- }
- p = process_wcc_stat (dir, p, !err);
- pthread_mutex_unlock (&dir->lock);
+ /* process_create_reply needs the start of the buffer */
+ p--;
+
+ pthread_mutex_lock (&np->lock);
+ err = process_create_reply (cred, dir, name, &np, p);
+ pthread_mutex_unlock (&np->lock);
}
- else
- pthread_mutex_unlock (&dir->lock);
}
- else
- pthread_mutex_unlock (&dir->lock);
free (rpcbuf);
break;
@@ -1213,19 +1203,9 @@
err = conduct_rpc (&rpcbuf, &p);
if (!err)
{
- err = nfs_error_trans (ntohl (*p));
- p++;
-
- if (!err)
- {
- pthread_mutex_lock (&np->lock);
- p = recache_handle (p, np);
- p = process_returned_stat (np, p, 1);
- pthread_mutex_unlock (&np->lock);
- }
- pthread_mutex_lock (&dir->lock);
- p = process_wcc_stat (dir, p, !err);
- pthread_mutex_unlock (&dir->lock);
+ pthread_mutex_lock (&np->lock);
+ err = process_create_reply (cred, dir, name, &np, p);
+ pthread_mutex_unlock (&np->lock);
}
free (rpcbuf);
}
@@ -1381,13 +1361,15 @@
else
err = process_create_reply (cred, np, name, newnp, p);
- if (*newnp && !netfs_validate_stat (*newnp, (struct iouser *) -1))
+ if (protocol_version == 3 && !err)
{
- if ((*newnp)->nn_stat.st_uid != owner)
- err = netfs_attempt_chown ((struct iouser *) -1, *newnp, owner, (*newnp)->nn_stat.st_gid);
+ int *_cs_sattr_encoder (int * sp)
+ {
+ return xdr_encode_create_state (sp, mode, owner);
+ }
- if (!err && (*newnp)->nn_stat.st_mode != mode)
- err = netfs_attempt_chmod (cred, *newnp, mode);
+ err = nfs_setattr_rpc(cred, *newnp, -1, _cs_sattr_encoder);
+ /* Unlink *newnp if err ? */
}
}
Reply to: