Re: Hurd NFS translator
Hello,
Applied, thanks!
Samuel
Michael Kelly, le ven. 18 juil. 2025 15:33:43 +0100, a ecrit:
> 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 ? */
> }
> }
>
--
Samuel
"c'est pas nous qui sommes à la rue, c'est la rue qui est à nous"
Reply to: